Changeset 52830 in webkit

Timestamp:

Jan 5, 2010 3:07:33 PM (14 years ago)

Author:

Adam Roben

Message:

Make IWebView::close and destroying a WebView's HWND optional for WebKit clients

WebView will now take care of these operations itself when its last
reference is released, if they haven't already been done.

IWebView::close now also destroys the WebView's HWND. All WebKit
clients were already performing these operations in succession anyway,
or were attempting to by calling IWebView::close then destroying the
WebView's host window (which actually resulted in the WebView's HWND
leaking, and the crash in the below bug).

Fixes <​http://webkit.org/b/32827> Crash when calling IWebView::close,
then releasing the WebView, without calling DestroyWindow

Fixes a few WebViewDestruction tests, too.

Reviewed by Steve Falkenburg.

• WebView.cpp:

(WebView::~WebView): Don't try to destroy m_viewWindow here. That
should already have happened. Assert that this is the case.
(WebView::close): If m_viewWindow isn't already being destroyed,
destroy it now. Moved the call to revokeDragDrop() here from our
WM_DESTROY handler because it needs to be done before m_viewWindow is
nulled out.
(WebView::WebViewWndProc): Removed call to revokeDragDrop() that
close() now performs.
(WebView::Release): If our last reference is being released, call
close() so that clients don't have to. (It's harmless to call close()
multiple times.) We do this here instead of in the destructor because
close() can cause AddRef() and Release() to be called, and calling
those from within the destructor leads to double-destruction.
(WebView::setHostWindow): Removed an unnecessary (and now harmful)
null-check.
(WebView::revokeDragDrop): Changed an assertion into a run-time check,
since this will now sometimes be called when m_viewWindow hasn't been
created yet. Changed the IsWindow call to a null-check because we
never hold onto a destroyed m_viewWindow.
(WebView::windowAncestryDidChange): If we don't have a view window,
stop tracking changes to our parent's active state.

Location:

trunk/WebKit/win

Files:

• ChangeLog (modified) (1 diff)
• WebView.cpp (modified) (8 diffs)

trunk/WebKit/win/ChangeLog

@@ +1 @@
+2010-01-05  Adam Roben  <aroben@apple.com>
+
+        Make IWebView::close and destroying a WebView's HWND optional for
+        WebKit clients
+
+        WebView will now take care of these operations itself when its last
+        reference is released, if they haven't already been done.
+
+        IWebView::close now also destroys the WebView's HWND. All WebKit
+        clients were already performing these operations in succession anyway,
+        or were attempting to by calling IWebView::close then destroying the
+        WebView's host window (which actually resulted in the WebView's HWND
+        leaking, and the crash in the below bug).
+
+        Fixes <http://webkit.org/b/32827> Crash when calling IWebView::close,
+        then releasing the WebView, without calling DestroyWindow
+
+        Fixes a few WebViewDestruction tests, too.
+
+        Reviewed by Steve Falkenburg.
+
+        * WebView.cpp:
+        (WebView::~WebView): Don't try to destroy m_viewWindow here. That
+        should already have happened. Assert that this is the case.
+        (WebView::close): If m_viewWindow isn't already being destroyed,
+        destroy it now. Moved the call to revokeDragDrop() here from our
+        WM_DESTROY handler because it needs to be done before m_viewWindow is
+        nulled out.
+        (WebView::WebViewWndProc): Removed call to revokeDragDrop() that
+        close() now performs.
+        (WebView::Release): If our last reference is being released, call
+        close() so that clients don't have to. (It's harmless to call close()
+        multiple times.) We do this here instead of in the destructor because
+        close() can cause AddRef() and Release() to be called, and calling
+        those from within the destructor leads to double-destruction.
+        (WebView::setHostWindow): Removed an unnecessary (and now harmful)
+        null-check.
+        (WebView::revokeDragDrop): Changed an assertion into a run-time check,
+        since this will now sometimes be called when m_viewWindow hasn't been
+        created yet. Changed the IsWindow call to a null-check because we
+        never hold onto a destroyed m_viewWindow.
+        (WebView::windowAncestryDidChange): If we don't have a view window,
+        stop tracking changes to our parent's active state.
+
 2010-01-05  Adam Roben  <aroben@apple.com>
 

trunk/WebKit/win/WebView.cpp

@@ -355 +355 @@
     deleteBackingStore();
 
-    // <rdar://4958382> m_viewWindow will be destroyed when m_hostWindow is destroyed, but if
-    // setHostWindow was never called we will leak our HWND. If we still have a valid HWND at
-    // this point, we should just destroy it ourselves.
-    if (!isBeingDestroyed() && ::IsWindow(m_viewWindow))
-        ::DestroyWindow(m_viewWindow);
-
     // the tooltip window needs to be explicitly destroyed since it isn't a WS_CHILD
     if (::IsWindow(m_toolTipHwnd))
@@ -367 +361 @@
     ASSERT(!m_page);
     ASSERT(!m_preferences);
+    ASSERT(!m_viewWindow);
 
     WebViewCount--;
@@ -647 +642 @@
     }
     
+    revokeDragDrop();
+
+    if (m_viewWindow) {
+        // We can't check IsWindow(m_viewWindow) here, because that will return true even while
+        // we're already handling WM_DESTROY. So we check !isBeingDestroyed() instead.
+        if (!isBeingDestroyed())
+            DestroyWindow(m_viewWindow);
+        // Either we just destroyed m_viewWindow, or it's in the process of being destroyed. Either
+        // way, we clear it out to make sure we don't try to use it later.
+        m_viewWindow = 0;
+    }
+
     setHostWindow(0);
 
@@ -1911 +1918 @@
             webView->setIsBeingDestroyed();
             webView->close();
-            webView->revokeDragDrop();
             break;
         case WM_GESTURENOTIFY:
@@ -2281 +2287 @@
     ASSERT(!m_deletionHasBegun);
 
+    if (m_refCount == 1) {
+        // Call close() now so that clients don't have to. (It's harmless to call close() multiple
+        // times.) We do this here instead of in our destructor because close() can cause AddRef()
+        // and Release() to be called, and if that happened in our destructor we would be destroyed
+        // more than once.
+        close();
+    }
+
     ULONG newRef = --m_refCount;
     if (!newRef) {
@@ -3061 +3075 @@
     m_hostWindow = window;
 
-    if (m_viewWindow)
-        windowAncestryDidChange();
+    windowAncestryDidChange();
 
     return S_OK;
@@ -4942 +4955 @@
 HRESULT WebView::revokeDragDrop()
 {
-    ASSERT(::IsWindow(m_viewWindow));
+    if (!m_viewWindow)
+        return S_OK;
+
     return ::RevokeDragDrop(m_viewWindow);
 }
@@ -5370 +5385 @@
 HRESULT STDMETHODCALLTYPE WebView::windowAncestryDidChange()
 {
-    HWND newParent = findTopLevelParent(m_hostWindow);
+    HWND newParent;
+    if (m_viewWindow)
+        newParent = findTopLevelParent(m_hostWindow);
+    else {
+        // There's no point in tracking active state changes of our parent window if we don't have
+        // a window ourselves.
+        newParent = 0;
+    }
+
     if (newParent == m_topLevelParent)
         return S_OK;