Bug 32827 - Crash when calling IWebView::close, then releasing the WebView, without calling DestroyWindow
: Crash when calling IWebView::close, then releasing the WebView, without calli...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit API
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To: Nobody
: InRadar, PlatformOnly
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-21 10:15 PST by Adam Roben (:aroben)
Modified: 2010-01-05 15:10 PST (History)
2 users (show)

See Also:


Attachments
Patch (4.38 KB, patch)
2009-12-21 10:15 PST, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
Make it safe to call IWebView::close when IWebView::initWithFrame hasn't been called (4.44 KB, patch)
2010-01-05 11:04 PST, Adam Roben (:aroben)
sfalken: review+
Details | Formatted Diff | Diff
Make IWebView::close and destroying a WebView's HWND optional for WebKit clients (5.97 KB, patch)
2010-01-05 12:02 PST, Adam Roben (:aroben)
sfalken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2009-12-21 10:15:36 PST
Created attachment 45337 [details]
Patch

WebKit clients can currently cleanly get rid of a WebView in two ways.
A)
1) DestroyWindow(webViewHWND)
2) webView->Release()
B)
1) webView->close()
2) DestroyWindow(webViewHWND) (can be swapped with (1))
3) webView->Release()

We'd like clients to be able to get rid of a WebView just by releasing
the last reference to it. This patch gets us a little closer to that
by removing step B2 above (though calling DestroyWindow in this case
is harmless). A future patch will make steps A1 and B1 unnecessary, as
well.

Fixes <rdar://problem/7374218> Crash in WebView::updateActiveState
when closing "Welcome to iTunes" window

Reviewed by NOBODY (OOPS!).

* WebView.cpp:
(WebView::~WebView): Call setIsBeingDestroyed() so that we won't be
ref'd by our WndProc, which would result in this destructor being
re-entered.
(WebView::close): Moved the call to revokeDragDrop here...
(WebView::WebViewWndProc): ...from here. This is important in order to
release the reference that OLE holds while we're registered as a drop
target. Otherwise, clients that call IWebView::close but not
DestroyWindow would leak the WebView.

* WebView.h:
(WebView::setIsBeingDestroyed):
(WebView::isBeingDestroyed):
Made these private, and added a comment about what isBeingDestroyed()
now means.
---
 3 files changed, 52 insertions(+), 5 deletions(-)
Comment 1 Adam Roben (:aroben) 2009-12-21 10:17:34 PST
<rdar://problem/7374218>
Comment 2 WebKit Review Bot 2009-12-21 10:19:06 PST
style-queue ran check-webkit-style on attachment 45337 [details] without any errors.
Comment 3 Adam Roben (:aroben) 2009-12-22 11:27:20 PST
Comment on attachment 45337 [details]
Patch

I'm going to try tackling this a different way.
Comment 4 Adam Roben (:aroben) 2010-01-05 11:04:57 PST
Created attachment 45912 [details]
Make it safe to call IWebView::close when IWebView::initWithFrame hasn't been called
Comment 5 WebKit Review Bot 2010-01-05 11:10:02 PST
style-queue ran check-webkit-style on attachment 45912 [details] without any errors.
Comment 6 Adam Roben (:aroben) 2010-01-05 12:02:30 PST
Created attachment 45916 [details]
Make IWebView::close and destroying a WebView's HWND optional for WebKit clients
Comment 7 Eric Seidel 2010-01-05 13:56:52 PST
Comment on attachment 45912 [details]
Make it safe to call IWebView::close when IWebView::initWithFrame hasn't been called

Looks sane to me too.
Comment 8 Adam Roben (:aroben) 2010-01-05 15:10:08 PST
Committed r52829: <http://trac.webkit.org/changeset/52829>
Comment 9 Adam Roben (:aroben) 2010-01-05 15:10:31 PST
Committed r52830: <http://trac.webkit.org/changeset/52830>