[comctl32] Fix SEGV in file dialogs (bug 3366)

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[comctl32] Fix SEGV in file dialogs (bug 3366)

Troy Rollo
The following change seems to prevent the SEGV that frequently occurs when you
double-click on a folder in the file open/save dialogs. This fix uses
IsWindow() to test if the ListView still exists. I have handled all
notifications other than those where it seems entirely implausible that the
callback could destroy the window.

ChangeLog:
        Detect when the notification callback has destroyed the ListView to avoid
        attempts to access data that is no longer valid.

=== dlls/comctl32/listview.c
==================================================================
--- dlls/comctl32/listview.c (revision 43320)
+++ dlls/comctl32/listview.c (local)
@@ -3018,6 +3018,7 @@
 {
     INT nFirst = min(infoPtr->nSelectionMark, nItem);
     INT nLast = max(infoPtr->nSelectionMark, nItem);
+    HWND hwndSelf = infoPtr->hwndSelf;
     NMLVODSTATECHANGE nmlv;
     LVITEMW item;
     BOOL bOldChange;
@@ -3046,6 +3047,8 @@
     nmlv.uOldState = item.state;
 
     notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv);
+    if (!IsWindow(hwndSelf))
+ return;
     infoPtr->bDoChangeNotify = bOldChange;
 }
 
@@ -3400,9 +3403,15 @@
     /* send LVN_ITEMCHANGING notification, if the item is not being inserted */
     /* and we are _NOT_ virtual (LVS_OWERNDATA), and change notifications */
     /* are enabled */
-    if(lpItem && !isNew && infoPtr->bDoChangeNotify &&
-       notify_listview(infoPtr, LVN_ITEMCHANGING, &nmlv))
+    if(lpItem && !isNew && infoPtr->bDoChangeNotify)
+    {
+      HWND hwndSelf = infoPtr->hwndSelf;
+
+      if (notify_listview(infoPtr, LVN_ITEMCHANGING, &nmlv))
  return FALSE;
+      if (!IsWindow(hwndSelf))
+ return FALSE;
+    }
 
     /* copy information */
     if (lpLVItem->mask & LVIF_TEXT)
@@ -3546,6 +3555,7 @@
 static BOOL LISTVIEW_SetItemT(LISTVIEW_INFO *infoPtr, const LVITEMW *lpLVItem, BOOL isW)
 {
     UINT uView = infoPtr->dwStyle & LVS_TYPEMASK;
+    HWND hwndSelf = infoPtr->hwndSelf;
     LPWSTR pszText = NULL;
     BOOL bResult, bChanged = FALSE;
     
@@ -3568,6 +3578,8 @@
  bResult = set_sub_item(infoPtr, lpLVItem, TRUE, &bChanged);
     else
  bResult = set_main_item(infoPtr, lpLVItem, FALSE, TRUE, &bChanged);
+    if (!IsWindow(hwndSelf))
+ return FALSE;
 
     /* redraw item, if necessary */
     if (bChanged && !infoPtr->bIsDrawing)
@@ -4504,6 +4516,7 @@
 static BOOL LISTVIEW_DeleteItem(LISTVIEW_INFO *infoPtr, INT nItem)
 {
     UINT uView = infoPtr->dwStyle & LVS_TYPEMASK;
+    HWND hwndSelf = infoPtr->hwndSelf;
     LVITEMW item;
 
     TRACE("(nItem=%d)\n", nItem);
@@ -4517,6 +4530,8 @@
     
     /* send LVN_DELETEITEM notification. */
     notify_deleteitem(infoPtr, nItem);
+    if (!IsWindow(hwndSelf))
+ return FALSE;
 
     /* we need to do this here, because we'll be deleting stuff */  
     if (uView == LVS_SMALLICON || uView == LVS_ICON)
@@ -4568,6 +4583,7 @@
  */
 static BOOL LISTVIEW_EndEditLabelT(LISTVIEW_INFO *infoPtr, LPWSTR pszText, BOOL isW)
 {
+    HWND hwndSelf = infoPtr->hwndSelf;
     NMLVDISPINFOW dispInfo;
 
     TRACE("(pszText=%s, isW=%d)\n", debugtext_t(pszText, isW), isW);
@@ -4585,6 +4601,8 @@
 
     /* Do we need to update the Item Text */
     if (!notify_dispinfoT(infoPtr, LVN_ENDLABELEDITW, &dispInfo, isW)) return FALSE;
+    if (!IsWindow(hwndSelf))
+ return FALSE;
     if (!pszText) return TRUE;
 
     if (!(infoPtr->dwStyle & LVS_OWNERDATA))
@@ -4625,6 +4643,7 @@
     WCHAR szDispText[DISP_TEXT_SIZE] = { 0 };
     NMLVDISPINFOW dispInfo;
     RECT rect;
+    HWND hwndSelf = infoPtr->hwndSelf;
 
     TRACE("(nItem=%d, isW=%d)\n", nItem, isW);
 
@@ -4662,6 +4681,8 @@
     
     if (notify_dispinfoT(infoPtr, LVN_BEGINLABELEDITW, &dispInfo, isW))
     {
+ if (!IsWindow(hwndSelf))
+    return 0;
  SendMessageW(infoPtr->hwndEdit, WM_CLOSE, 0, 0);
  infoPtr->hwndEdit = 0;
  return 0;
@@ -6191,6 +6212,7 @@
     ITEM_INFO *lpItem;
     BOOL is_sorted, has_changed;
     LVITEMW item;
+    HWND hwndSelf = infoPtr->hwndSelf;
 
     TRACE("(lpLVItem=%s, isW=%d)\n", debuglvitem_t(lpLVItem, isW), isW);
 
@@ -6264,6 +6286,8 @@
     nmlv.iItem = nItem;
     nmlv.lParam = lpItem->lParam;
     notify_listview(infoPtr, LVN_INSERTITEM, &nmlv);
+    if (!IsWindow(hwndSelf))
+ return -1;
 
     /* align items (set position of each item) */
     if ((uView == LVS_SMALLICON || uView == LVS_ICON))
@@ -7967,6 +7991,7 @@
 static LRESULT LISTVIEW_KeyDown(LISTVIEW_INFO *infoPtr, INT nVirtualKey, LONG lKeyData)
 {
   UINT uView =  infoPtr->dwStyle & LVS_TYPEMASK;
+  HWND hwndSelf = infoPtr->hwndSelf;
   INT nItem = -1;
   NMLVKEYDOWN nmKeyDown;
 
@@ -7976,6 +8001,8 @@
   nmKeyDown.wVKey = nVirtualKey;
   nmKeyDown.flags = 0;
   notify_hdr(infoPtr, LVN_KEYDOWN, &nmKeyDown.hdr);
+  if (!IsWindow(hwndSelf))
+    return 0;
 
   switch (nVirtualKey)
   {
@@ -7983,7 +8010,11 @@
     if ((infoPtr->nItemCount > 0) && (infoPtr->nFocusedItem != -1))
     {
       notify(infoPtr, NM_RETURN);
+      if (!IsWindow(hwndSelf))
+ return 0;
       notify(infoPtr, LVN_ITEMACTIVATE);
+      if (!IsWindow(hwndSelf))
+ return 0;
     }
     break;
 
@@ -8063,6 +8094,7 @@
  */
 static LRESULT LISTVIEW_KillFocus(LISTVIEW_INFO *infoPtr)
 {
+    HWND hwndSelf = infoPtr->hwndSelf;
     TRACE("()\n");
 
     /* if we did not have the focus, there's nothing to do */
@@ -8070,6 +8102,8 @@
   
     /* send NM_KILLFOCUS notification */
     notify(infoPtr, NM_KILLFOCUS);
+    if (!IsWindow(hwndSelf))
+ return 0;
 
     /* if we have a focus rectagle, get rid of it */
     LISTVIEW_ShowFocusRect(infoPtr, FALSE);
@@ -8098,11 +8132,14 @@
 static LRESULT LISTVIEW_LButtonDblClk(LISTVIEW_INFO *infoPtr, WORD wKey, INT x, INT y)
 {
     LVHITTESTINFO htInfo;
+    HWND hwndSelf = infoPtr->hwndSelf;
 
     TRACE("(key=%hu, X=%hu, Y=%hu)\n", wKey, x, y);
 
     /* send NM_RELEASEDCAPTURE notification */
     notify(infoPtr, NM_RELEASEDCAPTURE);
+    if (!IsWindow(hwndSelf))
+ return 0;
 
     htInfo.pt.x = x;
     htInfo.pt.y = y;
@@ -8110,6 +8147,8 @@
     /* send NM_DBLCLK notification */
     LISTVIEW_HitTest(infoPtr, &htInfo, TRUE, FALSE);
     notify_click(infoPtr, NM_DBLCLK, &htInfo);
+    if (!IsWindow(hwndSelf))
+    return 0;
 
     /* To send the LVN_ITEMACTIVATE, it must be on an Item */
     if(htInfo.iItem != -1) notify_itemactivate(infoPtr,&htInfo);
@@ -8134,12 +8173,15 @@
   LVHITTESTINFO lvHitTestInfo;
   static BOOL bGroupSelect = TRUE;
   POINT pt = { x, y };
+  HWND hwndSelf = infoPtr->hwndSelf;
   INT nItem;
 
   TRACE("(key=%hu, X=%hu, Y=%hu)\n", wKey, x, y);
 
   /* send NM_RELEASEDCAPTURE notification */
   notify(infoPtr, NM_RELEASEDCAPTURE);
+  if (!IsWindow(hwndSelf))
+  return FALSE;
 
   if (!infoPtr->bFocus) SetFocus(infoPtr->hwndSelf);
 
@@ -8183,6 +8225,8 @@
         if (bGroupSelect)
  {
           LISTVIEW_AddGroupSelection(infoPtr, nItem);
+  if (!IsWindow(hwndSelf))
+    return 0;
       LISTVIEW_SetItemFocus(infoPtr, nItem);
           infoPtr->nSelectionMark = nItem;
  }
@@ -8247,6 +8291,7 @@
 static LRESULT LISTVIEW_LButtonUp(LISTVIEW_INFO *infoPtr, WORD wKey, INT x, INT y)
 {
     LVHITTESTINFO lvHitTestInfo;
+    HWND hwndSelf = infoPtr->hwndSelf;
     
     TRACE("(key=%hu, X=%hu, Y=%hu)\n", wKey, x, y);
 
@@ -8258,6 +8303,8 @@
     /* send NM_CLICK notification */
     LISTVIEW_HitTest(infoPtr, &lvHitTestInfo, TRUE, FALSE);
     notify_click(infoPtr, NM_CLICK, &lvHitTestInfo);
+    if (!IsWindow(hwndSelf))
+ return 0;
 
     /* set left button flag */
     infoPtr->bLButtonDown = FALSE;
@@ -8332,6 +8379,7 @@
 static LRESULT LISTVIEW_HeaderNotification(LISTVIEW_INFO *infoPtr, const NMHEADERW *lpnmh)
 {
     UINT uView =  infoPtr->dwStyle & LVS_TYPEMASK;
+    HWND hwndSelf = infoPtr->hwndSelf;
     
     TRACE("(lpnmh=%p)\n", lpnmh);
 
@@ -8345,6 +8393,8 @@
  case HDN_ITEMCHANGEDW:
  case HDN_ITEMCHANGEDA:
             notify_forward_header(infoPtr, lpnmh);
+    if (!IsWindow(hwndSelf))
+ break;
             /* Fall through */
  case HDN_TRACKW:
  case HDN_TRACKA:
@@ -8571,11 +8621,14 @@
 static LRESULT LISTVIEW_RButtonDblClk(LISTVIEW_INFO *infoPtr, WORD wKey, INT x, INT y)
 {
     LVHITTESTINFO lvHitTestInfo;
+    HWND hwndSelf = infoPtr->hwndSelf;
     
     TRACE("(key=%hu,X=%hu,Y=%hu)\n", wKey, x, y);
 
     /* send NM_RELEASEDCAPTURE notification */
     notify(infoPtr, NM_RELEASEDCAPTURE);
+    if (!IsWindow(hwndSelf))
+ return 0;
 
     /* send NM_RDBLCLK notification */
     lvHitTestInfo.pt.x = x;
@@ -8602,11 +8655,14 @@
 {
     LVHITTESTINFO lvHitTestInfo;
     INT nItem;
+    HWND hwndSelf = infoPtr->hwndSelf;
 
     TRACE("(key=%hu,X=%hu,Y=%hu)\n", wKey, x, y);
 
     /* send NM_RELEASEDCAPTURE notification */
     notify(infoPtr, NM_RELEASEDCAPTURE);
+    if (!IsWindow(hwndSelf))
+ return 0;
 
     /* make sure the listview control window has the focus */
     if (!infoPtr->bFocus) SetFocus(infoPtr->hwndSelf);
@@ -8649,6 +8705,7 @@
 static LRESULT LISTVIEW_RButtonUp(LISTVIEW_INFO *infoPtr, WORD wKey, INT x, INT y)
 {
     LVHITTESTINFO lvHitTestInfo;
+    HWND hwndSelf = infoPtr->hwndSelf;
     POINT pt;
 
     TRACE("(key=%hu,X=%hu,Y=%hu)\n", wKey, x, y);
@@ -8663,6 +8720,8 @@
     lvHitTestInfo.pt.y = y;
     LISTVIEW_HitTest(infoPtr, &lvHitTestInfo, TRUE, FALSE);
     notify_click(infoPtr, NM_RCLICK, &lvHitTestInfo);
+    if (!IsWindow(hwndSelf))
+ return 0;
 
     /* Change to screen coordinate for WM_CONTEXTMENU */
     pt = lvHitTestInfo.pt;
@@ -8719,6 +8778,8 @@
  */
 static LRESULT LISTVIEW_SetFocus(LISTVIEW_INFO *infoPtr, HWND hwndLoseFocus)
 {
+    HWND hwndSelf = infoPtr->hwndSelf;
+
     TRACE("(hwndLoseFocus=%p)\n", hwndLoseFocus);
 
     /* if we have the focus already, there's nothing to do */
@@ -8726,6 +8787,8 @@
   
     /* send NM_SETFOCUS notification */
     notify(infoPtr, NM_SETFOCUS);
+    if (!IsWindow(hwndSelf))
+ return 0;
 
     /* set window focus flag */
     infoPtr->bFocus = TRUE;


Reply | Threaded
Open this post in threaded view
|

Re: [comctl32] Fix SEGV in file dialogs (bug 3366)

Alexandre Julliard
Troy Rollo <[hidden email]> writes:

> The following change seems to prevent the SEGV that frequently occurs when you
> double-click on a folder in the file open/save dialogs. This fix uses
> IsWindow() to test if the ListView still exists. I have handled all
> notifications other than those where it seems entirely implausible that the
> callback could destroy the window.

I think it would be a lot cleaner to put the IsWindow test in the
notify functions and make them return FALSE (or some appropriate code)
when the window is destroyed. Then all you have to do in the callers
is check the result instead of duplicating the IsWindow calls all over
the place.

--
Alexandre Julliard
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: [comctl32] Fix SEGV in file dialogs (bug 3366)

Troy Rollo
On Wed, 12 Oct 2005 06:05, Alexandre Julliard wrote:
> I think it would be a lot cleaner to put the IsWindow test in the
> notify functions and make them return FALSE (or some appropriate code)
> when the window is destroyed. Then all you have to do in the callers
> is check the result instead of duplicating the IsWindow calls all over
> the place.

In most cases this would work, but there are a few cases where a value
returned from the callback is used or tested for something else, including
one where a return is made if the return value is !FALSE and not if it is
FALSE, and one where execution should continue regardless of the return value
of the notification. The latter is more problematic since it would require
either some magic value that we hope the app does not return or a different
notification function that does not interfere with the value returned by the
application.