Detach old frame on WebFrame::swap.

Detach old frame on WebFrame::swap.

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

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

[dcheng, 2015-03-27 04:08:48]

Very cool! Some initial thoughts.

https://codereview.chromium.org/1041473002/diff/1/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/1/Source/web/WebFrame.cpp#new...
Source/web/WebFrame.cpp:40: if (isWebLocalFrame())
I think you can get the unload() behavior you want for free by doing something
like:

dispatchUnloadEvent();  // Only dispatches unload event for this subframe.
oldFrame->detachChildren();  // Now any remaining children are detached.

// If the unload event itself or the children already detached this frame, then
just
// return. There is no longer a place to swap this frame into the frame tree.
if (!oldFrame->host())
  return;

/* cache state */
oldFrame->detach();  // The children of the original frame are already gone, but
detach oldFrame itself now.

https://codereview.chromium.org/1041473002/diff/1/Source/web/WebFrame.cpp#new...
Source/web/WebFrame.cpp:65: // Object is invalid beyond this point.
I don't think this comment is completely accurate--after all, we call
finishSwapFrom(oldFrame.get()) below.

[lfg, 2015-03-27 04:57:53]

https://codereview.chromium.org/1041473002/diff/1/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/1/Source/web/WebFrame.cpp#new...
Source/web/WebFrame.cpp:40: if (isWebLocalFrame())
On 2015/03/27 04:08:47, dcheng wrote:
> I think you can get the unload() behavior you want for free by doing something
> like:
> 
> dispatchUnloadEvent();  // Only dispatches unload event for this subframe.
> oldFrame->detachChildren();  // Now any remaining children are detached.
> 
> // If the unload event itself or the children already detached this frame,
then
> just
> // return. There is no longer a place to swap this frame into the frame tree.
> if (!oldFrame->host())
>   return;
> 
> /* cache state */
> oldFrame->detach();  // The children of the original frame are already gone,
but
> detach oldFrame itself now.

Interesting, I'll try that.

https://codereview.chromium.org/1041473002/diff/1/Source/web/WebFrame.cpp#new...
Source/web/WebFrame.cpp:65: // Object is invalid beyond this point.
On 2015/03/27 04:08:47, dcheng wrote:
> I don't think this comment is completely accurate--after all, we call
> finishSwapFrom(oldFrame.get()) below.

The WebFrame is gone (i.e. the object pointed by the 'this' pointer), but the
Frame is still alive because of the refcounted reference we took at the
beginning of this function.

[lfg, 2015-06-03 17:54:11]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org

[lfg, 2015-06-03 17:54:11]

PTAL, thanks!

[dcheng, 2015-06-04 00:58:23]

https://codereview.chromium.org/1041473002/diff/160001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/core/frame/Fram...
Source/core/frame/Frame.h:56: enum class DetachType { Remove, Swap };
Let's call this FrameDetachType if we're going to make it a top-level type.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/FrameLoader...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/web/FrameLoader...
Source/web/FrameLoaderClientImpl.cpp:334: client->frameDetached(m_webFrame,
static_cast<WebFrameClient::DetachType>(type));
There should probably be something in AssertMatchingEnums.cpp for this.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/RemoteFrame...
File Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/web/RemoteFrame...
Source/web/RemoteFrameClientImpl.cpp:47:
client->frameDetached(static_cast<WebRemoteFrameClient::DetachType>(type));
There should probably be something in AssertMatchingEnums.cpp for this as well.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:52: // If the frame has been detached during detaching
its children, return
Update this comment to reflect that we want to check if any JS was executed that
should cause this swap to be cancelled, since it's not just child unload events
anymore.

Also, I have bad news =)

We also need to trigger XHR aborts here... which is done today by calling
FrameLoader::m_documentLoader->detachFromFrame().

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:83: std::swap(m_previousSibling,
frame->m_previousSibling);
No std:: since swap() is special.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:87: std::swap(m_nextSibling, frame->m_nextSibling);
Ditto.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:93: std::swap(m_opener, frame->m_opener);
Ditto.

[lfg, 2015-06-04 19:16:59]

Addressed some comments and added a couple of questions, please take another
look.

Thanks!

https://codereview.chromium.org/1041473002/diff/160001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/core/frame/Fram...
Source/core/frame/Frame.h:56: enum class DetachType { Remove, Swap };
On 2015/06/04 00:58:23, dcheng wrote:
> Let's call this FrameDetachType if we're going to make it a top-level type.

Done.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/FrameLoader...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/web/FrameLoader...
Source/web/FrameLoaderClientImpl.cpp:334: client->frameDetached(m_webFrame,
static_cast<WebFrameClient::DetachType>(type));
On 2015/06/04 00:58:23, dcheng wrote:
> There should probably be something in AssertMatchingEnums.cpp for this.

Done.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/RemoteFrame...
File Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/web/RemoteFrame...
Source/web/RemoteFrameClientImpl.cpp:47:
client->frameDetached(static_cast<WebRemoteFrameClient::DetachType>(type));
On 2015/06/04 00:58:23, dcheng wrote:
> There should probably be something in AssertMatchingEnums.cpp for this as
well.

Done.

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:52: // If the frame has been detached during detaching
its children, return
On 2015/06/04 00:58:23, dcheng wrote:
> Update this comment to reflect that we want to check if any JS was executed
that
> should cause this swap to be cancelled, since it's not just child unload
events
> anymore.

Updated.

> Also, I have bad news =)
> 
> We also need to trigger XHR aborts here... which is done today by calling
> FrameLoader::m_documentLoader->detachFromFrame().

Are you sure we need this? Where should it be?

We already end up calling it, because LocalFrame::detach() calls
FrameLoader::detach() which calls DocumentLoader::detachFromFrame().

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:83: std::swap(m_previousSibling,
frame->m_previousSibling);
On 2015/06/04 00:58:23, dcheng wrote:
> No std:: since swap() is special.

Not sure I understand this, do you mean don't use std::swap, or add the "using
std::swap" at the beginning of the function?

[dcheng, 2015-06-05 02:11:50]

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:52: // If the frame has been detached during detaching
its children, return
On 2015/06/04 at 19:16:58, lfg wrote:
> On 2015/06/04 00:58:23, dcheng wrote:
> > Update this comment to reflect that we want to check if any JS was executed
that
> > should cause this swap to be cancelled, since it's not just child unload
events
> > anymore.
> 
> Updated.
> 
> > Also, I have bad news =)
> > 
> > We also need to trigger XHR aborts here... which is done today by calling
> > FrameLoader::m_documentLoader->detachFromFrame().
> 
> Are you sure we need this? Where should it be?
> 
> We already end up calling it, because LocalFrame::detach() calls
FrameLoader::detach() which calls DocumentLoader::detachFromFrame().

We need to call it independently, because it can also cause the frame to be
detached: in that case, we need to cancel the swap().

https://codereview.chromium.org/1041473002/diff/160001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:83: std::swap(m_previousSibling,
frame->m_previousSibling);
On 2015/06/04 at 19:16:58, lfg wrote:
> On 2015/06/04 00:58:23, dcheng wrote:
> > No std:: since swap() is special.
> 
> Not sure I understand this, do you mean don't use std::swap, or add the "using
std::swap" at the beginning of the function?

Yes, restore the using std::swap and then remove the std:: qualifiers here. This
is the standard idiom for using swap().

[lfg, 2015-06-05 19:39:26]

On 2015/06/05 02:11:50, dcheng wrote:
> > > Also, I have bad news =)
> > > 
> > > We also need to trigger XHR aborts here... which is done today by calling
> > > FrameLoader::m_documentLoader->detachFromFrame().
> > 
> > Are you sure we need this? Where should it be?
> > 
> > We already end up calling it, because LocalFrame::detach() calls
> FrameLoader::detach() which calls DocumentLoader::detachFromFrame().
> 
> We need to call it independently, because it can also cause the frame to be
> detached: in that case, we need to cancel the swap().

Done.

[lfg, 2015-06-05 19:48:57]

On 2015/06/05 19:39:26, lfg wrote:
> On 2015/06/05 02:11:50, dcheng wrote:
> > > > Also, I have bad news =)
> > > > 
> > > > We also need to trigger XHR aborts here... which is done today by
calling
> > > > FrameLoader::m_documentLoader->detachFromFrame().
> > > 
> > > Are you sure we need this? Where should it be?
> > > 
> > > We already end up calling it, because LocalFrame::detach() calls
> > FrameLoader::detach() which calls DocumentLoader::detachFromFrame().
> > 
> > We need to call it independently, because it can also cause the frame to be
> > detached: in that case, we need to cancel the swap().
> 
> Done.

Wait, there's an issue here, the FrameLoader is not guaranteed to exist anymore
at that point. Let me think about this.

[lfg, 2015-06-05 21:50:39]

Please take a look.

I've added a prepareForCommit() function in FrameLoader to handle this, and I'm
calling it both from WebFrame::swap() as well as
FrameLoader::commitProvisionalLoad().

[dcheng, 2015-06-08 19:34:43]

https://codereview.chromium.org/1041473002/diff/210001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1041473002/diff/210001/Source/core/frame/Fram...
Source/core/frame/Frame.h:56: enum class FrameDetachType { Remove,
This is wrapped weird. Just put it all on one line?

https://codereview.chromium.org/1041473002/diff/210001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1041473002/diff/210001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:986: m_documentLoader =
m_provisionalDocumentLoader.release();
Is it actually safe to assume that m_provisionalDocumentLoader will always be
null here in the detach case? Or we do just say that it doesn't matter, because
we're about to really detach the frame shortly afterwards anyway?

https://codereview.chromium.org/1041473002/diff/210001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/210001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:52: oldFrame->detachChildren();
If we're swapping remote to local, and detaching the children causes the remote
frame to be detached, we also need to bail out.

https://codereview.chromium.org/1041473002/diff/210001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:58: if (!readyToSwap)
I think you can keep the original check here, since that will handle both the
local and remote cases.

[lfg, 2015-06-10 18:41:28]

https://codereview.chromium.org/1041473002/diff/210001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1041473002/diff/210001/Source/core/frame/Fram...
Source/core/frame/Frame.h:56: enum class FrameDetachType { Remove,
On 2015/06/08 19:34:42, dcheng wrote:
> This is wrapped weird. Just put it all on one line?

git cl format did this :(. Fixed.

https://codereview.chromium.org/1041473002/diff/210001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1041473002/diff/210001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:986: m_documentLoader =
m_provisionalDocumentLoader.release();
On 2015/06/08 19:34:42, dcheng wrote:
> Is it actually safe to assume that m_provisionalDocumentLoader will always be
> null here in the detach case? Or we do just say that it doesn't matter,
because
> we're about to really detach the frame shortly afterwards anyway?

It doesn't matter, since the oldFrame is being detached no matter what when we
get here.

https://codereview.chromium.org/1041473002/diff/210001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/210001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:52: oldFrame->detachChildren();
On 2015/06/08 19:34:42, dcheng wrote:
> If we're swapping remote to local, and detaching the children causes the
remote
> frame to be detached, we also need to bail out.

At first I thought this would never be the case, but it can happen if you have
an A -> B -> A situation.

https://codereview.chromium.org/1041473002/diff/210001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:58: if (!readyToSwap)
On 2015/06/08 19:34:42, dcheng wrote:
> I think you can keep the original check here, since that will handle both the
> local and remote cases.

Done.

[dcheng, 2015-06-10 19:56:30]

Looking good. Just a few more comments, and I think we can land this.

https://codereview.chromium.org/1041473002/diff/230001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1041473002/diff/230001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:1041: RefPtr<DocumentLoader> pdl =
m_provisionalDocumentLoader;
Do we still need this protector? JS events don't run before lines 1045-1048.

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:15: #include "core/loader/DocumentLoader.h"
Nit: remove this since it's no longer needed here.

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:49:
toLocalFrame(toCoreFrame(this))->loader().prepareForCommit();
Let's use virtual dispatch on Frame instead. We can add a new virtual to Frame,
that does the right thing depending on whether it's Local or Remote.

[dcheng, 2015-06-10 19:56:42]

Also, CC japhet@

[lfg, 2015-06-10 22:43:35]

https://codereview.chromium.org/1041473002/diff/230001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1041473002/diff/230001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:1041: RefPtr<DocumentLoader> pdl =
m_provisionalDocumentLoader;
On 2015/06/10 19:56:29, dcheng wrote:
> Do we still need this protector? JS events don't run before lines 1045-1048.

No, we don't need it. Removed.

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:15: #include "core/loader/DocumentLoader.h"
On 2015/06/10 19:56:29, dcheng wrote:
> Nit: remove this since it's no longer needed here.

Done.

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:49:
toLocalFrame(toCoreFrame(this))->loader().prepareForCommit();
On 2015/06/10 19:56:29, dcheng wrote:
> Let's use virtual dispatch on Frame instead. We can add a new virtual to
Frame,
> that does the right thing depending on whether it's Local or Remote.

I've moved to a virtual dispatch, but I'm hoping you can come up with a better
name for the function.

[dcheng, 2015-06-11 18:45:02]

lgtm

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1041473002/diff/230001/Source/web/WebFrame.cp...
Source/web/WebFrame.cpp:49:
toLocalFrame(toCoreFrame(this))->loader().prepareForCommit();
On 2015/06/10 at 22:43:35, lfg wrote:
> On 2015/06/10 19:56:29, dcheng wrote:
> > Let's use virtual dispatch on Frame instead. We can add a new virtual to
Frame,
> > that does the right thing depending on whether it's Local or Remote.
> 
> I've moved to a virtual dispatch, but I'm hoping you can come up with a better
name for the function.

Meh. It's good enough =)

[lfg, 2015-06-11 19:55:59]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-06-11 19:56:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1041473002/270001

[commit-bot: I haz the power, 2015-06-11 20:00:31]

Committed patchset #15 (id:270001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196968

[dglazkov, 2015-06-11 21:25:37]

A revert of this CL (patchset #15 id:270001) has been created in
https://codereview.chromium.org/1177333002/ by dglazkov@chromium.org.

The reason for reverting is: Breaks html_viewer/mandoline stuff:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_....

[dcheng, 2015-06-11 21:41:00]

I'm not sure why, but part of the original change to prepare Chrome for this
Blink patch was silently reverted:
https://codereview.chromium.org/1164963002/

[dcheng, 2015-06-12 04:21:10]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-06-12 04:21:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1041473002/270001

[commit-bot: I haz the power, 2015-06-12 05:36:17]

Committed patchset #15 (id:270001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197000