Detach all subframes before swapping a frame.

Detach all subframes before swapping a frame.

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

[nasko, 2014-09-04 17:42:22]

nasko@chromium.org changed reviewers:
+ dcheng@chromium.org, kenrb@chromium.org

[nasko, 2014-09-04 17:42:22]

Hey Ken,
Can you review this CL for me? We need to detach all subframes before swapping a
frame. In regular renderer-initiated navigation, the renderer detaches the
frames as part of destructing the document. When we have browser-initiated
navigation, we might not have destructed the document before swapping the
frames.
Thanks!
Nasko

[dcheng, 2014-09-04 18:38:57]

I took a glance, and it looks reasonable. It's been awhile since I've thought
through the implications of making this change
(https://codereview.chromium.org/419863004/ has some context) but this seems
like something we'll need.

[kenrb, 2014-09-04 19:04:50]

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

https://codereview.chromium.org/544443002/diff/40001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:35: oldFrame->detachChildren();
I haven't spent time thinking about the right sequence of things here, and from
the link Daniel provided I see he has been thinking about it but hasn't gotten
to a firm conclusion, so I have some questions/comments.

For one, I suspect you need to RefPtr any WebCore objects you are referencing
here because detachChildren() will fire child frame unload handlers which have
arbitrary behavior.

My main question is where does the unload handler run for the old frame? If this
is a remote frame getting swapped for a local frame then the handler should run
in the other process, but what if it is a local frame getting swapped for a
remote frame? Child detachment is part of the unload process, which is usually
handled by FrameLoader, and I am not sure what the story is for keeping the
behavior consistent.

[dcheng, 2014-09-04 19:18:12]

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

https://codereview.chromium.org/544443002/diff/40001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:35: oldFrame->detachChildren();
On 2014/09/04 at 19:04:50, kenrb wrote:
> I haven't spent time thinking about the right sequence of things here, and
from the link Daniel provided I see he has been thinking about it but hasn't
gotten to a firm conclusion, so I have some questions/comments.
> 
> For one, I suspect you need to RefPtr any WebCore objects you are referencing
here because detachChildren() will fire child frame unload handlers which have
arbitrary behavior.

Ken makes some good points. This reminds me of why I didn't implement this as
part of the original change. Down below, we reach into the Frame and pull out
some of its pointers (like the owner, frame host, name, etc). So we need to
figure out if its possible for this frame to be blown away from underneath
itself in swap()--and what we should do in that case.

> 
> My main question is where does the unload handler run for the old frame? If
this is a remote frame getting swapped for a local frame then the handler should
run in the other process, but what if it is a local frame getting swapped for a
remote frame? Child detachment is part of the unload process, which is usually
handled by FrameLoader, and I am not sure what the story is for keeping the
behavior consistent.

Swap is part of frame tree mirroring, so the idea is any renderers that need to
be updated will get a swap() as appropriate and do the "right thing" for
detaching that subtree.

We already break the abstraction for swapping out render views; there's an
escape hatch in WebFrame to trigger FrameLoader's detach. Since we're moving the
detach functionality up into Frame, so it works for both local and remote
frames, it makes sense that we would start using Frame to break this
abstraction.

[nasko, 2014-09-04 23:46:28]

Addressed Ken's and Daniel's comments.

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

https://codereview.chromium.org/544443002/diff/40001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:35: oldFrame->detachChildren();
On 2014/09/04 19:18:11, dcheng (OOO) wrote:
> On 2014/09/04 at 19:04:50, kenrb wrote:
> > I haven't spent time thinking about the right sequence of things here, and
> from the link Daniel provided I see he has been thinking about it but hasn't
> gotten to a firm conclusion, so I have some questions/comments.
> > 
> > For one, I suspect you need to RefPtr any WebCore objects you are
referencing
> here because detachChildren() will fire child frame unload handlers which have
> arbitrary behavior.
> 
> Ken makes some good points. This reminds me of why I didn't implement this as
> part of the original change. Down below, we reach into the Frame and pull out
> some of its pointers (like the owner, frame host, name, etc). So we need to
> figure out if its possible for this frame to be blown away from underneath
> itself in swap()--and what we should do in that case.

I've implemented an idea based on our discussion. If the frame itself is
detached while detaching its children, the method returns false to the embedder.
The embedder can assume the client object no longer exists, so it should be
returning immediately without executing more code.

> > 
> > My main question is where does the unload handler run for the old frame? If
> this is a remote frame getting swapped for a local frame then the handler
should
> run in the other process, but what if it is a local frame getting swapped for
a
> remote frame? Child detachment is part of the unload process, which is usually
> handled by FrameLoader, and I am not sure what the story is for keeping the
> behavior consistent.
> 
> Swap is part of frame tree mirroring, so the idea is any renderers that need
to
> be updated will get a swap() as appropriate and do the "right thing" for
> detaching that subtree.
> 
> We already break the abstraction for swapping out render views; there's an
> escape hatch in WebFrame to trigger FrameLoader's detach. Since we're moving
the
> detach functionality up into Frame, so it works for both local and remote
> frames, it makes sense that we would start using Frame to break this
> abstraction.

Yes, the unload handler for the frame that is being swapped is already called by
the embedder. We should change that and hide this detail here, but it belongs to
a different CL.

[kenrb, 2014-09-05 19:28:09]

non-owner lgtm.

We need to spend some more time on the swap sequence but I don't see a reason to
hold up this change for it.

https://codereview.chromium.org/544443002/diff/60001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/544443002/diff/60001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:96: ASSERT(refCount() > 1);
I'm fine with this, but it should have a comment on it so if somebody has a
reason to remove it later, they at least can be made to think about what they
are doing.

[nasko, 2014-09-05 23:37:29]

nasko@chromium.org changed reviewers:
+ eseidel@chromium.org

[nasko, 2014-09-05 23:37:29]

Eric,
Can you review this CL? The goal is to ensure all child frames are detached
before swapping a frame.
Thanks in advance!
Nasko

https://codereview.chromium.org/544443002/diff/60001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/544443002/diff/60001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:96: ASSERT(refCount() > 1);
On 2014/09/05 19:28:09, kenrb wrote:
> I'm fine with this, but it should have a comment on it so if somebody has a
> reason to remove it later, they at least can be made to think about what they
> are doing.

Done.

[nasko, 2014-09-08 20:11:10]

On 2014/09/05 23:37:29, nasko wrote:
> Eric,
> Can you review this CL? The goal is to ensure all child frames are detached
> before swapping a frame.

Ping!

[eseidel, 2014-09-10 18:09:12]

dcheng is the right reviewer for this, but lgtm.

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

https://codereview.chromium.org/544443002/diff/100001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:36: // If the frame has been detached during detaching
its children, return
When does this happen?  Are there JS events which fire, etc?

[nasko, 2014-09-10 19:36:57]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-09-10 19:37:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/544443002/100001

[commit-bot: I haz the power, 2014-09-10 20:13:32]

Committed patchset #6 (id:100001) as 181764