RemoteBridgeFrameOwner: keep better track of the attached frame

RemoteBridgeFrameOwner: keep better track of the attached frame

This CL eliminates some of the branches and simplifies the bookkeeping,
since it needs to be done whether the FrameOwner is local or remote.
Doing this bookkeeping is important, since it prevents a remote frame
owner from retaining a WebLocalFrameImpl and keeping it live after it's
already swapped out.

BUG=none

Committed: https://crrev.com/2a9d38d2a9724e5fc907ebcb239fc39f0929add6
Cr-Commit-Position: refs/heads/master@{#381508}

[dcheng, 2016-03-16 09:15:26]

Description was changed from

==========
The cleanup will continue until morale improves.

BUG=
==========

to

==========
RemoteBridgeFrameOwner: keep better track of the attached frame

BUG=none
==========

[dcheng, 2016-03-16 09:15:27]

dcheng@chromium.org changed reviewers:
+ sigbjornf@opera.com

[dcheng, 2016-03-16 09:19:04]

Description was changed from

==========
RemoteBridgeFrameOwner: keep better track of the attached frame

BUG=none
==========

to

==========
RemoteBridgeFrameOwner: keep better track of the attached frame

This CL eliminates some of the branches and simplifies the bookkeeping,
since it needs to be done whether the FrameOwner is local or remote.

BUG=none
==========

[dcheng, 2016-03-16 09:26:18]

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (left):

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:125: if
(m_remotePlatformLayer)
This lets Frame::disconnectOwnerElement be devirtualized. Previously, this code
lived here because WebFrame::swap() didn't call Frame::detach(), but that's no
longer the case.

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrame.cpp:74: FrameOwner* owner =
oldFrame->owner();
Note: there's actually a rather embarrassing leak. A remote frame maintains a
map of child frames to frame owners, to keep the frame owners alive.
Unfortunately, this map does /not/ get updated on swap.

If non-Oilpan support could be removed, fixing this would be easy: I would just
get rid of the map and use a self-persistent that gets set/unset in
setContentFrame/clearContentFrame, and rely on the stack pointer here to keep it
live in swap().

Unfortunately, since I still need to support non-Oilpan, it gets trickier. I
can't just make this a RefPtrWillBeRawPtr<FrameOwner> because FrameOwner is a
mixin and is a base class of an already refcounted class. I plan on addressing
this in a followup.

[dcheng, 2016-03-16 09:27:31]

This CL is prompted by feedback from haraken@ on https://crbug.com/582004: it's
good to clear the pointer to keep it from retaining no-longer needed frames.

Unfortunately, it's not a 100% solution yet due to the plumbing issues in
WebRemoteFrameImpl, which I will address in a followup.

[dcheng, 2016-03-16 09:28:19]

Description was changed from

==========
RemoteBridgeFrameOwner: keep better track of the attached frame

This CL eliminates some of the branches and simplifies the bookkeeping,
since it needs to be done whether the FrameOwner is local or remote.

BUG=none
==========

to

==========
RemoteBridgeFrameOwner: keep better track of the attached frame

This CL eliminates some of the branches and simplifies the bookkeeping,
since it needs to be done whether the FrameOwner is local or remote.
Doing this bookkeeping is important, since it prevents a remote frame
owner from retaining a WebLocalFrameImpl and keeping it live after it's
already swapped out.

BUG=none
==========

[haraken, 2016-03-16 14:17:40]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-03-16 14:17:41]

LGTM

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Frame.cpp:118: m_owner = nullptr;

Is there any assert to check that m_owner's content frame is in sync with the
actual content frame?

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameOwner.h (right):

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameOwner.h:42: class CORE_EXPORT
StubFrameOwner : public NoBaseWillBeGarbageCollectedFinalized<StubFrameOwner>,
public FrameOwner {

Is there any better name than StubFrameOwner? DummyFrameOwner or something
better.

[dcheng, 2016-03-16 16:51:40]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-03-16 16:51:41]

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Frame.cpp:118: m_owner = nullptr;
On 2016/03/16 at 14:17:41, haraken wrote:
> Is there any assert to check that m_owner's content frame is in sync with the
actual content frame?

Done. I did it in the two "real" FrameOwner implementations though: otherwise, I
have to add a new virtual to FrameOwner for ASSERT-only purposes.

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameOwner.h (right):

https://codereview.chromium.org/1802953003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameOwner.h:42: class CORE_EXPORT
StubFrameOwner : public NoBaseWillBeGarbageCollectedFinalized<StubFrameOwner>,
public FrameOwner {
On 2016/03/16 at 14:17:41, haraken wrote:
> Is there any better name than StubFrameOwner? DummyFrameOwner or something
better.

Done.

[dcheng, 2016-03-16 16:51:41]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1802953003/#ps40001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-03-16 16:51:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802953003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802953003/40001

[dcheng, 2016-03-16 17:28:29]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-03-16 17:28:30]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1802953003/#ps60001
(title: "Doing work in constructors is evil.")

[commit-bot: I haz the power, 2016-03-16 17:28:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802953003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802953003/60001

[commit-bot: I haz the power, 2016-03-16 19:09:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-16 19:12:12]

Description was changed from

==========
RemoteBridgeFrameOwner: keep better track of the attached frame

This CL eliminates some of the branches and simplifies the bookkeeping,
since it needs to be done whether the FrameOwner is local or remote.
Doing this bookkeeping is important, since it prevents a remote frame
owner from retaining a WebLocalFrameImpl and keeping it live after it's
already swapped out.

BUG=none
==========

to

==========
RemoteBridgeFrameOwner: keep better track of the attached frame

This CL eliminates some of the branches and simplifies the bookkeeping,
since it needs to be done whether the FrameOwner is local or remote.
Doing this bookkeeping is important, since it prevents a remote frame
owner from retaining a WebLocalFrameImpl and keeping it live after it's
already swapped out.

BUG=none

Committed: https://crrev.com/2a9d38d2a9724e5fc907ebcb239fc39f0929add6
Cr-Commit-Position: refs/heads/master@{#381508}
==========

[commit-bot: I haz the power, 2016-03-16 19:12:13]

Patchset 4 (id:??) landed as
https://crrev.com/2a9d38d2a9724e5fc907ebcb239fc39f0929add6
Cr-Commit-Position: refs/heads/master@{#381508}