Add a BeforeDestroyFrame notification method to WebViewDelegate

There are cases where a Frame may outlive its associated Page.  Get the WebViewImpl by accessing it indirectly through the Frame's Page so that we don't have to worry about cleaning up the WebFrameImpl -> WebViewImpl pointer. WebCore already clears the Frame's Page pointer when the Page is destroyed by the WebViewImpl.

[Marshall Greenblatt, 2009-05-01 17:41:05]

Please review this patch.

[Aaron Boodman, 2009-05-01 18:34:25]

On 2009/05/01 17:41:05, Marshall Greenblatt wrote:
> Please review this patch.

LGTM, but waiting for Darin's approval.

[darin (slow to review), 2009-05-04 17:03:39]

http://codereview.chromium.org/99283/diff/5/1005
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/99283/diff/5/1005#newcode192
Line 192: virtual void BeforeDestroyFrame(WebFrame* webframe) {
I think this would be better named WillDestroyFrame.

[Marshall Greenblatt, 2009-05-04 17:11:34]

Changed BeforeDestroyFrame() to WillDestroyFrame().  Please review.

[darin (slow to review), 2009-05-04 17:48:12]

LGTM

[darin (slow to review), 2009-05-04 17:53:34]

Committed as:
http://src.chromium.org/viewvc/chrome?view=rev&revision=15223

[darin (slow to review), 2009-05-04 18:50:30]

Reverted as r15230 since it looks like this might explain the new CRASH of
http/tests/security/aboutBlank/security-context-with-base-tag.html

Perhaps the WebViewImpl pointer is null or junk when frameLoaderDestroyed is
called?

[Marshall Greenblatt, 2009-05-04 19:05:28]

On 2009/05/04 18:50:30, darin wrote:
> Reverted as r15230 since it looks like this might explain the new CRASH of
> http/tests/security/aboutBlank/security-context-with-base-tag.html
> 
> Perhaps the WebViewImpl pointer is null or junk when frameLoaderDestroyed is
> called?

I didn't experience any crashes in my own testing.  Is there something unique
about the way that security-context-with-base-tag.html works with frames?  In
any case, let's wait and see if this reversion fixes the crash before we spend
too much time trying to debug something that may not be broken :-).

[Marshall Greenblatt, 2009-05-05 17:05:28]

The crash was due to the WebViewImpl object being destroyed before the attached
WebFrameImpl objects in release build only.  This left a garbage webview_impl
pointer in WebFrameImpl.  The updated patch iterates through all attached
WebFrameImpl objects in WebViewImpl::Close() setting the webview_impl pointer to
NULL.  Please review.

[darin (slow to review), 2009-05-05 17:19:57]

Question: This solution means that the WebViewDelegate will not get notified for
some frames.  Won't this cause you problems if you are trying to free memory
when the frame is destroyed?

Implementation suggestion:  Instead of iterating over the frames, it might be
better to create a reference counted wrapper object for a WebViewImpl.  That
wrapper object could just hold a pointer to a WebViewImpl.  Imagine that each
WebFrameImpl holds a reference to that object instead of a direct pointer to the
WebViewImpl.  Then, to clear all back pointers to a WebViewImpl, the WebViewImpl
can just null out the pointer held by the wrapper object.

[darin (slow to review), 2009-05-05 17:20:31]

Also, I wonder why it is possible for a WebViewImpl to die before its frames. 
Maybe that is a bug.

[Marshall Greenblatt, 2009-05-05 17:53:15]

On 2009/05/05 17:19:57, darin wrote:
> Question: This solution means that the WebViewDelegate will not get notified
for
> some frames.  Won't this cause you problems if you are trying to free memory
> when the frame is destroyed?

In CEF we associate the memory with both the browser window and the frame. 
Therefore, memory will always be cleaned up when the browser window is destroyed
(this is the case where the delegate would be NULL).  The new delegate
notification lets us be more efficient by also cleaning up memory when the
associated frame is destroyed.

Sending notifications after WebView/WebFrame destruction seems a bit tricky.  We
explicitly set the delegate to NULL in WebViewImpl::Close() to avoid re-entrant
notifications to the initiator of the close call -- might there be cases in the
future where a re-entrant WillDestroyFrame call could be problematic?

Your question leads to an interesting idea, though.  Perhaps we should have the
WebFrameImpl maintain a scoped_refptr to the WebViewDelegate directly instead of
retrieving it via the WebViewImpl.  Then we could send the notification no
matter what and not have to worry about WebViewImpl's lifespan.

> 
> Implementation suggestion:  Instead of iterating over the frames, it might be
> better to create a reference counted wrapper object for a WebViewImpl.  That
> wrapper object could just hold a pointer to a WebViewImpl.  Imagine that each
> WebFrameImpl holds a reference to that object instead of a direct pointer to
the
> WebViewImpl.  Then, to clear all back pointers to a WebViewImpl, the
WebViewImpl
> can just null out the pointer held by the wrapper object.

That's a nifty idea.  Do we do this somewhere in the source code currently?

[Marshall Greenblatt, 2009-05-05 17:58:57]

On 2009/05/05 17:53:15, Marshall Greenblatt wrote:
> On 2009/05/05 17:19:57, darin wrote:
> > Question: This solution means that the WebViewDelegate will not get notified
> for
> > some frames.  Won't this cause you problems if you are trying to free memory
> > when the frame is destroyed?
> 
> In CEF we associate the memory with both the browser window and the frame. 
> Therefore, memory will always be cleaned up when the browser window is
destroyed
> (this is the case where the delegate would be NULL).  The new delegate
> notification lets us be more efficient by also cleaning up memory when the
> associated frame is destroyed.

I should mention that frame destruction will occur when you navigate from a page
with more frames to a page with less frames.

[Marshall Greenblatt, 2009-05-05 18:16:02]

On 2009/05/05 17:20:31, darin wrote:
> Also, I wonder why it is possible for a WebViewImpl to die before its frames. 
> Maybe that is a bug.

I think it might actually be a timing issue (still a bug?). Below are the
interesting stack traces:

Debug WebFrameLoaderClient::frameLoaderDestroyed() call:

>	test_shell.exe!WebFrameLoaderClient::frameLoaderDestroyed()  Line 105	C++
 	test_shell.exe!WebCore::FrameLoader::~FrameLoader()  Line 311 + 0x15 bytes	C++
 	test_shell.exe!WebCore::Frame::~Frame()  Line 209 + 0xe5 bytes	C++
 	test_shell.exe!WebCore::Frame::`scalar deleting destructor'()  + 0x16
bytes	C++
 	test_shell.exe!WTF::RefCounted<WebCore::Frame>::deref()  Line 94 + 0x1c
bytes	C++
 	test_shell.exe!WTF::RefPtr<WebCore::Frame>::~RefPtr<WebCore::Frame>()  Line 50
+ 0x2d bytes	C++
 	test_shell.exe!WebCore::InspectorResource::~InspectorResource()  Line 65 +
0x67 bytes	C++
 	test_shell.exe!WebCore::InspectorResource::`scalar deleting destructor'()  +
0x16 bytes	C++
 	test_shell.exe!WTF::RefCounted<WebCore::InspectorResource>::deref()  Line 94 +
0x1c bytes	C++

	test_shell.exe!WTF::RefPtr<WebCore::InspectorResource>::~RefPtr<WebCore::InspectorResource>()
 Line 50 + 0x2d bytes	C++
 	test_shell.exe!WebCore::InspectorController::~InspectorController()  Line 216
+ 0x83 bytes	C++
 	test_shell.exe!WebCore::InspectorController::`scalar deleting destructor'()  +
0x16 bytes	C++
 	test_shell.exe!WTF::RefCounted<WebCore::InspectorController>::deref()  Line 94
+ 0x1c bytes	C++

	test_shell.exe!WTF::RefPtr<WebCore::InspectorController>::~RefPtr<WebCore::InspectorController>()
 Line 50 + 0x2d bytes	C++
 	test_shell.exe!WebCore::Page::~Page()  Line 175 + 0x79 bytes	C++
 	test_shell.exe!WebCore::Page::`scalar deleting destructor'()  + 0x16 bytes	C++
 	test_shell.exe!scoped_ptr<WebCore::Page>::reset(WebCore::Page * p=0x00000000) 
Line 81 + 0x1e bytes	C++
 	test_shell.exe!WebViewImpl::Close()  Line 930	C++
 	test_shell.exe!WebWidgetHost::~WebWidgetHost()  Line 206 + 0x16 bytes	C++
 	test_shell.exe!WebViewHost::~WebViewHost()  + 0x16 bytes	C++
 	test_shell.exe!WebViewHost::`scalar deleting destructor'()  + 0x16 bytes	C++
 	test_shell.exe!scoped_ptr<WebViewHost>::~scoped_ptr<WebViewHost>()  Line 72 +
0x1e bytes	C++
 	test_shell.exe!TestShell::~TestShell()  Line 147 + 0x42 bytes	C++
 	test_shell.exe!TestShell::`scalar deleting destructor'()  + 0x16 bytes	C++
 	test_shell.exe!TestShell::WndProc(HWND__ * hwnd=0x00010822, unsigned int
message=2, unsigned int wParam=0, long lParam=0)  Line 595 + 0x22 bytes	C++


Debug WebViewImpl::~WebViewImpl() call:

>	test_shell.exe!WebViewImpl::~WebViewImpl()  Line 394	C++
 	test_shell.exe!WebViewImpl::`scalar deleting destructor'()  + 0x16 bytes	C++
 	test_shell.exe!base::RefCounted<WebViewImpl>::Release()  Line 80 + 0x3b
bytes	C++
 	test_shell.exe!WebViewImpl::Close()  Line 931	C++
 	test_shell.exe!WebWidgetHost::~WebWidgetHost()  Line 206 + 0x16 bytes	C++
 	test_shell.exe!WebViewHost::~WebViewHost()  + 0x16 bytes	C++
 	test_shell.exe!WebViewHost::`scalar deleting destructor'()  + 0x16 bytes	C++
 	test_shell.exe!scoped_ptr<WebViewHost>::~scoped_ptr<WebViewHost>()  Line 72 +
0x1e bytes	C++
 	test_shell.exe!TestShell::~TestShell()  Line 147 + 0x42 bytes	C++
 	test_shell.exe!TestShell::`scalar deleting destructor'()  + 0x16 bytes	C++
 	test_shell.exe!TestShell::WndProc(HWND__ * hwnd=0x00060864, unsigned int
message=2, unsigned int wParam=0, long lParam=0)  Line 595 + 0x22 bytes	C++



Release WebViewImpl::~WebViewImpl() call:

>	test_shell.exe!WebViewImpl::~WebViewImpl()  Line 394	C++
 	test_shell.exe!WebViewImpl::`scalar deleting destructor'()  + 0xb bytes	C++
 	test_shell.exe!WebViewImpl::Close()  Line 930 + 0x22 bytes	C++
 	test_shell.exe!WebWidgetHost::~WebWidgetHost()  Line 207	C++
 	test_shell.exe!TestShell::~TestShell()  Line 147 + 0x6e bytes	C++
 	test_shell.exe!TestShell::`scalar deleting destructor'()  + 0xb bytes	C++
 	test_shell.exe!TestShell::WndProc(HWND__ * hwnd=0x0005089a, unsigned int
message=2, unsigned int wParam=0, long lParam=0)  Line 595 + 0xe bytes	C++



Release WebFrameLoaderClient::frameLoaderDestroyed() call:

>	test_shell.exe!WebFrameLoaderClient::frameLoaderDestroyed()  Line 93	C++
 	test_shell.exe!WebCore::FrameLoader::~FrameLoader()  Line 312	C++
 	test_shell.exe!WebCore::Frame::~Frame()  Line 209 + 0x14f bytes	C++

	test_shell.exe!WebCore::Frame::lifeSupportTimerFired(WebCore::Timer<WebCore::Frame>
* __formal=0x00837b14)  Line 1119 + 0x5 bytes	C++
 	test_shell.exe!WebCore::Timer<WebCore::AnimationControllerPrivate>::fired() 
Line 98 + 0xb bytes	C++
 	test_shell.exe!WebCore::ThreadTimers::fireTimers(double
fireTime=1241547039.5785339, const WTF::Vector<WebCore::TimerBase *,0> &
firingTimers=[2](...))  Line 114	C++
 	test_shell.exe!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 143	C++
 	test_shell.exe!MessageLoop::RunTask(Task * task=0x0171ec90)  Line 309	C++
 	test_shell.exe!MessageLoop::DoWork()  Line 424	C++
 	test_shell.exe!base::MessagePumpForUI::DoRunLoop()  Line 210	C++
 	test_shell.exe!base::MessagePumpWin::Run(base::MessagePump::Delegate *
delegate=0x0013f590)  Line 78 + 0x40 bytes	C++
 	test_shell.exe!MessageLoop::RunInternal()  Line 197 + 0x8 bytes	C++
 	test_shell.exe!MessageLoop::RunHandler()  Line 181	C++
 	test_shell.exe!MessageLoop::Run()  Line 155	C++
 	test_shell.exe!main(int argc=1, char * * argv=0x01624570)  Line 318	C++
 	test_shell.exe!__tmainCRTStartup()  Line 327 + 0x12 bytes	C

[darin (slow to review), 2009-05-05 18:19:30]

OK, yeah... I think there are definitely cases where a Frame may outlive its
associated Page.  Perhaps we should switch to getting the WebViewImpl by
accessing it indirectly through the Frame's Page's Chrome's ChromeClient.  If
you follow that path, then we don't have to worry about cleaning up the
WebFrameImpl -> WebViewImpl pointer since WebCore already clears the Frame's
Page pointer when the Page is destroyed.  The Page is destroyed by the
WebViewImpl.

[Marshall Greenblatt, 2009-05-05 21:06:13]

I've made the change that Darin suggested to retrieve the WebViewImpl pointer
via the Page.  Unfortunately for the purposes of this patch the WebCore::Frame
object is destroyed before WebFrameLoaderClient::frameLoaderDestroyed() is
called, and so the WebViewImpl pointer is already gone.  Therefore I've also
modified WebFrameImpl to maintain a scoped_refptr to the WebViewDelegate
directly instead of retrieving it via the WebViewImpl.  The WebViewDelegate
scoped_refptr held by the WebFrameImpl is released/reset in
WebViewImpl::SetDelegate() at the same time that it's released/reset for the
WebViewImpl.  Please review.

[darin (slow to review), 2009-05-05 21:37:38]

I think trying to keep the WebViewDelegate alive past the lifetime of the
WebView is likely to cause problems.  I would rather just not support the
notifications after WebView destruction if they are not needed.  That would help
keep this patch simpler.

Unfortunately, I think the change to make webview_impl() non-trivial, means that
it should be renamed to something like GetWebViewImpl().  I'm sorry for making
more work for you! :(

[Marshall Greenblatt, 2009-05-06 18:51:59]

The patch has been updated for just the WebFrameImpl -> WebViewImpl pointer
changes.

In WebFrameLoaderClient we check that the WebFrameImpl is non-NULL in some
places but not in others.  There's even a comment to this effect in the code:

// TODO(ericroman): why might webframe_->webview_impl be null?
// http://b/1234461

Since we're revisiting this code it may be a good time for us to clarify when
the check is necessary and when it isn't.  Or perhaps we should just check that
it's non-NULL everywhere?  Please review.

[darin (slow to review), 2009-05-06 20:31:09]

LGTM

I agree that it would be nice to be consistent with the null checking.  But, I
think we should go with this patch as-is for now.  Shortly, I am going to be
splitting WebViewDelegate into WebViewClient and WebFrameClient, so many of
these calls to GetWebViewImpl will simply go away.

[Marshall Greenblatt, 2009-05-06 20:56:18]

On 2009/05/06 20:31:09, darin wrote:
> LGTM
> 
> I agree that it would be nice to be consistent with the null checking.  But, I
> think we should go with this patch as-is for now.  Shortly, I am going to be
> splitting WebViewDelegate into WebViewClient and WebFrameClient, so many of
> these calls to GetWebViewImpl will simply go away.

Sounds good :-)

[darin (slow to review), 2009-05-06 21:08:47]

Committed as r15458