Invert the owning relationship between WebFrame and Frame.

Invert the owning relationship between WebFrame and Frame.

TODOs:
- See if there's a cleaner way to handle find in page cancellation other than plumbing it through FrameLoaderClient...
- Fix unload dispatch since V8WindowShell has a if clause depend on FrameLoaderClient not being null.

BUG=329266
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164390

[dcheng, 2013-12-17 23:39:13]

I think this patch is ready for an initial review. The primary issue I see is
that we never call FrameLoaderClient::willReleaseScriptContext, which is
problematic because it looks like the extension dispatcher expects this callback
to trigger unload events (and probably other things). 

I'm currently working on adding an escape hatch to allow this to be (indirectly)
called by FrameLoader::clear(). The alternative was to allow Frame::script() to
return NULL and have Frame::willDetachPage() clear it to trigger
V8WindowShell::clearForClose(), but that seems far messier.

[abarth-chromium, 2013-12-19 07:54:24]

Sorry, too tired to review this one.  Will need to look again tomorrow.

[eseidel, 2013-12-21 01:17:34]

Looks OK if it works.  I would like this to be a bit more conceptually simple
eventually.  WebFrame simply owns a Ref to frame.  After
WebFrame::detatchedFromParent is called, Frame should die very shortly after.

In any case, looks fine.  Looks like you mostly had to teach FrameLoader that it
could outlive its client.

https://codereview.chromium.org/117493002/diff/100001/Source/bindings/v8/V8Wi...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/117493002/diff/100001/Source/bindings/v8/V8Wi...
Source/bindings/v8/V8WindowShell.cpp:101: // FIXME: This is probably wrong.
A bit more context?

https://codereview.chromium.org/117493002/diff/100001/Source/web/WebFrameImpl.h
File Source/web/WebFrameImpl.h (right):

https://codereview.chromium.org/117493002/diff/100001/Source/web/WebFrameImpl...
Source/web/WebFrameImpl.h:439: // The cycle of madness.
Something a bit more informative. :)

[dcheng, 2013-12-28 01:04:29]

PTAL.

https://codereview.chromium.org/117493002/diff/100001/Source/bindings/v8/V8Wi...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/117493002/diff/100001/Source/bindings/v8/V8Wi...
Source/bindings/v8/V8WindowShell.cpp:101: // FIXME: This is probably wrong.
On 2013/12/21 01:17:34, eseidel wrote:
> A bit more context?

I've found a better way to fix this. Normally, this gets invoked during frame
detach by FrameLoaderClientImpl. The problem is that certain things like
SVGImage have a main frame, but just use EmptyFrameLoaderClient. To fix this, I
moved the call to ScriptController::clearForClose() from FrameLoaderClientImpl
into FrameLoader and added an assert that it was called before ScriptController
is destroyed. I also had to update the inspector overlay to tear down its Page
the same way as well.

https://codereview.chromium.org/117493002/diff/100001/Source/web/WebFrameImpl.h
File Source/web/WebFrameImpl.h (right):

https://codereview.chromium.org/117493002/diff/100001/Source/web/WebFrameImpl...
Source/web/WebFrameImpl.h:439: // The cycle of madness.
On 2013/12/21 01:17:34, eseidel wrote:
> Something a bit more informative. :)

Oops. This was a leftover comment from my initial patch =)
I've updated it to document when it's expected to be set and when it's expected
to be null. The .cpp implementation covers the interaction with the embedder in
more detail.

[eseidel, 2013-12-28 01:18:40]

This lgtm.  But would be nice to see some responses to my questions below before
landing.

https://codereview.chromium.org/117493002/diff/190001/Source/core/inspector/I...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/core/inspector/I...
Source/core/inspector/InspectorOverlay.cpp:685:
m_overlayPage->mainFrame()->loader().frameDetached();
Is it too late to do this during Page destruction?  We should add a comment as
to why this explicit call is necessary.

https://codereview.chromium.org/117493002/diff/190001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1147: // Close down the proxy. The purpose of
this change is to make the call to
I think you mean "call" instead of change.

https://codereview.chromium.org/117493002/diff/190001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1149: // Without this change, this call to
clearWindowShell will cause a crash. If you
clearWindowShell is called by clearForClose? or did you mean clearForClose?

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (left):

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:266: // Alert the client that the frame is
being detached. This is the last
Is this still the last chance? or are we completely done already?  I would think
the willDetach method would be the last chance and this would be after our
chance is over?

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:245: void
FrameLoaderClientImpl::willDetachParent()
What's "parent" in this case?  The page?  The parent frame?  Is this called for
the main frame?

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:261: // frameDetached() may release the
final reference to WebFrame. Since we are
I'm confused by this comment.  The goal is to clear our m_webFrame pointer
before we tell WebFrameClient that we're detached just in case it calls back to
us and tries to talk to a dead m_webFrame?

https://codereview.chromium.org/117493002/diff/190001/Source/web/WebFrameImpl...
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/web/WebFrameImpl...
Source/web/WebFrameImpl.cpp:61: // in FrameLoader::detachFromParent for each
subframe. detachFromParent()
Might be nice to know the call order of detachFromParent.  Presumbly its from
the subtree-root up? i.e. recursive?

https://codereview.chromium.org/117493002/diff/190001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/117493002/diff/190001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:3194: RefPtr<WebCore::Frame> holdSecondFrame =
secondFrame->frame();
Doesn't secondFrame above hold this reference?

[dcheng, 2013-12-28 01:51:04]

https://codereview.chromium.org/117493002/diff/190001/Source/core/inspector/I...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/core/inspector/I...
Source/core/inspector/InspectorOverlay.cpp:685:
m_overlayPage->mainFrame()->loader().frameDetached();
On 2013/12/28 01:18:40, eseidel wrote:
> Is it too late to do this during Page destruction?  We should add a comment as
> to why this explicit call is necessary.

One possible alternative is to move this into Page's destructor. I didn't do
this and chose to just mirror the existing structure of SVGImage/WebViewImpl.

Since it seems kind of silly to put a comment in all three locations (and the
comment would reference code in seemingly unrelated bits of Blink), I've added a
FIXME to investigate moving this common logic into Page.

https://codereview.chromium.org/117493002/diff/190001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1147: // Close down the proxy. The purpose of
this change is to make the call to
On 2013/12/28 01:18:40, eseidel wrote:
> I think you mean "call" instead of change.

I don't have any clue what the original author meant (this was moved verbatim
from FrameLoaderClientImpl).

https://codereview.chromium.org/117493002/diff/190001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1149: // Without this change, this call to
clearWindowShell will cause a crash. If you
On 2013/12/28 01:18:40, eseidel wrote:
> clearWindowShell is called by clearForClose? or did you mean clearForClose?

This comment seems to have suffered from significant bitrot. For example, there
is no such thing as Frame::pageDestroyed() anymore. I've rewritten it so
hopefully it makes sense now.

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (left):

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:266: // Alert the client that the frame is
being detached. This is the last
On 2013/12/28 01:18:40, eseidel wrote:
> Is this still the last chance? or are we completely done already?  I would
think
> the willDetach method would be the last chance and this would be after our
> chance is over?

This is the last chance because the final line of this function is what
(typically) triggers the embedder to release its reference to the WebFrame (and
consequently, this object).

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:245: void
FrameLoaderClientImpl::willDetachParent()
On 2013/12/28 01:18:40, eseidel wrote:
> What's "parent" in this case?  The page?  The parent frame?  Is this called
for
> the main frame?

I can't think of a better name for this unfortunately. It's called for any
frame, regardless of whether or not its the main frame. This mirrors the naming
convention of FrameLoader::detachFromParent... perhaps a followup patch to think
of more apt names is in order.

https://codereview.chromium.org/117493002/diff/190001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:261: // frameDetached() may release the
final reference to WebFrame. Since we are
On 2013/12/28 01:18:40, eseidel wrote:
> I'm confused by this comment.  The goal is to clear our m_webFrame pointer
> before we tell WebFrameClient that we're detached just in case it calls back
to
> us and tries to talk to a dead m_webFrame?

I've tried to clarify this comment. Basically, calling
WebFrameClient::frameDetached() typically results in destruction of this.

https://codereview.chromium.org/117493002/diff/190001/Source/web/WebFrameImpl...
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/117493002/diff/190001/Source/web/WebFrameImpl...
Source/web/WebFrameImpl.cpp:61: // in FrameLoader::detachFromParent for each
subframe. detachFromParent()
On 2013/12/28 01:18:40, eseidel wrote:
> Might be nice to know the call order of detachFromParent.  Presumbly its from
> the subtree-root up? i.e. recursive?

Done.

https://codereview.chromium.org/117493002/diff/190001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/117493002/diff/190001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:3194: RefPtr<WebCore::Frame> holdSecondFrame =
secondFrame->frame();
On 2013/12/28 01:18:40, eseidel wrote:
> Doesn't secondFrame above hold this reference?

Logically, the embedder holds a reference to it, but we only want to expose
create()/close() semantics to the embedder (this was suggested by darin@, to
model the way WebView works).

The embedder releases this reference by calling close() when Blink tells it
to--e.g. the page is being navigated away from, or the frame was removed from
the DOM. At that point, the embedder releases its reference by calling close().
Typically, this will destroy the WebFrame since there will be no remaining
references.

In this somewhat artificial test, we have to force the WebFrame to remain alive
after line 3197 by manually retaining a reference to it.

[commit-bot: I haz the power, 2013-12-30 19:29:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/117493002/310001

[commit-bot: I haz the power, 2013-12-30 22:18:27]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-12-30 23:58:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/117493002/490001

[commit-bot: I haz the power, 2013-12-31 01:24:24]

Change committed as 164390