Move Frame to the Oilpan heap.

Move Frame to the Oilpan heap.

Move Frame and related classes to the Oilpan heap.

R=haraken,dcheng,ager,tkent
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182337

[sof, 2014-08-28 21:54:01]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-08-28 21:54:01]

This isn't quite ready for review yet as there are some failing tests and some
rougher edges still remain, but thought I'd pass along this CL now as a heads-up
of what I'm upto.

So, if anyone else have looked into details of what it would take to move Frame
to the heap & see something obviously problematic or wrong here, do let me know.
But please don't spend time reviewing this is any detail.

[sof, 2014-09-02 13:59:50]

Please take a look.

This is now ready, I think. The tricky part here proved how to handle a GCed
representation of Frame that might be externally closed -- something that should
take immediate effect & not be delayed until the object is swept. I've tried to
handle that in an orderly manner with the help of a pair of "protector"
abstractions. Workable?

If there are people that really ought to be Cc:ed here, please let me know.
(Wisely or not, I cleared the default Cc: line for this CL as it was rather
enormous.)

[haraken, 2014-09-02 15:00:00]

Let me take a close look tomorrow.

> If there are people that really ought to be Cc:ed here, please let me know.
> (Wisely or not, I cleared the default Cc: line for this CL as it was rather
> enormous.)

I'd prefer ccing people who are cced by default even if the CL is enormous. I
personally want to avoid giving the community an impression that we're doing
something in a stealth mode :)

[sof, 2014-09-02 15:09:46]

Include Cc:ers :)

[haraken, 2014-09-03 09:09:39]

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

[haraken, 2014-09-03 09:09:40]

Sorry about the review delay.

FrameProtector is an interesting idea, but it looks a bit fragile to me in the
current shape.

If I'm understanding FrameProtector correctly, I think you're assuming that a
Frame is "own"ed by only one on-heap object and a machine stack. If this
assumption is correct, it's enough to protect all on-stack pointers to the Frame
and send Frame::willBeDestroyed when the on-heap object that owns the Frame is
disposed. This is what the CL is doing.

However, what guarantees that the assumption is correct? If there is another
on-heap object that owns the Frame, the assumption breaks. Given that the
current code is written in a way in which on-heap objects refer to the Frame
using a RefPtr, we have to assume that the Frame can be owned by multiple
on-heap objects. (That being said, I agree that in reality the Frame is owned by
only one on-heap object and thus your CL works as expected.)

[sof, 2014-09-03 09:53:24]

On 2014/09/03 09:09:40, haraken wrote:
> Sorry about the review delay.
> 

Thinking through first the overall approach, makes good sense.

> FrameProtector is an interesting idea, but it looks a bit fragile to me in the
> current shape.
> 
> If I'm understanding FrameProtector correctly, I think you're assuming that a
> Frame is "own"ed by only one on-heap object and a machine stack. If this
> assumption is correct, it's enough to protect all on-stack pointers to the
Frame
> and send Frame::willBeDestroyed when the on-heap object that owns the Frame is
> disposed. This is what the CL is doing.
> 

Yes. The assumption, with a couple of exceptional cases, is that one external
party will call close() to explicitly let go of the (wrapped) Frame object that
it holds. At which point willBeDestroyed() (which could have a different name)
is invoked, or if there are stack references, delayed invoked until they've all
gone out of scope.

If there are other references besides, they'll be able to access a
now-closed&detached, but alive, Frame object afterwards.

> However, what guarantees that the assumption is correct? If there is another
> on-heap object that owns the Frame, the assumption breaks. Given that the
> current code is written in a way in which on-heap objects refer to the Frame
> using a RefPtr, we have to assume that the Frame can be owned by multiple
> on-heap objects. (That being said, I agree that in reality the Frame is owned
by
> only one on-heap object and thus your CL works as expected.)

This would suggest that we try to make it harder and/or catch such multiple
references from being introduced. I'm not sure how though yet :)

[Mads Ager (chromium), 2014-09-03 10:07:52]

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

[Mads Ager (chromium), 2014-09-03 10:07:55]

Thanks for tackling this! Overall, this looks like what we will have to do to
move Frame to Oilpan. It is a shame that we have to use prompt finalization for
Frame (and therefore keep track of the willBeDestroyed calls) but I don't think
there is any short-term way around that.

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameProtector.h (right):

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameProtector.h:15: class FrameProtector {
Let's add a comment to FrameProtector stating that disposal of a Frame is
delayed until there are no more FrameProtectors for that frame?

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameProtector.h:23: void trace(Visitor*);
Trace is unimplemented? It is not used because FrameProtector is in fact always
STACK_ALLOCATED, but we should implement it? We might be able to extend the
STACK_ALLOCATED check in the plugin to state that a STACK_ALLOCATED class can be
a part object in other STACK_ALLOCATED objects. (I can't remember if we do
something like that already? This cannot be marked as STACK_ALLOCATED now,
right?)

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:159: if (hasBeenDisposed())
Could you explain when we get a detach of a local frame after it has been
disposed? (Maybe add a comment?)

https://codereview.chromium.org/517043003/diff/140001/Source/core/page/FocusC...
File Source/core/page/FocusController.h (right):

https://codereview.chromium.org/517043003/diff/140001/Source/core/page/FocusC...
Source/core/page/FocusController.h:119: Page* m_page;
RawPtrWillBeMember<Page> and trace it?

https://codereview.chromium.org/517043003/diff/140001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/517043003/diff/140001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.cpp:89: detach();
#if !ENABLE(OILPAN) to make it clear that detach should not happen in the
destructor when moving to Oilpan?

https://codereview.chromium.org/517043003/diff/140001/Source/web/PopupContain...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/517043003/diff/140001/Source/web/PopupContain...
Source/web/PopupContainer.cpp:248: // destructed. That may happen longer after
the FrameView has been
longer -> long

https://codereview.chromium.org/517043003/diff/140001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/140001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1562: if (!frame && m_frame)
Why the !frame check? If we lose the reference to m_frame it seems to me that it
should always be notified that it is going down?

https://codereview.chromium.org/517043003/diff/140001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/140001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:868: m_frame = frame;
In the oilpan build, shouldn't we do a willBeDestroyed notification if m_frame
is not null?

[haraken, 2014-09-03 11:32:58]

Help me understand: Where do we need prompty finalization of Frame objects? I
guess most FrameDestructionObservers will work without being notified promptly.
I'd like to understand the place where the prompt finalization is unavoidable.

I thought about introducing something like RefCountedPtr<Frame> to oilpan's
infrastructure, but it wouldn't be a nice idea.

[sof, 2014-09-03 11:45:02]

On 2014/09/03 11:32:58, haraken wrote:
> Help me understand: Where do we need prompty finalization of Frame objects? I
> guess most FrameDestructionObservers will work without being notified
promptly.
> I'd like to understand the place where the prompt finalization is unavoidable.
> 

I don't profess to having a complete picture of where that's needed&unavoidable,
but having window.closed return true before the next GC would be one place where
the observers need to be notified promptly.

[haraken, 2014-09-03 12:10:14]

On 2014/09/03 11:45:02, sof wrote:
> On 2014/09/03 11:32:58, haraken wrote:
> > Help me understand: Where do we need prompty finalization of Frame objects?
I
> > guess most FrameDestructionObservers will work without being notified
> promptly.
> > I'd like to understand the place where the prompt finalization is
unavoidable.
> > 
> 
> I don't profess to having a complete picture of where that's
needed&unavoidable,
> but having window.closed return true before the next GC would be one place
where
> the observers need to be notified promptly.

In the particular case of window.closed, we can change the implementation so
that it doesn't depend on m_frame (because there should be a more explicit way
to know when the window is closed) :)

[zerny-chromium, 2014-09-03 12:35:06]

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

[zerny-chromium, 2014-09-03 12:35:08]

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameProtector.h (right):

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameProtector.h:23: void trace(Visitor*);
On 2014/09/03 10:07:54, Mads Ager (chromium) wrote:
> Trace is unimplemented? It is not used because FrameProtector is in fact
always
> STACK_ALLOCATED, but we should implement it? We might be able to extend the
> STACK_ALLOCATED check in the plugin to state that a STACK_ALLOCATED class can
be
> a part object in other STACK_ALLOCATED objects. (I can't remember if we do
> something like that already? This cannot be marked as STACK_ALLOCATED now,
> right?)

A stack allocated object may have parts that are and pointers to stack allocated
things. If you can't get it past the plugin file a bug and I'll get it fixed
asap

[sof, 2014-09-03 14:28:28]

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameProtector.h (right):

https://codereview.chromium.org/517043003/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameProtector.h:23: void trace(Visitor*);
On 2014/09/03 12:35:08, zerny-chromium wrote:
> On 2014/09/03 10:07:54, Mads Ager (chromium) wrote:
> > Trace is unimplemented? It is not used because FrameProtector is in fact
> always
> > STACK_ALLOCATED, but we should implement it? We might be able to extend the
> > STACK_ALLOCATED check in the plugin to state that a STACK_ALLOCATED class
can
> be
> > a part object in other STACK_ALLOCATED objects. (I can't remember if we do
> > something like that already? This cannot be marked as STACK_ALLOCATED now,
> > right?)
> 
> A stack allocated object may have parts that are and pointers to stack
allocated
> things. If you can't get it past the plugin file a bug and I'll get it fixed
> asap

It was an oversight not to add a trace() implementation - FrameProtector was
initially STACK_ALLOCATED() - when moving to DISALLOW_ALLOCATION(). Will fix.

[sof, 2014-09-04 13:23:58]

On 2014/09/03 12:10:14, haraken wrote:
> On 2014/09/03 11:45:02, sof wrote:
> > On 2014/09/03 11:32:58, haraken wrote:
> > > Help me understand: Where do we need prompty finalization of Frame
objects?
> I
> > > guess most FrameDestructionObservers will work without being notified
> > promptly.
> > > I'd like to understand the place where the prompt finalization is
> unavoidable.
> > > 
> > 
> > I don't profess to having a complete picture of where that's
> needed&unavoidable,
> > but having window.closed return true before the next GC would be one place
> where
> > the observers need to be notified promptly.
> 
> In the particular case of window.closed, we can change the implementation so
> that it doesn't depend on m_frame (because there should be a more explicit way
> to know when the window is closed) :)

Yes, signalling that differently isn't a real problem.

I'm trying to switch to non-prompt/normal Frame finalization locally atm; some
changes required to DOMWindowProperty, and frame destruction observers. It might
work out, but some detached corner cases to work through.

[haraken, 2014-09-04 13:29:11]

On 2014/09/04 13:23:58, sof wrote:
> On 2014/09/03 12:10:14, haraken wrote:
> > On 2014/09/03 11:45:02, sof wrote:
> > > On 2014/09/03 11:32:58, haraken wrote:
> > > > Help me understand: Where do we need prompty finalization of Frame
> objects?
> > I
> > > > guess most FrameDestructionObservers will work without being notified
> > > promptly.
> > > > I'd like to understand the place where the prompt finalization is
> > unavoidable.
> > > > 
> > > 
> > > I don't profess to having a complete picture of where that's
> > needed&unavoidable,
> > > but having window.closed return true before the next GC would be one place
> > where
> > > the observers need to be notified promptly.
> > 
> > In the particular case of window.closed, we can change the implementation so
> > that it doesn't depend on m_frame (because there should be a more explicit
way
> > to know when the window is closed) :)
> 
> Yes, signalling that differently isn't a real problem.
> 
> I'm trying to switch to non-prompt/normal Frame finalization locally atm; some
> changes required to DOMWindowProperty, and frame destruction observers. It
might
> work out, but some detached corner cases to work through.

I'm sorry I posted a comment that changes the overall approach after seeing your
complete CL...

[sof, 2014-09-04 13:46:12]

On 2014/09/04 13:29:11, haraken wrote:
> On 2014/09/04 13:23:58, sof wrote:
> > On 2014/09/03 12:10:14, haraken wrote:
> > > On 2014/09/03 11:45:02, sof wrote:
> > > > On 2014/09/03 11:32:58, haraken wrote:
> > > > > Help me understand: Where do we need prompty finalization of Frame
> > objects?
> > > I
> > > > > guess most FrameDestructionObservers will work without being notified
> > > > promptly.
> > > > > I'd like to understand the place where the prompt finalization is
> > > unavoidable.
> > > > > 
> > > > 
> > > > I don't profess to having a complete picture of where that's
> > > needed&unavoidable,
> > > > but having window.closed return true before the next GC would be one
place
> > > where
> > > > the observers need to be notified promptly.
> > > 
> > > In the particular case of window.closed, we can change the implementation
so
> > > that it doesn't depend on m_frame (because there should be a more explicit
> way
> > > to know when the window is closed) :)
> > 
> > Yes, signalling that differently isn't a real problem.
> > 
> > I'm trying to switch to non-prompt/normal Frame finalization locally atm;
some
> > changes required to DOMWindowProperty, and frame destruction observers. It
> might
> > work out, but some detached corner cases to work through.
> 
> I'm sorry I posted a comment that changes the overall approach after seeing
your
> complete CL...

If we can avoid having a custom protector abstraction, all for the better. The
CL's footprint won't change much (unfortunately?)

[sof, 2014-09-05 12:18:22]

On 2014/09/04 13:46:12, sof wrote:
> On 2014/09/04 13:29:11, haraken wrote:
> > On 2014/09/04 13:23:58, sof wrote:
> > > On 2014/09/03 12:10:14, haraken wrote:
> > > > On 2014/09/03 11:45:02, sof wrote:
> > > > > On 2014/09/03 11:32:58, haraken wrote:
> > > > > > Help me understand: Where do we need prompty finalization of Frame
> > > objects?
> > > > I
> > > > > > guess most FrameDestructionObservers will work without being
notified
> > > > > promptly.
> > > > > > I'd like to understand the place where the prompt finalization is
> > > > unavoidable.
> > > > > > 
> > > > > 
> > > > > I don't profess to having a complete picture of where that's
> > > > needed&unavoidable,
> > > > > but having window.closed return true before the next GC would be one
> place
> > > > where
> > > > > the observers need to be notified promptly.
> > > > 
> > > > In the particular case of window.closed, we can change the
implementation
> so
> > > > that it doesn't depend on m_frame (because there should be a more
explicit
> > way
> > > > to know when the window is closed) :)
> > > 
> > > Yes, signalling that differently isn't a real problem.
> > > 
> > > I'm trying to switch to non-prompt/normal Frame finalization locally atm;
> some
> > > changes required to DOMWindowProperty, and frame destruction observers. It
> > might
> > > work out, but some detached corner cases to work through.
> > 
> > I'm sorry I posted a comment that changes the overall approach after seeing
> your
> > complete CL...
> 
> If we can avoid having a custom protector abstraction, all for the better. The
> CL's footprint won't change much (unfortunately?)

ok, experimented with various degrees of explicit disposing/notification on
frame closure. No acute shortage of lifetime dependencies in this area.

If we do the minimal thing of having WebLocalFrameImpl::close() merely set an
"is-closed" flag on the frame, but don't clear frame refs for all that is
downstream (DOMWindow, FrameDestructionObserver, DOMWindowProperty objects) at
that point, then these downstream objects will be able to access and use a frame
object without a FrameClient. Which they won't non-Oilpan as the frame would be
released&cleared at the same time as being close()d. There's one test that
fatally runs into this, and a couple of "detached" frame tests that now produce
different output. I've not worked out why yet the latter produce different
output, but hopefully for reasons that aren't fundamentally flawed.

To have the above work, both FrameDestructionObserver and DOMWindowProperty
needs to be on the heap & keep weak frame (back) references. Some extra
thinking&work needed to make that work out for the plugin container
FrameDestructionObserver, but otherwise not a problem. Is this an approach to
try to go with? There's additional work to be done on my part next, if so.

(If not clear, the FrameProtector abstraction will not be needed in this
setting.)

[sof, 2014-09-07 21:02:58]

This is now ready for another look; {,Local,Remote}Frame and WebLocalFrameImpl
containing the key changes. Which is to have WebLocalFrameImpl::close() simply
mark its LocalFrame as having being closed, but not performing any prompt
finalization like in the previous version. (Consequently, no custom protector
stack object used now.)

One consequence of this is that both DOMWindowProperty and
FrameDestructionObserver are now GC mixins -- both having weak references back
to their frame. If many/most implementations of either had been non-GCed object,
this wouldn't be needed, but they all are (except one, the
WebPluginContainerImpl "widget".)

I will have to more carefully go over again the (lack of) finalization of the
frame's attached document & check if some of that needs to be moved to Document.
But details they are -- this approach is now ready for a closer look.

[dcheng, 2014-09-07 22:26:29]

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

[dcheng, 2014-09-07 22:26:34]

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:93: #if !ENABLE(OILPAN)
Why is this block no longer needed with Oilpan? It seems like we'll be missing a
call to setDOMWindow(nullptr) now when the embedder explicitly invokes close().

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.h:88: void setRemotePlatformLayer(WebLayer*);
Nit: it would be better to move pure cleanups like this to a separate patch to
minimize unrelated diffs.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:142: setView(nullptr);
Similarly, is it safe to omit this call to setView(nullptr)? This does do a
non-trivial amount of work (in particular, it will detach an active document).

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:145: loader().dispose(false);
Why do we pass different bool values, depending on if Oilpan is enabled?

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:249: void FrameLoader::dispose(bool
clearFrameContents)
Nit: don't use a bool parameter here. Use an enum.

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:54: // How frames are destroyed
Frame detach is really complicated. Does this comment need to be updated?

[haraken, 2014-09-08 04:51:12]

Overall the approach looks good to me. Started looking in details.

[sof, 2014-09-08 05:14:46]

On 2014/09/08 04:51:12, haraken wrote:
> Overall the approach looks good to me. Started looking in details.

I notice there's a plugin finalization/detach test failing on Macs (not entirely
surprising.) I'll go after that today.

[haraken, 2014-09-08 07:25:59]

Thanks for a great CL. The overall approach looks good.

As commented below, it might be worth trying to reduce #if ENABLE(OILPAN). We
don't want to have different call paths between oilpan builds and non-oilpan
builds as much as possible.

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Edi...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Edi...
Source/core/editing/Editor.cpp:105:
m_editor->m_frame->selection().revealSelection(ScrollAlignment::alignToEdgeIfNeeded,
RevealExtent);

It looks better to use frame().foo than m_frame->foo. The same comment for other
parts of this CL.

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:248: if (guard->hasOneRef() &&
!m_selection.isNonOrphanedCaretOrRange())

Would it be possible to use guard->hasBeenClosed() in non-oilpan builds as well?
(To make it workable, we'll need to enable the close call in non-oilpan builds.)

We don't want to have different call paths between oilpan builds and non-oilpan
builds as much as possible.

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1182: view->clear();

What is this change for?

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Spe...
File Source/core/editing/SpellCheckRequester.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Spe...
Source/core/editing/SpellCheckRequester.cpp:78: return
PassRefPtrWillBeRawPtr<SpellCheckRequest>();

nullptr

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Spe...
Source/core/editing/SpellCheckRequester.cpp:268: if (m_frame->document())

Why do we need to introduce this check in this CL?

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.cpp:42: if (this->frame()) {

What is this change for?

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.cpp:49:
DOMWindowProperty::~DOMWindowProperty()

Is it guaranteed that willDestroyGlobalObjectInFrame is called before
DOMWindowProperty's destructor is called?

If yes, let's add ASSERT(!m_frame).

If no, you'll need to change LocalDOMWindow::m_properties to a hash set of weak
members, so that the DOMWindowProperty gets removed from the hash set when the
DOMWindowProperty gets destructed.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:36: class DOMWindowProperty : public
WillBeGarbageCollectedMixin {

Would it be possible to make DOMWindowProperty a GC-mixin before landing this
change? (Then we can make this CL smaller.)

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:88: dispose();

Shall we rewrite this to:

#if ENABLE(OILPAN)
  ASSERT(!m_owner);
#else
  dispose();
#endif

?

(It would be even better if we could rewrite non-oilpan code so that it uses
explicit dispose() as well. Then we won't need different call paths between
oilpan builds and non-oilpan builds.)

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.h:109: FrameHost* m_host;

This should be Member<FrameHost>.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:419: // 'emulates' the RefPtr-cycle that is kept
between the two objects non-Oilpan.

Help me understand: What's a lifetime relationship between LocalFrame and
FrameView in non-oilpan? I want to understand why the reference cycle between
LocalFrame and FrameView is not a problem.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:501: reset();

Why is it OK to just remove the reset() call from oilpan builds? I guess someone
needs to call the reset() before the LocalDOMWindow gets destructed.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:549: reset();

Ditto.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:1225: // transition to 'true' slightly
earlier.

If possible, we should make this behavior change before landing this CL. It
would be better if we could use the explicit close() call in non-oilpan builds
as well and avoid introducing different call paths.

I believe the new behavior is a correct one.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:118: dispose();

It looks unsafe to call dispose() methods in LocalFrame's destructor.

Probably can we introduce:

  PersistentHeapHashSet<WeakMember<LocalFrame>, OwnPtr<LocalFrameDisposer>>
m_frames;

and call the dispose() method in the destructor of LocalFrameDisposer?

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:312: m_destructionObservers.clear();

This is just for performance optimization and not needed for correctness, right?

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:21: return frame.release();

return adoptRefWillBeNoop(...)

https://codereview.chromium.org/517043003/diff/260001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.cpp:248: if (isStopped())

Can we simply replace the above isDetached() with isStopped() ? (We don't want
to have different call paths between oilpan builds and non-oilpan builds.)

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:249: void FrameLoader::dispose(bool
clearFrameContents)

clearFrameContents => isDuringFrameDestruction ?

enum is better either way :)

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:271: m_frame->script().enableEval();

Is it to safe to touch m_frame here if the dispose() is called in LocalFrame's
destructor?

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:277: m_frame->navigationScheduler().cancel();

Ditto. It looks not safe to touch m_frame here.

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:911: if (client())

Why was this check not needed before this CL but is needed after this CL?

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1344: RefPtrWillBeRawPtr<LocalFrame>
protect(m_frame.get());

.get() is needed? The same comment for other parts in this file.

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Navi...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Navi...
Source/core/loader/NavigationScheduler.cpp:375: RefPtrWillBeRawPtr<LocalFrame>
protect(m_frame.get());

Is .get() needed?

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Navi...
Source/core/loader/NavigationScheduler.cpp:391: RefPtrWillBeRawPtr<LocalFrame>
protect(m_frame.get());

Ditto.

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FocusC...
File Source/core/page/FocusController.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FocusC...
Source/core/page/FocusController.h:119: Page* m_page;

You can use Member<Page>.

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FrameT...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FrameT...
Source/core/page/FrameTree.cpp:54: // FIXME: Why is this here? Doesn't this
parallel what we already do in ~LocalFrame?

Not related to this CL, it looks like we can remove line 55 - 60 from non-oilpan
builds as well.

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.cpp:89: detach();

Shall we add:

#if ENABLE(OILPAN)
  ASSERT(!m_deferredFrames.size());
#endif

?

However, as I commented in WebViewImpl.cpp, it would be great if we can use the
same call paths between oilpan and non-oilpan.

https://codereview.chromium.org/517043003/diff/260001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.cpp:62: if (!m_navigator.frame() ||
!m_navigator.frame()->client())

Why do we need to add this check?

https://codereview.chromium.org/517043003/diff/260001/Source/web/ExternalPopu...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.h:90: WebExternalPopupMenu* m_webExternalPopupMenu;

Are you sure that ExternalPopupMenu does not outlive m_popupMenuClient and
m_webExternalPopupMenu? If ExternalPopupMenu outlives them, they will become
dangling pointers.

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupContain...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupContain...
Source/web/PopupContainer.cpp:249: // closed and detached from its Frame (and
Page), hence the need to check.

Actually I don't fully understand the lifetime relationship between the popups
and the WebView.

Why isn't WebViewImpl::closePagePopup() called and gracefully clear all popups
before the FrameView's page shuts down?

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupMenuChr...
File Source/web/PopupMenuChromium.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupMenuChr...
Source/web/PopupMenuChromium.cpp:82: m_popup.clear();

What is this change for?

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

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:223: void WebFrame::trace(Visitor* visitor, WebFrame*
frame)

It looks dangerous to do something non-trivial in trace methods (although I
haven't checked if this particular trace method is safe or not), because trace
methods can be called while constructing an object.

Can we restructure the code so that trace methods just call
visitor->trace(m_member)'s?

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1526: return adoptRef(frame).leakRef();

Just help me understand: In oilpan builds, who keeps the WebLocalFrameImpl alive
until WebLocalFrameImpl::close() is called?

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.h:202: // FIXME: Oilpan: consider moving
Widget to the heap, allowing this

Yeah, it would be nicer if we could move the Widget to the heap. What's blocking
the work?

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:117: return adoptRef(frame).leakRef();

The same question as the WebLocalFrameImpl: In oilpan builds, who keeps the
WebRemoteFrameImpl alive until WebRemoteFrameImpl::close() gets called?

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:322: delete deferrer;

Can we use OwnPtr<ScopedPageLoadDeferrer> and avoid calling explicit 'delete'?
Then hopefully it would become possible to share code between oilpan and
non-oilpan.

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:332: m_remoteMainFrame.clear();

Can we add an ASSERT somewhere to verify that only either of m_localMainFrame
and m_remoteMainFrame has an actual value?

[dcheng, 2014-09-08 08:34:18]

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:419: // 'emulates' the RefPtr-cycle that is kept
between the two objects non-Oilpan.
On 2014/09/08 07:25:58, haraken wrote:
> 
> Help me understand: What's a lifetime relationship between LocalFrame and
> FrameView in non-oilpan? I want to understand why the reference cycle between
> LocalFrame and FrameView is not a problem.

LocalFrame and FrameView ref each other, but when a LocalFrame is detached by
FrameLoader::detachFromParent(), FrameLoader clears LocalFrame's ref to
FrameView which should break the cycle.

[sof, 2014-09-08 21:17:47]

Thanks so much for finding the time for the in-depth reviewing; awesome! I think
we're getting there, but a couple more iterations needed, at least.

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Edi...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Edi...
Source/core/editing/Editor.cpp:105:
m_editor->m_frame->selection().revealSelection(ScrollAlignment::alignToEdgeIfNeeded,
RevealExtent);
On 2014/09/08 07:25:57, haraken wrote:
> 
> It looks better to use frame().foo than m_frame->foo. The same comment for
other
> parts of this CL.
> 

Yes; switched over throughout.

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1182: view->clear();
On 2014/09/08 07:25:57, haraken wrote:
> 
> What is this change for?

It does not belong here; reverted.

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Spe...
File Source/core/editing/SpellCheckRequester.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Spe...
Source/core/editing/SpellCheckRequester.cpp:78: return
PassRefPtrWillBeRawPtr<SpellCheckRequest>();
On 2014/09/08 07:25:57, haraken wrote:
> 
> nullptr

Simplified.

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Spe...
Source/core/editing/SpellCheckRequester.cpp:268: if (m_frame->document())
On 2014/09/08 07:25:57, haraken wrote:
> 
> Why do we need to introduce this check in this CL?

Thanks for bringing this up. With the prompt disposing of LocalFrames upon
calling close(), we could end up having access to a document-less m_frame here.
This broke editing/spelling/spellcheck-async-remove-frame.html.

As close() now no longer causes any explicit disposal operations, the check is
no longer needed. m_frame will still be accessible via these closed&detached
frames (and have a document.) Reverted.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.cpp:42: if (this->frame()) {
On 2014/09/08 07:25:57, haraken wrote:
> 
> What is this change for?

Reverted back (a leftover from an experiment to see what it would take to make
m_frame a private across the codebase.)

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.cpp:49:
DOMWindowProperty::~DOMWindowProperty()
On 2014/09/08 07:25:57, haraken wrote:
> 
> Is it guaranteed that willDestroyGlobalObjectInFrame is called before
> DOMWindowProperty's destructor is called?
> 
> If yes, let's add ASSERT(!m_frame).
> 
> If no, you'll need to change LocalDOMWindow::m_properties to a hash set of
weak
> members, so that the DOMWindowProperty gets removed from the hash set when the
> DOMWindowProperty gets destructed.

LocalDOMWindow::reset() calls willDestroyGlobalObjectInFrame() and is not
guaranteed to be called with Oilpan.

As DOMWindowProperty and LocalDOMWindow keep strong references to one another, I
don't see the need to do what you suggest. The m_frame reference is a weak one,
so it'll be cleared for the DOMWindowProperty (as it will be for the
FrameDestructionObserver-derived LocalDOMWindow.)

As it is now, ~DOMWindowProperty has no destructor with Oilpan. I hope we can
remove some upstream *Finalized uses as a result (but haven't looked into
details yet.)

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:36: class DOMWindowProperty : public
WillBeGarbageCollectedMixin {
On 2014/09/08 07:25:57, haraken wrote:
> 
> Would it be possible to make DOMWindowProperty a GC-mixin before landing this
> change? (Then we can make this CL smaller.)

The only potentially tricky part I can think of right now is how to get the
frame clearing to be reliably done; I can investigate.

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:88: dispose();
On 2014/09/08 07:25:58, haraken wrote:
> 
> Shall we rewrite this to:
> 
> #if ENABLE(OILPAN)
>   ASSERT(!m_owner);
> #else
>   dispose();
> #endif
> 
> ?
> 
> (It would be even better if we could rewrite non-oilpan code so that it uses
> explicit dispose() as well. Then we won't need different call paths between
> oilpan builds and non-oilpan builds.)

I've added that as an Oilpan assert (in dispose()); I _believe_ that captures
the gist of this comment.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:93: #if !ENABLE(OILPAN)
On 2014/09/07 22:26:33, dcheng (OOO) wrote:
> Why is this block no longer needed with Oilpan? It seems like we'll be missing
a
> call to setDOMWindow(nullptr) now when the embedder explicitly invokes
close().

It is handled implicitly upon the next GC; LocalDOMWindow will have its
(Local)Frame* reference cleared as it is a weak member. Should the Frame and its
attached LocalDOMWindow die at the same time, it would be unsafe to attempt to
access m_domWindow from this finalizer..but it would be redundant in that case.

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.h:109: FrameHost* m_host;
On 2014/09/08 07:25:58, haraken wrote:
> 
> This should be Member<FrameHost>.

Yes, now a traced RawPtrWillBeMember<>.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:501: reset();
On 2014/09/08 07:25:58, haraken wrote:
> 
> Why is it OK to just remove the reset() call from oilpan builds? I guess
someone
> needs to call the reset() before the LocalDOMWindow gets destructed.

Notifying the registered DOMWindowProperty instances isn't needed, they're being
swept at the same time. The property clearing in resetDOMWindowProperties() is
also not of value during finalization.

If not clear, the reason for having to disable these two lines is that when the
LocalFrame is swept, its strong reference to the LocalDOMWindow goes. i.e., the
will no longer trigger an explicit reset() when being destructed. Hence the
assert will no longer apply. And due to the fact that m_properties (holding the
DOMWindowProperty instances) is now on the heap, we cannot call reset() from
here & unsafely access m_properties.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:549: reset();
On 2014/09/08 07:25:58, haraken wrote:
> 
> Ditto.

The additional frame clearing that that reset() effectively forwards to the
DOMWindowProperty instances is now handled by keeping WeakMember<LocalFrame>s
instead.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:1225: // transition to 'true' slightly
earlier.
On 2014/09/08 07:25:58, haraken wrote:
> 
> If possible, we should make this behavior change before landing this CL. It
> would be better if we could use the explicit close() call in non-oilpan builds
> as well and avoid introducing different call paths.
> 
> I believe the new behavior is a correct one.

It wouldn't be a problem to separate it out and land it first, making non-Oilpan
and Oilpan behave the same.

I'm slightly nervous that it might break some old&ingrained assumptions in code
using window.closed though. (no, I don't know what code that might be! :) )

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:118: dispose();
On 2014/09/08 07:25:58, haraken wrote:
> 
> It looks unsafe to call dispose() methods in LocalFrame's destructor.
> 
> Probably can we introduce:
> 
>   PersistentHeapHashSet<WeakMember<LocalFrame>, OwnPtr<LocalFrameDisposer>>
> m_frames;
> 
> and call the dispose() method in the destructor of LocalFrameDisposer?

Why do you consider it unsafe? loader() (FrameLoader) is a part object of the
LocalFrame.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:145: loader().dispose(false);
On 2014/09/07 22:26:34, dcheng (OOO) wrote:
> Why do we pass different bool values, depending on if Oilpan is enabled? 

As dispose() is called from the dtor, we have to be careful not to touch other
heap objects when finalizing here (as they might have already been finalized.)
Those objects and their finalizers are instead responsible for carrying out the
needed actions that the FrameLoader::dispose() cannot perform.

For the Frame-accessible Document, as mentioned in the dispose() FIXME and
elsewhere, this needs to be considered carefully again - there's considerable
amount of code that's indirectly triggered here. I don't consider myself done
here -- expanded the FrameLoader::dispose() FIXME just to make that clear.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:312: m_destructionObservers.clear();
On 2014/09/08 07:25:58, haraken wrote:
> 
> This is just for performance optimization and not needed for correctness,
right?

Yes, I just thought it appropriate to release the traced references at the point
we're through with them, but of no real importance. I won't insist we keep it =>
removed.

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:21: return frame.release();
On 2014/09/08 07:25:58, haraken wrote:
> 
> return adoptRefWillBeNoop(...)

Done.

https://codereview.chromium.org/517043003/diff/260001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.cpp:248: if (isStopped())
On 2014/09/08 07:25:58, haraken wrote:
> 
> Can we simply replace the above isDetached() with isStopped() ? (We don't want
> to have different call paths between oilpan builds and non-oilpan builds.)

I thought it worthwhile to call out as separate at this stage (I'm slightly
worried that we might be missing some other code that relies on seeing the
Detached state, but won't now with Oilpan..). 

But that simplification could be done.

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:249: void FrameLoader::dispose(bool
clearFrameContents)
On 2014/09/07 22:26:34, dcheng (OOO) wrote:
> Nit: don't use a bool parameter here. Use an enum.

Done.

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:277: m_frame->navigationScheduler().cancel();
On 2014/09/08 07:25:58, haraken wrote:
> 
> Ditto. It looks not safe to touch m_frame here.

See the FIXME above? :) If NavigationScheduler is a part object of the
FrameLoader (which again is a part object of the LocalFrame), how can m_frame's
allocation have been reclaimed at this stage?

Arguably unsavoury, but is it unsafe?

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:911: if (client())
On 2014/09/08 07:25:58, haraken wrote:
> 
> Why was this check not needed before this CL but is needed after this CL?

The FrameClient is cleared upon calling close() on the Frame, but downstream
uses of the Frame aren't cleared as finalization is delayed. Hence we can end up
calling FrameLoader methods where the client isn't available, like here.

(I don't have the failing testcase in front of me.)

https://codereview.chromium.org/517043003/diff/260001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1344: RefPtrWillBeRawPtr<LocalFrame>
protect(m_frame.get());
On 2014/09/08 07:25:58, haraken wrote:
> 
> .get() is needed? The same comment for other parts in this file.

Ambiguity results, if not: Member<LocalFrame> can either be implicitly converted
into LocalFrame* or (const RawPtr<LocalFrame>), for which RawPtr<>() has
constructor overloads for.

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FocusC...
File Source/core/page/FocusController.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FocusC...
Source/core/page/FocusController.h:119: Page* m_page;
On 2014/09/08 07:25:59, haraken wrote:
> 
> You can use Member<Page>.

Done.

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FrameT...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/page/FrameT...
Source/core/page/FrameTree.cpp:54: // FIXME: Why is this here? Doesn't this
parallel what we already do in ~LocalFrame?
On 2014/09/08 07:25:59, haraken wrote:
> 
> Not related to this CL, it looks like we can remove line 55 - 60 from
non-oilpan
> builds as well.

I don't actually understand the FIXME's claim.

https://codereview.chromium.org/517043003/diff/260001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.cpp:62: if (!m_navigator.frame() ||
!m_navigator.frame()->client())
On 2014/09/08 07:25:59, haraken wrote:
> 
> Why do we need to add this check?

The client is consulted when building up the Beacon request (UA and Referrer
info.) If the Frame is closed, but still detachedly accessible, it will have no
FrameClient but be non-null. Hence the need for the extra check.

Relevant test: http/tests/navigation/beacon-detached-no-crash.html

https://codereview.chromium.org/517043003/diff/260001/Source/web/ExternalPopu...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.h:90: WebExternalPopupMenu* m_webExternalPopupMenu;
On 2014/09/08 07:25:59, haraken wrote:
> 
> Are you sure that ExternalPopupMenu does not outlive m_popupMenuClient and
> m_webExternalPopupMenu? If ExternalPopupMenu outlives them, they will become
> dangling pointers.

m_webExternalPopupMenu is explicitly close()d when ExternalPopupMenu is
disconnected, so this should be the same with or without Oilpan. Right?

PopupMenuClient is a platform object not on the heap, so I don't see any (new)
lifetime issue.

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupContain...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupContain...
Source/web/PopupContainer.cpp:249: // closed and detached from its Frame (and
Page), hence the need to check.
On 2014/09/08 07:25:59, haraken wrote:
> 
> Actually I don't fully understand the lifetime relationship between the popups
> and the WebView.
> 
> Why isn't WebViewImpl::closePagePopup() called and gracefully clear all popups
> before the FrameView's page shuts down?

There are quite a number of objects and abstractions to deal with when it comes
to popup handling & I don't claim to have grasped them all, but the problem
enters via the PopupMenuChromium finalizer (PopupMenuChromium is on the heap,
and implements PopupMenu and is a Member of RenderMenuList.) It will hide its
popup container, if that's needed.

If needed, the rendering related object is actually closed & PopupContainer will
eventually receive a notifyPopupHidden notification. And we end up here.

This may happen before or after the HTMLFrameOwnerElement is detached and
finalized. If it is finalized first, then its LocalFrame will become detached
from its FrameHost..and it will have no Page accessible. Hence the need for the
check here.

That's a very operational & low-level explanation of the "why" here, but the
only one I have. Notice that it is safe to access the FrameView's LocalFrame ,
as that's a Persistent. 

I've tried to improve the comment a bit.

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupMenuChr...
File Source/web/PopupMenuChromium.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/PopupMenuChr...
Source/web/PopupMenuChromium.cpp:82: m_popup.clear();
On 2014/09/08 07:25:59, haraken wrote:
> 
> What is this change for?

The hiding during disconnect isn't separately needed provided the PopupContainer
handles the FrameHost-detached LocalFrame condition, which I think there's no
way around (now.) So, I've reverted the earlier triggering of hiding here -
sorry about the noise/mess.

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

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:223: void WebFrame::trace(Visitor* visitor, WebFrame*
frame)
On 2014/09/08 07:25:59, haraken wrote:
> 
> It looks dangerous to do something non-trivial in trace methods (although I
> haven't checked if this particular trace method is safe or not), because trace
> methods can be called while constructing an object.
> 
> Can we restructure the code so that trace methods just call
> visitor->trace(m_member)'s?

Perhaps the (static) method should be renamed as traceChildren()? This is
tracing the children of a WebFrame, allowing that tracing code to be shared
between local and remote "impl" objects.

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1526: return adoptRef(frame).leakRef();
On 2014/09/08 07:25:59, haraken wrote:
> 
> Just help me understand: In oilpan builds, who keeps the WebLocalFrameImpl
alive
> until WebLocalFrameImpl::close() is called?

If it needs to be kept alive, WebViewImpl::setMainFrame() assigns it to a
Persistent that it keeps.

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.h:202: // FIXME: Oilpan: consider moving
Widget to the heap, allowing this
On 2014/09/08 07:25:59, haraken wrote:
> 
> Yeah, it would be nicer if we could move the Widget to the heap. What's
blocking
> the work?

I had a look earlier when moving WebPluginContainerImpl to the heap, and believe
it will work out ok. A slight "pity" to have just this odd container object be
the only motivation, though.

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:117: return adoptRef(frame).leakRef();
On 2014/09/08 07:25:59, haraken wrote:
> 
> The same question as the WebLocalFrameImpl: In oilpan builds, who keeps the
> WebRemoteFrameImpl alive until WebRemoteFrameImpl::close() gets called?

Same place, via WebViewImpl's use of Persistent<>s for any main frame that is
assigned to it.

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:322: delete deferrer;
On 2014/09/08 07:25:59, haraken wrote:
> 
> Can we use OwnPtr<ScopedPageLoadDeferrer> and avoid calling explicit 'delete'?
> Then hopefully it would become possible to share code between oilpan and
> non-oilpan.

I don't quite see where you're going with this.

https://codereview.chromium.org/517043003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:332: m_remoteMainFrame.clear();
On 2014/09/08 07:25:59, haraken wrote:
> 
> Can we add an ASSERT somewhere to verify that only either of m_localMainFrame
> and m_remoteMainFrame has an actual value?

assert added to ~WebViewImpl.

[sof, 2014-09-10 20:41:51]

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.h (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:36: class DOMWindowProperty : public
WillBeGarbageCollectedMixin {
On 2014/09/08 21:17:45, sof wrote:
> On 2014/09/08 07:25:57, haraken wrote:
> > 
> > Would it be possible to make DOMWindowProperty a GC-mixin before landing
this
> > change? (Then we can make this CL smaller.)
> 
> The only potentially tricky part I can think of right now is how to get the
> frame clearing to be reliably done; I can investigate.

The DOMWindowProperty changes to make it a GC mixin was handled via r181717 (
https://codereview.chromium.org/552733003/ ) & this CL has been rebased on top
of it.

Reduced the file footprint by ~50 :)

[sof, 2014-09-11 13:27:05]

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:248: if (guard->hasOneRef() &&
!m_selection.isNonOrphanedCaretOrRange())
On 2014/09/08 07:25:57, haraken wrote:
> 
> Would it be possible to use guard->hasBeenClosed() in non-oilpan builds as
well?
> (To make it workable, we'll need to enable the close call in non-oilpan
builds.)
> 
> We don't want to have different call paths between oilpan builds and
non-oilpan
> builds as much as possible.

Overloooked previously; now done.

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

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:88: dispose();
On 2014/09/08 21:17:45, sof wrote:
> On 2014/09/08 07:25:58, haraken wrote:
> > 
> > Shall we rewrite this to:
> > 
> > #if ENABLE(OILPAN)
> >   ASSERT(!m_owner);
> > #else
> >   dispose();
> > #endif
> > 
> > ?
> > 
> > (It would be even better if we could rewrite non-oilpan code so that it uses
> > explicit dispose() as well. Then we won't need different call paths between
> > oilpan builds and non-oilpan builds.)
> 
> I've added that as an Oilpan assert (in dispose()); I _believe_ that captures
> the gist of this comment.

This turned out a step too far -- "non local" FrameOwners on remote (only?)
frames do not have a detach, meaning that m_owner will be non-null during
finalization. Retired the Oilpan-only assert.

[haraken, 2014-09-11 14:47:26]

The second round of comments. The next one will be final :) Some of the comments
are on your previous patch set since I couldn't review it in one go :D

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:419: // 'emulates' the RefPtr-cycle that is kept
between the two objects non-Oilpan.

Would you add a comment about how the ref-cycle gets broken, as dcheng
explained?

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:509: reset();

Not related to this CL, why are we calling reset() after m_hasBeenReset becomes
true?

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrameView.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Remot...
Source/core/frame/RemoteFrameView.h:36: RefPtrWillBePersistent<RemoteFrame>
m_remoteFrame;

Add a comment about how the cycle between RemoteFrame and RemoteFrameView is
handled.

https://codereview.chromium.org/517043003/diff/440001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.cpp:246: // is finalized either.) 
Hence, a stopped parser is the strongest

currently => in non-oilpan builds

https://codereview.chromium.org/517043003/diff/440001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/page/Page.c...
Source/core/page/Page.cpp:642: m_mainFrame = 0;

Nit: m_mainFrame = nullptr

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
File Source/core/plugins/DOMMimeType.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
Source/core/plugins/DOMMimeType.h:52: virtual void trace(Visitor*) OVERRIDE;

Nit: This is just dispatching a trace method to a parent interface. I don't
think we need this.

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
File Source/core/plugins/DOMPlugin.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
Source/core/plugins/DOMPlugin.h:56: virtual void trace(Visitor*) OVERRIDE;

Nit: We won't need this.

https://codereview.chromium.org/517043003/diff/440001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.cpp:62: if (!m_navigator.frame() ||
!m_navigator.frame()->client())

Now I understand why you need the client check.

Are you sure that this is the only place where we need to add the client check?

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

https://codereview.chromium.org/517043003/diff/440001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:233: void WebFrame::trace(Visitor* visitor, WebFrame*
frame)

trace => traceChildren ?

We use "trace" only for pure trace methods. Customized trace methods are named
"traceAfterDispatch" etc.

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

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:86: dispose();

What's a benefit of introducing the dispose() method?

Nit: Actually I'm not really a fan of naming a method called inside a destructor
"dispose". "dispose" and "detach" should be called before a destructor gets
called.

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:92: disconnectOwnerElement();

Who calls this disconnectOwnerElement() in oilpan builds?

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:547: reset();

Sorry for asking a similar question (c.f.,
https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...):

Why is it OK not to call reset() in the weak processing of
LocalDOMWindow::m_frame? In non-oilpan reset() is called when the Frame is
destructed. I guess that in oilpan reset() needs to be called as well when the
Frame is weak-processed.

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (left):

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:120: setDOMWindow(nullptr);

Why did you remove this from non-oilpan builds?

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:148: #if !ENABLE(OILPAN)

Merge this #if with the #if above.

https://codereview.chromium.org/517043003/diff/560001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:167: ASSERT(!m_contentFrame);

Just help me understand: How is it guaranteed that m_contentFrame is cleared
before the HTMLFrameOwnerElement gets destructed?

https://codereview.chromium.org/517043003/diff/560001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.cpp:245: // and detached. (The will
not currently happen when the LocalFrame

The => That

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

https://codereview.chromium.org/517043003/diff/560001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:265: // is of no particular importance.

I guess the above two FIXMEs need to be addressed.

https://codereview.chromium.org/517043003/diff/560001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:270: if (FrameView* view = m_frame->view())

Add a comment about why it's safe to touch m_frame->view().

https://codereview.chromium.org/517043003/diff/560001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.cpp:65: void
ScopedPageLoadDeferrer::detach()

Shall we rename this to 'dispose'?

Then you can write WebViewImpl.cpp like:

  deferrer->dispose();
  #if ENABLE(OILPAN)
  delete deferrer;
  #endif

https://codereview.chromium.org/517043003/diff/560001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.cpp:89: detach();

Then you can remove detach() from the destructor. Alternately you can add:

  ASSERT(!m_deferredFrames.size());

https://codereview.chromium.org/517043003/diff/560001/Source/web/PopupContain...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/PopupContain...
Source/web/PopupContainer.cpp:257: // (In a non-Oilpan setting, the object
owning the PopupMenu and

Just help me understand: What's "the object"?

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.h:206: // when detaching.

This comment is a bit misleading. Instead of clearing this Frame reference,
we're explicitly destructing the Widget object earlier than the Frame gets
destructed.

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:474: ASSERT(!(m_localMainFrame &&
m_remoteMainFrame));

It looks a bit strange that (either of) the persistent handle remains until the
WebViewImpl object is destructed.

Don't we need to clear the persistent handle when Web*FrameImpl::close() is
called?

[dcheng, 2014-09-11 17:03:10]

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl....
Source/web/WebViewImpl.h:615: Persistent<WebRemoteFrameImpl> m_remoteMainFrame;
Page also has a main frame field. It feels redundant to maintain two of them.
Why can't we just make Page's m_mainFrame Persistent?

[sof, 2014-09-12 10:06:22]

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl....
Source/web/WebViewImpl.h:615: Persistent<WebRemoteFrameImpl> m_remoteMainFrame;
On 2014/09/11 17:03:10, dcheng (OOO) wrote:
> Page also has a main frame field. It feels redundant to maintain two of them.
> Why can't we just make Page's m_mainFrame Persistent?

This is about retaining the Web{Local,Remote}FrameImpl GCed objects, Page keeps
a Frame. Something close to the web view will have to keep those alive, I think.

But how to represent Page::m_mainFrame in Oilpan is something that needs to be
tidied up.

[sof, 2014-09-12 12:26:08]

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

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:86: dispose();
On 2014/09/11 14:47:25, haraken wrote:
> 
> What's a benefit of introducing the dispose() method?
> 
> Nit: Actually I'm not really a fan of naming a method called inside a
destructor
> "dispose". "dispose" and "detach" should be called before a destructor gets
> called.

There's no inherent value in keeping this helper, as it has now turned out to be
called always from this destructor (same for LocalFrame::dispose().)

Inlined away.

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:92: disconnectOwnerElement();
On 2014/09/11 14:47:25, haraken wrote:
> 
> Who calls this disconnectOwnerElement() in oilpan builds?

Done when the FrameOwner detaches/disconnects. I think it is guaranteed that
local FrameOwners will always do this. That leaves the non-Local ones - will
have to investigate if some of disconnectOwnerElement() needs to be handled in
other ways for those.

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (left):

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:120: setDOMWindow(nullptr);
On 2014/09/11 14:47:25, haraken wrote:
> 
> Why did you remove this from non-oilpan builds?

Added back.

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:148: #if !ENABLE(OILPAN)
On 2014/09/11 14:47:25, haraken wrote:
> 
> Merge this #if with the #if above.

Done as part of inlining dispose() into the dtor.

https://codereview.chromium.org/517043003/diff/560001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:167: ASSERT(!m_contentFrame);
On 2014/09/11 14:47:25, haraken wrote:
> 
> Just help me understand: How is it guaranteed that m_contentFrame is cleared
> before the HTMLFrameOwnerElement gets destructed?

I don't fully understand the detachment guarantees to offer you a reliable
answer -- i.e., how it is guaranteed that this (local) FrameOwner is
disconnected/detached first. This isn't specific to Oilpan, I'm quite sure, but
kept the assert as such.

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:474: ASSERT(!(m_localMainFrame &&
m_remoteMainFrame));
On 2014/09/11 14:47:26, haraken wrote:
> 
> It looks a bit strange that (either of) the persistent handle remains until
the
> WebViewImpl object is destructed.
> 
> Don't we need to clear the persistent handle when Web*FrameImpl::close() is
> called?

I don't think it is required, as the lifetimes of a (main) Web*FrameImpl and its
WebViewImpl wouldn't be substantially different.

I've now added an unregistration step to do this - preferable?

[sof, 2014-09-12 12:46:12]

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
File Source/core/plugins/DOMMimeType.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
Source/core/plugins/DOMMimeType.h:52: virtual void trace(Visitor*) OVERRIDE;
On 2014/09/11 14:47:25, haraken wrote:
> 
> Nit: This is just dispatching a trace method to a parent interface. I don't
> think we need this.

And the presence of a virtual dtor ensures that the leftmost class has a vtable,
but declaring a mixin without a trace()... do we want to do that?

[sof, 2014-09-12 12:55:19]

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:509: reset();
On 2014/09/11 14:47:25, haraken wrote:
> 
> Not related to this CL, why are we calling reset() after m_hasBeenReset
becomes
> true?

You can destroy the window's frame, which resets, but then access one of the
supplement properties via a script in another frame, which will re-create an
object needing to be cleared/reset again here. (cf the various
<foo>-detached-crash.html tests :-) )

I don't think it is worth accurately updating m_hasBeenReset in the face of such
actions, but reasonable minds might feel differently about it.

[sof, 2014-09-12 13:16:56]

(I'm thinking/hoping that following up on these rather important issues one by
one will make them easier to digest for reviewers and Cc:ers alike, rather than
in a long list. If it is just getting in the way, please do say.)

https://codereview.chromium.org/517043003/diff/440001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.cpp:62: if (!m_navigator.frame() ||
!m_navigator.frame()->client())
On 2014/09/11 14:47:25, haraken wrote:
> 
> Now I understand why you need the client check.
> 
> Are you sure that this is the only place where we need to add the client
check?

I am not confident and it is a slight worry; I don't know of a systematic way to
determine where we might access a now-missing frame host with Oilpan.

This is of course limited to "detached" uses of frame supplements (and
DOMWindowProperties.)

[sof, 2014-09-12 13:44:02]

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

https://codereview.chromium.org/517043003/diff/560001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:265: // is of no particular importance.
On 2014/09/11 14:47:26, haraken wrote:
> 
> I guess the above two FIXMEs need to be addressed.

I've looked into the 2nd quite a bit, how much of
FrameSelection::prepareForDestruction() is needed during frame finalization, but
it'd be good to have the following confirmed by someone closer to rendering
details -- i.e.,

 Is it acceptable to _not_ clear the frame document RenderView's selection when
the frame (and document) is being finalized?

That is the only FrameSelection::prepareForDestruction() action I'm concerned of
leaving out during finalization. Naively so, perhaps.

[sof, 2014-09-12 14:47:40]

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:419: // 'emulates' the RefPtr-cycle that is kept
between the two objects non-Oilpan.
On 2014/09/11 14:47:25, haraken wrote:
> 
> Would you add a comment about how the ref-cycle gets broken, as dcheng
> explained?

Done (and thanks for the original explanation!)

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrameView.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/frame/Remot...
Source/core/frame/RemoteFrameView.h:36: RefPtrWillBePersistent<RemoteFrame>
m_remoteFrame;
On 2014/09/11 14:47:25, haraken wrote:
> 
> Add a comment about how the cycle between RemoteFrame and RemoteFrameView is
> handled.

Done, cross-referencing the FrameView::m_frame comment.

We really need the FrameViews on the heap though.

https://codereview.chromium.org/517043003/diff/440001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.cpp:246: // is finalized either.) 
Hence, a stopped parser is the strongest
On 2014/09/11 14:47:25, haraken wrote:
> 
> currently => in non-oilpan builds

Adjusted the comment a bit. If we need to trigger parser detachment earlier
somehow is still unresolved, btw.

https://codereview.chromium.org/517043003/diff/440001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/page/Page.c...
Source/core/page/Page.cpp:642: m_mainFrame = 0;
On 2014/09/11 14:47:25, haraken wrote:
> 
> Nit: m_mainFrame = nullptr

Done.

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

https://codereview.chromium.org/517043003/diff/440001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:233: void WebFrame::trace(Visitor* visitor, WebFrame*
frame)
On 2014/09/11 14:47:25, haraken wrote:
> 
> trace => traceChildren ?
> 
> We use "trace" only for pure trace methods. Customized trace methods are named
> "traceAfterDispatch" etc.

Good name for it, switched.

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

https://codereview.chromium.org/517043003/diff/560001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:270: if (FrameView* view = m_frame->view())
On 2014/09/11 14:47:26, haraken wrote:
> 
> Add a comment about why it's safe to touch m_frame->view().

Done.

[sof, 2014-09-12 17:08:40]

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:547: reset();
On 2014/09/11 14:47:25, haraken wrote:
> 
> Sorry for asking a similar question (c.f.,
>
https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...):
> 
> Why is it OK not to call reset() in the weak processing of
> LocalDOMWindow::m_frame? In non-oilpan reset() is called when the Frame is
> destructed. I guess that in oilpan reset() needs to be called as well when the
> Frame is weak-processed.
> 

I think calling reset() at all here is questionable, non-Oilpan :-)

i.e., detaching a window from its frame shouldn't remove the dependent objects
(e.g., window.location) and their wrappers. If try accessing window.location
(say) afterwards, a new object will be created, but with a different wrapper.

https://codereview.chromium.org/565243002/ is what I suggest we do for
non-Oilpan instead. By doing so, there would be no extra reset() weak processing
step needed here. wdyt?

[sof, 2014-09-14 06:48:49]

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:547: reset();
On 2014/09/12 17:08:40, sof wrote:
> On 2014/09/11 14:47:25, haraken wrote:
> > 
> > Sorry for asking a similar question (c.f.,
> >
>
https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...):
> > 
> > Why is it OK not to call reset() in the weak processing of
> > LocalDOMWindow::m_frame? In non-oilpan reset() is called when the Frame is
> > destructed. I guess that in oilpan reset() needs to be called as well when
the
> > Frame is weak-processed.
> > 
> 
> I think calling reset() at all here is questionable, non-Oilpan :-)
> 
> i.e., detaching a window from its frame shouldn't remove the dependent objects
> (e.g., window.location) and their wrappers. If try accessing window.location
> (say) afterwards, a new object will be created, but with a different wrapper.
> 
> https://codereview.chromium.org/565243002/ is what I suggest we do for
> non-Oilpan instead. By doing so, there would be no extra reset() weak
processing
> step needed here. wdyt?

There's actually no need to do any resetting here, as the frame will alreadly
have done as part of clearing its window reference during finalization.

Removed the call, but left an assert.

[haraken, 2014-09-14 13:21:02]

(Sorry about the review delay. I'm now a little sick and might take a couple of
days to take a look at this.)

[zerny-chromium, 2014-09-15 06:33:03]

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
File Source/core/plugins/DOMMimeType.h (right):

https://codereview.chromium.org/517043003/diff/440001/Source/core/plugins/DOM...
Source/core/plugins/DOMMimeType.h:52: virtual void trace(Visitor*) OVERRIDE;
On 2014/09/12 12:46:12, sof wrote:
> On 2014/09/11 14:47:25, haraken wrote:
> > 
> > Nit: This is just dispatching a trace method to a parent interface. I don't
> > think we need this.
> 
> And the presence of a virtual dtor ensures that the leftmost class has a
vtable,
> but declaring a mixin without a trace()... do we want to do that?

We need to keep the trace here because we want all virtual trace methods to be
located in the "top/left-most" vtbl. Removing this trace definition should
result in a plugin-reported error (for the ToT plugin), but I think there is an
implementation error in the plugin here because the trace method then becomes
inherited.

I'll look at adding some HeapTests and plugin tests to verify the behavior of
this case.

[sof, 2014-09-15 16:46:42]

https://codereview.chromium.org/517043003/diff/560001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.cpp:89: detach();
On 2014/09/11 14:47:26, haraken wrote:
> 
> Then you can remove detach() from the destructor. Alternately you can add:
> 
>   ASSERT(!m_deferredFrames.size());

No, that won't work -- ScopedPageLoadDeferrer may also be stack allocated.

Please, let's not micro-optimize #if use here, it seems less important (iow,
I've spent quite a bit of time getting this wrong, then right(er) already :) )

https://codereview.chromium.org/517043003/diff/560001/Source/web/PopupContain...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/PopupContain...
Source/web/PopupContainer.cpp:257: // (In a non-Oilpan setting, the object
owning the PopupMenu and
On 2014/09/11 14:47:26, haraken wrote:
> 
> Just help me understand: What's "the object"?

A RenderMenuList would be what's controller/owner of this implementation of a
popup menu. Tried to add some more details & clarity to this comment.

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/517043003/diff/560001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.h:206: // when detaching.
On 2014/09/11 14:47:26, haraken wrote:
> 
> This comment is a bit misleading. Instead of clearing this Frame reference,
> we're explicitly destructing the Widget object earlier than the Frame gets
> destructed.

Thanks, misleading it is -- it describes the initial approach, and hadn't been
updated. Done now.

[sof, 2014-09-16 09:08:36]

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:142: setView(nullptr);
On 2014/09/07 22:26:34, dcheng (OOO) wrote:
> Similarly, is it safe to omit this call to setView(nullptr)? This does do a
> non-trivial amount of work (in particular, it will detach an active document).

I now believe it is safe, except that it would be tidier to detach the FrameView
for the Oilpan code path. Updated it do so, along with adding (yet) more
comments.

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

https://codereview.chromium.org/517043003/diff/560001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:265: // is of no particular importance.
On 2014/09/12 13:44:02, sof wrote:
> On 2014/09/11 14:47:26, haraken wrote:
> > 
> > I guess the above two FIXMEs need to be addressed.
> 
> I've looked into the 2nd quite a bit, how much of
> FrameSelection::prepareForDestruction() is needed during frame finalization,
but
> it'd be good to have the following confirmed by someone closer to rendering
> details -- i.e.,
> 
>  Is it acceptable to _not_ clear the frame document RenderView's selection
when
> the frame (and document) is being finalized?
> 
> That is the only FrameSelection::prepareForDestruction() action I'm concerned
of
> leaving out during finalization. Naively so, perhaps.

Gone through the details of what we're not doing in case of Oilpan during frame
finalization, and updated the comment to fit.

One issue remaining is that the effects of Document::detach() will not be
performed.

(I don't propose we leave this amount of comments around in the end, but added
Oilpan specific play-by-play comments to hopefully clarify a bit.)

[Mads Ager (chromium), 2014-09-16 12:17:45]

Lots of stuff going on here. This looks great!

There are a bunch of FIXMEs in FrameLoader and it is not clear to me if those
are under control?

Also, below, I have a question about the LocalFrame destruction sequence in
connection with dispatching Document prepareForDestruction.

The rest of these comments are just nits (and I'm sorry that my comments are
spread over two patch sets, I started before the latest patch set and somehow
ended up reviewing the other half in the new patch set).

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
File LayoutTests/fast/dom/Window/orphaned-frame-access.html (right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/orphaned-frame-access.html:18: gc();
If you src js-test.js instead you get asyncGC that you can use instead of gc()
followed by setTimeout:

asyncGC(function() {
  // A full precise GC will have happened here.
});

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
File
LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html
(right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html:38:
gc();
We should use asyncGC here to make sure that the garbage collection is precise.
Otherwise, we can get flakiness if a value on the stack is interpreted as a
pointer to the frame.

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
File
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
(right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html:31:
gc();
Let's do asynGC here as well to make sure that we get a precise GC before
verifying.

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/events...
File LayoutTests/fast/events/message-port-gc-closed.html (right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/events...
LayoutTests/fast/events/message-port-gc-closed.html:10: gc();
Now that we are touching this test we should make this asyncGC to make sure that
we get a precise GC at the event loop. Otherwise the test can flake because of
values on the stack being treated as a pointer to the frame.

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/events...
LayoutTests/fast/events/message-port-gc-closed.html:15: // The iframe is
reachable via evt.source, and evt is on the
asyncGC above should solve this issue reliably by using a precise GC at the
event loop.

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.h (right):

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:53: RawPtrWillBeMember<LocalDOMWindow>
m_associatedDOMWindow;
It looks like the m_associatedDOMWindow is only ever used to unregister the
DOMWindowProperty again when it dies. With Oilpan, that is not necessary. Do we
need the pointer to the associated DOMWindow at all here in the oilpan build?

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/Frame...
File Source/core/frame/FrameDestructionObserver.h (right):

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/Frame...
Source/core/frame/FrameDestructionObserver.h:40: // Oilpan: Frame finalization
can now only be observed via
The alternative is to register custom weak processing here. In the callback
check if m_frame is dead. If m_frame is dead clear out m_frame and call
frameDestroyed. However, it seems that this changelist actually gets rid of all
need for the frameDestroyed notification?

Maybe we can get rid of the comment (or update it to state that the notification
can be reintroduced if needed)? If we don't need the notification, great. If we
do we can easily add it back using a custom weak callback.

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:422: // FrameLoader::detachFromParent(), it then
clearing its
clearing -> clears

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:509: ASSERT(m_hasBeenReset);
This looks a little odd (it is not new though). We assert that the
LocalDOMWindow has been reset and then we reset it again. Isn't the reset call
redundant here?

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:548: // The frame resets its LocalDOMWindow
prior to frame destruction
So, really, we could remove the frameDestroyed override here in the non-oilpan
build as well. But maybe the assert is worth it here? The assert is in the
destructor which is maybe enough?

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:202: if (!view)
Why this extra check compared to before?

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:210: document()->prepareForDestruction();
Are we losing this with Oilpan on destruction because we are not doing a
setView(0). If so, is that OK? Looks like that will disable child frame
disconnection. I haven't digged too deeply in to what that means, but it looks
like a lot of stuff is going on there that we need to double check?

https://codereview.chromium.org/517043003/diff/660001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:154: // As yet unknown if we can "get
away" with releasing it slightly earlier..
How do we resolve this one? No tests failing I assume? Once we are happy with
the rest we should find the right person to ask about this.

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2084: if (LocalFrame* innerFrame =
targetedEvent.hitTestResult().innerNodeFrame()) {
Left-over edit from debugging? :)

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.cpp:89: detach();
Maybe wrap in #if !ENABLE(OILPAN) to make it clear that the destructor will be
empty with oilpan. With Oilpan, dispose will always be called instead, right?

[sof, 2014-09-16 12:23:23]

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:1225: // transition to 'true' slightly
earlier.
On 2014/09/08 21:17:45, sof wrote:
> On 2014/09/08 07:25:58, haraken wrote:
> > 
> > If possible, we should make this behavior change before landing this CL. It
> > would be better if we could use the explicit close() call in non-oilpan
builds
> > as well and avoid introducing different call paths.
> > 
> > I believe the new behavior is a correct one.
> 
> It wouldn't be a problem to separate it out and land it first, making
non-Oilpan
> and Oilpan behave the same.
> 
> I'm slightly nervous that it might break some old&ingrained assumptions in
code
> using window.closed though. (no, I don't know what code that might be! :) )

Handling the change to window.closed via http://crbug.com/414658 (
https://codereview.chromium.org/538323003/ )

[sof, 2014-09-17 09:42:58]

Thanks for the intrepid reviewing. :)

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
File LayoutTests/fast/dom/Window/orphaned-frame-access.html (right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/orphaned-frame-access.html:18: gc();
On 2014/09/16 12:17:44, Mads Ager (chromium) wrote:
> If you src js-test.js instead you get asyncGC that you can use instead of gc()
> followed by setTimeout:
> 
> asyncGC(function() {
>   // A full precise GC will have happened here.
> });

Refreshed the test to use asyncGC()

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
File
LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html
(right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html:38:
gc();
On 2014/09/16 12:17:44, Mads Ager (chromium) wrote:
> We should use asyncGC here to make sure that the garbage collection is
precise.
> Otherwise, we can get flakiness if a value on the stack is interpreted as a
> pointer to the frame.

Done.

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
File
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
(right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html:31:
gc();
On 2014/09/16 12:17:44, Mads Ager (chromium) wrote:
> Let's do asynGC here as well to make sure that we get a precise GC before
> verifying.

Done.

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/events...
File LayoutTests/fast/events/message-port-gc-closed.html (right):

https://codereview.chromium.org/517043003/diff/640001/LayoutTests/fast/events...
LayoutTests/fast/events/message-port-gc-closed.html:15: // The iframe is
reachable via evt.source, and evt is on the
On 2014/09/16 12:17:44, Mads Ager (chromium) wrote:
> asyncGC above should solve this issue reliably by using a precise GC at the
> event loop.

Thanks, much tidier that way; done.

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.h (right):

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:53: RawPtrWillBeMember<LocalDOMWindow>
m_associatedDOMWindow;
On 2014/09/16 12:17:44, Mads Ager (chromium) wrote:
> It looks like the m_associatedDOMWindow is only ever used to unregister the
> DOMWindowProperty again when it dies. With Oilpan, that is not necessary. Do
we
> need the pointer to the associated DOMWindow at all here in the oilpan build?

Quite right, it serves no purpose with Oilpan; great! Simplified away.

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/Frame...
File Source/core/frame/FrameDestructionObserver.h (right):

https://codereview.chromium.org/517043003/diff/640001/Source/core/frame/Frame...
Source/core/frame/FrameDestructionObserver.h:40: // Oilpan: Frame finalization
can now only be observed via
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> The alternative is to register custom weak processing here. In the callback
> check if m_frame is dead. If m_frame is dead clear out m_frame and call
> frameDestroyed. However, it seems that this changelist actually gets rid of
all
> need for the frameDestroyed notification?
> 
> Maybe we can get rid of the comment (or update it to state that the
notification
> can be reintroduced if needed)? If we don't need the notification, great. If
we
> do we can easily add it back using a custom weak callback.

The intent of this comment was to offer up a solution for any future needs for
this notification -- there's currently no custom processing that needs to happen
when m_frame is cleared with Oilpan.

Updated/narrowed down the comment, along the lines suggested.

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:422: // FrameLoader::detachFromParent(), it then
clearing its
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> clearing -> clears

Done.

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:509: ASSERT(m_hasBeenReset);
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> This looks a little odd (it is not new though). We assert that the
> LocalDOMWindow has been reset and then we reset it again. Isn't the reset call
> redundant here?

See https://codereview.chromium.org/517043003/#msg36

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:548: // The frame resets its LocalDOMWindow
prior to frame destruction
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> So, really, we could remove the frameDestroyed override here in the non-oilpan
> build as well. But maybe the assert is worth it here? The assert is in the
> destructor which is maybe enough?

Hmm, it doesn't add much value -- if frame destruction should change the order
in which it issues frameDestroyed() vs clearing its LocalDOMWindow, that isn't a
problem. If it fails to do the latter, then that will cause badness regardless.

Let's remove the override; nice catch (I failed to see the potential
simplification while removing the reset() call.) I kept frameDestroyed() as a
virtual.

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:202: if (!view)
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> Why this extra check compared to before?

It is not needed really, just pointlessly optimizing call patterns. Retired.

https://codereview.chromium.org/517043003/diff/660001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:210: document()->prepareForDestruction();
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> Are we losing this with Oilpan on destruction because we are not doing a
> setView(0). If so, is that OK? Looks like that will disable child frame
> disconnection. I haven't digged too deeply in to what that means, but it looks
> like a lot of stuff is going on there that we need to double check?

The issue of signalling document destruction is definitely an issue. But let's
deal with this one via FrameLoader::dispose() -- this call is redundant during
frame finalization, as it is repeated in that dispose().

https://codereview.chromium.org/517043003/diff/660001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:154: // As yet unknown if we can "get
away" with releasing it slightly earlier..
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> How do we resolve this one? No tests failing I assume? Once we are happy with
> the rest we should find the right person to ask about this.

Yes, destruction shouldn't be far off regardless. No plugins/ tests, such as
they are, are suffering as a result.

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2084: if (LocalFrame* innerFrame =
targetedEvent.hitTestResult().innerNodeFrame()) {
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> Left-over edit from debugging? :)

Spot on :)

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/517043003/diff/660001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.cpp:89: detach();
On 2014/09/16 12:17:45, Mads Ager (chromium) wrote:
> Maybe wrap in #if !ENABLE(OILPAN) to make it clear that the destructor will be
> empty with oilpan. With Oilpan, dispose will always be called instead, right?

No, ScopedPageLoadDeferrer is also stack allocated in various places.

[dcheng, 2014-09-17 10:58:09]

https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:505: if (m_frame && m_frame->isMainFrame() &&
viewImpl())
I'm a little concerned about how it's much harder to reason about memory
correctness in the Oilpan version of this code.

Before, the ownership model was very simple--Source/web internally holds a ref
on behalf of the embedder, which it drops once close() is called. The embedder
knows that as long as it doesn't call close(), the WebFrame remains live, no
matter what it does.

With Oilpan, the only reference the Blink API holds on behalf of the embedder is
for the main frame. As part of frame detach, the embedder (RenderFrameImpl)
removes child WebFrames from the frame tree. Today, the code for
RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:

1798  if (is_subframe)
1799    frame->parent()->removeChild(frame);
1800
1801  // |frame| is invalid after here.
1802  frame->close();

Imagine that |frame| wasn't passed as a parameter to this method, and it was
using |frame_| instead (something that's likely to happen in the future). After
the call to removeChild(), I don't think there would actually be any references
to keep this child WebFrame alive. While this isn't technically an issue
(because I don't think a GC can run between line 1799 and 1802), it seems
conceptually wrong.

[sof, 2014-09-17 11:04:11]

On 2014/09/17 10:58:09, dcheng (OOO) wrote:
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> File Source/web/WebLocalFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> Source/web/WebLocalFrameImpl.cpp:505: if (m_frame && m_frame->isMainFrame() &&
> viewImpl())
> I'm a little concerned about how it's much harder to reason about memory
> correctness in the Oilpan version of this code.
> 
> Before, the ownership model was very simple--Source/web internally holds a ref
> on behalf of the embedder, which it drops once close() is called. The embedder
> knows that as long as it doesn't call close(), the WebFrame remains live, no
> matter what it does.
> 
> With Oilpan, the only reference the Blink API holds on behalf of the embedder
is
> for the main frame. As part of frame detach, the embedder (RenderFrameImpl)
> removes child WebFrames from the frame tree. Today, the code for
> RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> 
> 1798  if (is_subframe)
> 1799    frame->parent()->removeChild(frame);
> 1800
> 1801  // |frame| is invalid after here.
> 1802  frame->close();
> 
> Imagine that |frame| wasn't passed as a parameter to this method, and it was
> using |frame_| instead (something that's likely to happen in the future).
After
> the call to removeChild(), I don't think there would actually be any
references
> to keep this child WebFrame alive. While this isn't technically an issue
> (because I don't think a GC can run between line 1799 and 1802), it seems
> conceptually wrong.

'frame' would be on the stack, so a GC that scans the stack between those two
points shouldn't pose a problem.

[dcheng, 2014-09-17 11:06:50]

On 2014/09/17 at 11:04:11, sigbjornf wrote:
> On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > File Source/web/WebLocalFrameImpl.cpp (right):
> > 
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame && m_frame->isMainFrame()
&&
> > viewImpl())
> > I'm a little concerned about how it's much harder to reason about memory
> > correctness in the Oilpan version of this code.
> > 
> > Before, the ownership model was very simple--Source/web internally holds a
ref
> > on behalf of the embedder, which it drops once close() is called. The
embedder
> > knows that as long as it doesn't call close(), the WebFrame remains live, no
> > matter what it does.
> > 
> > With Oilpan, the only reference the Blink API holds on behalf of the
embedder is
> > for the main frame. As part of frame detach, the embedder (RenderFrameImpl)
> > removes child WebFrames from the frame tree. Today, the code for
> > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > 
> > 1798  if (is_subframe)
> > 1799    frame->parent()->removeChild(frame);
> > 1800
> > 1801  // |frame| is invalid after here.
> > 1802  frame->close();
> > 
> > Imagine that |frame| wasn't passed as a parameter to this method, and it was
> > using |frame_| instead (something that's likely to happen in the future).
After
> > the call to removeChild(), I don't think there would actually be any
references
> > to keep this child WebFrame alive. While this isn't technically an issue
> > (because I don't think a GC can run between line 1799 and 1802), it seems
> > conceptually wrong.
> 
> 'frame' would be on the stack, so a GC that scans the stack between those two
points shouldn't pose a problem.

It's a problem because:
1) It might not be on the stack in the future--it's a parameter that's probably
going to get removed at some point.
2) I don't think the embedder should have to know about Oilpan conventions (it
might only be held in a member, and the embedder shouldn't have to spill it to
the stack to keep it alive).

[Mads Ager (chromium), 2014-09-17 11:07:59]

On 2014/09/17 10:58:09, dcheng (OOO) wrote:
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> File Source/web/WebLocalFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> Source/web/WebLocalFrameImpl.cpp:505: if (m_frame && m_frame->isMainFrame() &&
> viewImpl())
> I'm a little concerned about how it's much harder to reason about memory
> correctness in the Oilpan version of this code.
> 
> Before, the ownership model was very simple--Source/web internally holds a ref
> on behalf of the embedder, which it drops once close() is called. The embedder
> knows that as long as it doesn't call close(), the WebFrame remains live, no
> matter what it does.
> 
> With Oilpan, the only reference the Blink API holds on behalf of the embedder
is
> for the main frame. As part of frame detach, the embedder (RenderFrameImpl)
> removes child WebFrames from the frame tree. Today, the code for
> RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> 
> 1798  if (is_subframe)
> 1799    frame->parent()->removeChild(frame);
> 1800
> 1801  // |frame| is invalid after here.
> 1802  frame->close();
> 
> Imagine that |frame| wasn't passed as a parameter to this method, and it was
> using |frame_| instead (something that's likely to happen in the future).
After
> the call to removeChild(), I don't think there would actually be any
references
> to keep this child WebFrame alive. While this isn't technically an issue
> (because I don't think a GC can run between line 1799 and 1802), it seems
> conceptually wrong.

That is not a problem. If you get a GC the stack (and registers) are
conservatively scanned and we will find a pointer to frame and it will be kept
alive. So, there are references that keep the frame alive until you are done
with it.

[Mads Ager (chromium), 2014-09-17 11:14:44]

On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
m_frame->isMainFrame()
> &&
> > > viewImpl())
> > > I'm a little concerned about how it's much harder to reason about memory
> > > correctness in the Oilpan version of this code.
> > > 
> > > Before, the ownership model was very simple--Source/web internally holds a
> ref
> > > on behalf of the embedder, which it drops once close() is called. The
> embedder
> > > knows that as long as it doesn't call close(), the WebFrame remains live,
no
> > > matter what it does.
> > > 
> > > With Oilpan, the only reference the Blink API holds on behalf of the
> embedder is
> > > for the main frame. As part of frame detach, the embedder
(RenderFrameImpl)
> > > removes child WebFrames from the frame tree. Today, the code for
> > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > 
> > > 1798  if (is_subframe)
> > > 1799    frame->parent()->removeChild(frame);
> > > 1800
> > > 1801  // |frame| is invalid after here.
> > > 1802  frame->close();
> > > 
> > > Imagine that |frame| wasn't passed as a parameter to this method, and it
was
> > > using |frame_| instead (something that's likely to happen in the future).
> After
> > > the call to removeChild(), I don't think there would actually be any
> references
> > > to keep this child WebFrame alive. While this isn't technically an issue
> > > (because I don't think a GC can run between line 1799 and 1802), it seems
> > > conceptually wrong.
> > 
> > 'frame' would be on the stack, so a GC that scans the stack between those
two
> points shouldn't pose a problem.
> 
> It's a problem because:
> 1) It might not be on the stack in the future--it's a parameter that's
probably
> going to get removed at some point.
> 2) I don't think the embedder should have to know about Oilpan conventions (it
> might only be held in a member, and the embedder shouldn't have to spill it to
> the stack to keep it alive).

As long as a WebFrame exists that has a persistent handle registered with the
oilpan heap infrastructure so until the WebFrame is deallocated the frame will
be alive. The embedder doesn't have to worry about oilpan. The embedding layer
takes care of that. Any WebXYZ object keeps the underlying blink object alive.

[dcheng, 2014-09-17 11:17:06]

On 2014/09/17 at 11:14:44, ager wrote:
> On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
m_frame->isMainFrame()
> > &&
> > > > viewImpl())
> > > > I'm a little concerned about how it's much harder to reason about memory
> > > > correctness in the Oilpan version of this code.
> > > > 
> > > > Before, the ownership model was very simple--Source/web internally holds
a
> > ref
> > > > on behalf of the embedder, which it drops once close() is called. The
> > embedder
> > > > knows that as long as it doesn't call close(), the WebFrame remains
live, no
> > > > matter what it does.
> > > > 
> > > > With Oilpan, the only reference the Blink API holds on behalf of the
> > embedder is
> > > > for the main frame. As part of frame detach, the embedder
(RenderFrameImpl)
> > > > removes child WebFrames from the frame tree. Today, the code for
> > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > > 
> > > > 1798  if (is_subframe)
> > > > 1799    frame->parent()->removeChild(frame);
> > > > 1800
> > > > 1801  // |frame| is invalid after here.
> > > > 1802  frame->close();
> > > > 
> > > > Imagine that |frame| wasn't passed as a parameter to this method, and it
was
> > > > using |frame_| instead (something that's likely to happen in the
future).
> > After
> > > > the call to removeChild(), I don't think there would actually be any
> > references
> > > > to keep this child WebFrame alive. While this isn't technically an issue
> > > > (because I don't think a GC can run between line 1799 and 1802), it
seems
> > > > conceptually wrong.
> > > 
> > > 'frame' would be on the stack, so a GC that scans the stack between those
two
> > points shouldn't pose a problem.
> > 
> > It's a problem because:
> > 1) It might not be on the stack in the future--it's a parameter that's
probably
> > going to get removed at some point.
> > 2) I don't think the embedder should have to know about Oilpan conventions
(it
> > might only be held in a member, and the embedder shouldn't have to spill it
to
> > the stack to keep it alive).
> 
> As long as a WebFrame exists that has a persistent handle registered with the
oilpan heap infrastructure so until the WebFrame is deallocated the frame will
be alive. The embedder doesn't have to worry about oilpan. The embedding layer
takes care of that. Any WebXYZ object keeps the underlying blink object alive.

Right, I'm saying that after the call to removeChild(), I don't believe there
will be any such handles registered with Oilpan.

[Mads Ager (chromium), 2014-09-17 11:20:07]

On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> On 2014/09/17 at 11:14:44, ager wrote:
> > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> m_frame->isMainFrame()
> > > &&
> > > > > viewImpl())
> > > > > I'm a little concerned about how it's much harder to reason about
memory
> > > > > correctness in the Oilpan version of this code.
> > > > > 
> > > > > Before, the ownership model was very simple--Source/web internally
holds
> a
> > > ref
> > > > > on behalf of the embedder, which it drops once close() is called. The
> > > embedder
> > > > > knows that as long as it doesn't call close(), the WebFrame remains
> live, no
> > > > > matter what it does.
> > > > > 
> > > > > With Oilpan, the only reference the Blink API holds on behalf of the
> > > embedder is
> > > > > for the main frame. As part of frame detach, the embedder
> (RenderFrameImpl)
> > > > > removes child WebFrames from the frame tree. Today, the code for
> > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > > > 
> > > > > 1798  if (is_subframe)
> > > > > 1799    frame->parent()->removeChild(frame);
> > > > > 1800
> > > > > 1801  // |frame| is invalid after here.
> > > > > 1802  frame->close();
> > > > > 
> > > > > Imagine that |frame| wasn't passed as a parameter to this method, and
it
> was
> > > > > using |frame_| instead (something that's likely to happen in the
> future).
> > > After
> > > > > the call to removeChild(), I don't think there would actually be any
> > > references
> > > > > to keep this child WebFrame alive. While this isn't technically an
issue
> > > > > (because I don't think a GC can run between line 1799 and 1802), it
> seems
> > > > > conceptually wrong.
> > > > 
> > > > 'frame' would be on the stack, so a GC that scans the stack between
those
> two
> > > points shouldn't pose a problem.
> > > 
> > > It's a problem because:
> > > 1) It might not be on the stack in the future--it's a parameter that's
> probably
> > > going to get removed at some point.
> > > 2) I don't think the embedder should have to know about Oilpan conventions
> (it
> > > might only be held in a member, and the embedder shouldn't have to spill
it
> to
> > > the stack to keep it alive).
> > 
> > As long as a WebFrame exists that has a persistent handle registered with
the
> oilpan heap infrastructure so until the WebFrame is deallocated the frame will
> be alive. The embedder doesn't have to worry about oilpan. The embedding layer
> takes care of that. Any WebXYZ object keeps the underlying blink object alive.
> 
> Right, I'm saying that after the call to removeChild(), I don't believe there
> will be any such handles registered with Oilpan.

In your example you are using frame_ again to call close() on it. So frame_
which is a WebFrame has not been deleted yet and therefore the underlying Frame
is kept alive. Once you deallocate the WebFrame and therefore do not touch the
Frame anymore it is free to die. That is as expected I would think?

[sof, 2014-09-17 11:22:21]

On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> On 2014/09/17 at 11:14:44, ager wrote:
> > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> m_frame->isMainFrame()
> > > &&
> > > > > viewImpl())
> > > > > I'm a little concerned about how it's much harder to reason about
memory
> > > > > correctness in the Oilpan version of this code.
> > > > > 
> > > > > Before, the ownership model was very simple--Source/web internally
holds
> a
> > > ref
> > > > > on behalf of the embedder, which it drops once close() is called. The
> > > embedder
> > > > > knows that as long as it doesn't call close(), the WebFrame remains
> live, no
> > > > > matter what it does.
> > > > > 
> > > > > With Oilpan, the only reference the Blink API holds on behalf of the
> > > embedder is
> > > > > for the main frame. As part of frame detach, the embedder
> (RenderFrameImpl)
> > > > > removes child WebFrames from the frame tree. Today, the code for
> > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > > > 
> > > > > 1798  if (is_subframe)
> > > > > 1799    frame->parent()->removeChild(frame);
> > > > > 1800
> > > > > 1801  // |frame| is invalid after here.
> > > > > 1802  frame->close();
> > > > > 
> > > > > Imagine that |frame| wasn't passed as a parameter to this method, and
it
> was
> > > > > using |frame_| instead (something that's likely to happen in the
> future).
> > > After
> > > > > the call to removeChild(), I don't think there would actually be any
> > > references
> > > > > to keep this child WebFrame alive. While this isn't technically an
issue
> > > > > (because I don't think a GC can run between line 1799 and 1802), it
> seems
> > > > > conceptually wrong.
> > > > 
> > > > 'frame' would be on the stack, so a GC that scans the stack between
those
> two
> > > points shouldn't pose a problem.
> > > 
> > > It's a problem because:
> > > 1) It might not be on the stack in the future--it's a parameter that's
> probably
> > > going to get removed at some point.
> > > 2) I don't think the embedder should have to know about Oilpan conventions
> (it
> > > might only be held in a member, and the embedder shouldn't have to spill
it
> to
> > > the stack to keep it alive).
> > 
> > As long as a WebFrame exists that has a persistent handle registered with
the
> oilpan heap infrastructure so until the WebFrame is deallocated the frame will
> be alive. The embedder doesn't have to worry about oilpan. The embedding layer
> takes care of that. Any WebXYZ object keeps the underlying blink object alive.
> 
> Right, I'm saying that after the call to removeChild(), I don't believe there
> will be any such handles registered with Oilpan.

The persistent reference is only cleared upon close() (cf.
WebLocalFrameImpl::close()) - does that reduce your concern?

[dcheng, 2014-09-17 11:23:16]

On 2014/09/17 at 11:20:07, ager wrote:
> On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> > On 2014/09/17 at 11:14:44, ager wrote:
> > > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> > m_frame->isMainFrame()
> > > > &&
> > > > > > viewImpl())
> > > > > > I'm a little concerned about how it's much harder to reason about
memory
> > > > > > correctness in the Oilpan version of this code.
> > > > > > 
> > > > > > Before, the ownership model was very simple--Source/web internally
holds
> > a
> > > > ref
> > > > > > on behalf of the embedder, which it drops once close() is called.
The
> > > > embedder
> > > > > > knows that as long as it doesn't call close(), the WebFrame remains
> > live, no
> > > > > > matter what it does.
> > > > > > 
> > > > > > With Oilpan, the only reference the Blink API holds on behalf of the
> > > > embedder is
> > > > > > for the main frame. As part of frame detach, the embedder
> > (RenderFrameImpl)
> > > > > > removes child WebFrames from the frame tree. Today, the code for
> > > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > > > > 
> > > > > > 1798  if (is_subframe)
> > > > > > 1799    frame->parent()->removeChild(frame);
> > > > > > 1800
> > > > > > 1801  // |frame| is invalid after here.
> > > > > > 1802  frame->close();
> > > > > > 
> > > > > > Imagine that |frame| wasn't passed as a parameter to this method,
and it
> > was
> > > > > > using |frame_| instead (something that's likely to happen in the
> > future).
> > > > After
> > > > > > the call to removeChild(), I don't think there would actually be any
> > > > references
> > > > > > to keep this child WebFrame alive. While this isn't technically an
issue
> > > > > > (because I don't think a GC can run between line 1799 and 1802), it
> > seems
> > > > > > conceptually wrong.
> > > > > 
> > > > > 'frame' would be on the stack, so a GC that scans the stack between
those
> > two
> > > > points shouldn't pose a problem.
> > > > 
> > > > It's a problem because:
> > > > 1) It might not be on the stack in the future--it's a parameter that's
> > probably
> > > > going to get removed at some point.
> > > > 2) I don't think the embedder should have to know about Oilpan
conventions
> > (it
> > > > might only be held in a member, and the embedder shouldn't have to spill
it
> > to
> > > > the stack to keep it alive).
> > > 
> > > As long as a WebFrame exists that has a persistent handle registered with
the
> > oilpan heap infrastructure so until the WebFrame is deallocated the frame
will
> > be alive. The embedder doesn't have to worry about oilpan. The embedding
layer
> > takes care of that. Any WebXYZ object keeps the underlying blink object
alive.
> > 
> > Right, I'm saying that after the call to removeChild(), I don't believe
there
> > will be any such handles registered with Oilpan.
> 
> In your example you are using frame_ again to call close() on it. So frame_
which is a WebFrame has not been deleted yet and therefore the underlying Frame
is kept alive. Once you deallocate the WebFrame and therefore do not touch the
Frame anymore it is free to die. That is as expected I would think?

How is the underlying child WebFrame being kept alive after the call to
WebFrame::removeChild()? Nothing in Source/web is keeping it alive--it's already
been removed as a child of its parent frame, so WebFrame::traceChildren() won't
find it. Nothing in Source/core retains a persistent handle to the WebFrame. And
unless the embedder happens to spill it to the stack, nothing in the embedder
keeps it alive either.

[dcheng, 2014-09-17 11:24:00]

On 2014/09/17 at 11:22:21, sigbjornf wrote:
> On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> > On 2014/09/17 at 11:14:44, ager wrote:
> > > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> > m_frame->isMainFrame()
> > > > &&
> > > > > > viewImpl())
> > > > > > I'm a little concerned about how it's much harder to reason about
memory
> > > > > > correctness in the Oilpan version of this code.
> > > > > > 
> > > > > > Before, the ownership model was very simple--Source/web internally
holds
> > a
> > > > ref
> > > > > > on behalf of the embedder, which it drops once close() is called.
The
> > > > embedder
> > > > > > knows that as long as it doesn't call close(), the WebFrame remains
> > live, no
> > > > > > matter what it does.
> > > > > > 
> > > > > > With Oilpan, the only reference the Blink API holds on behalf of the
> > > > embedder is
> > > > > > for the main frame. As part of frame detach, the embedder
> > (RenderFrameImpl)
> > > > > > removes child WebFrames from the frame tree. Today, the code for
> > > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > > > > 
> > > > > > 1798  if (is_subframe)
> > > > > > 1799    frame->parent()->removeChild(frame);
> > > > > > 1800
> > > > > > 1801  // |frame| is invalid after here.
> > > > > > 1802  frame->close();
> > > > > > 
> > > > > > Imagine that |frame| wasn't passed as a parameter to this method,
and it
> > was
> > > > > > using |frame_| instead (something that's likely to happen in the
> > future).
> > > > After
> > > > > > the call to removeChild(), I don't think there would actually be any
> > > > references
> > > > > > to keep this child WebFrame alive. While this isn't technically an
issue
> > > > > > (because I don't think a GC can run between line 1799 and 1802), it
> > seems
> > > > > > conceptually wrong.
> > > > > 
> > > > > 'frame' would be on the stack, so a GC that scans the stack between
those
> > two
> > > > points shouldn't pose a problem.
> > > > 
> > > > It's a problem because:
> > > > 1) It might not be on the stack in the future--it's a parameter that's
> > probably
> > > > going to get removed at some point.
> > > > 2) I don't think the embedder should have to know about Oilpan
conventions
> > (it
> > > > might only be held in a member, and the embedder shouldn't have to spill
it
> > to
> > > > the stack to keep it alive).
> > > 
> > > As long as a WebFrame exists that has a persistent handle registered with
the
> > oilpan heap infrastructure so until the WebFrame is deallocated the frame
will
> > be alive. The embedder doesn't have to worry about oilpan. The embedding
layer
> > takes care of that. Any WebXYZ object keeps the underlying blink object
alive.
> > 
> > Right, I'm saying that after the call to removeChild(), I don't believe
there
> > will be any such handles registered with Oilpan.
> 
> The persistent reference is only cleared upon close() (cf.
WebLocalFrameImpl::close()) - does that reduce your concern?

The persistent handle is only for the topmost frame, is it not?

[sof, 2014-09-17 11:28:49]

On 2014/09/17 11:24:00, dcheng (OOO) wrote:
> On 2014/09/17 at 11:22:21, sigbjornf wrote:
> > On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> > > On 2014/09/17 at 11:14:44, ager wrote:
> > > > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > > > 
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> > > m_frame->isMainFrame()
> > > > > &&
> > > > > > > viewImpl())
> > > > > > > I'm a little concerned about how it's much harder to reason about
> memory
> > > > > > > correctness in the Oilpan version of this code.
> > > > > > > 
> > > > > > > Before, the ownership model was very simple--Source/web internally
> holds
> > > a
> > > > > ref
> > > > > > > on behalf of the embedder, which it drops once close() is called.
> The
> > > > > embedder
> > > > > > > knows that as long as it doesn't call close(), the WebFrame
remains
> > > live, no
> > > > > > > matter what it does.
> > > > > > > 
> > > > > > > With Oilpan, the only reference the Blink API holds on behalf of
the
> > > > > embedder is
> > > > > > > for the main frame. As part of frame detach, the embedder
> > > (RenderFrameImpl)
> > > > > > > removes child WebFrames from the frame tree. Today, the code for
> > > > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > > > > > 
> > > > > > > 1798  if (is_subframe)
> > > > > > > 1799    frame->parent()->removeChild(frame);
> > > > > > > 1800
> > > > > > > 1801  // |frame| is invalid after here.
> > > > > > > 1802  frame->close();
> > > > > > > 
> > > > > > > Imagine that |frame| wasn't passed as a parameter to this method,
> and it
> > > was
> > > > > > > using |frame_| instead (something that's likely to happen in the
> > > future).
> > > > > After
> > > > > > > the call to removeChild(), I don't think there would actually be
any
> > > > > references
> > > > > > > to keep this child WebFrame alive. While this isn't technically an
> issue
> > > > > > > (because I don't think a GC can run between line 1799 and 1802),
it
> > > seems
> > > > > > > conceptually wrong.
> > > > > > 
> > > > > > 'frame' would be on the stack, so a GC that scans the stack between
> those
> > > two
> > > > > points shouldn't pose a problem.
> > > > > 
> > > > > It's a problem because:
> > > > > 1) It might not be on the stack in the future--it's a parameter that's
> > > probably
> > > > > going to get removed at some point.
> > > > > 2) I don't think the embedder should have to know about Oilpan
> conventions
> > > (it
> > > > > might only be held in a member, and the embedder shouldn't have to
spill
> it
> > > to
> > > > > the stack to keep it alive).
> > > > 
> > > > As long as a WebFrame exists that has a persistent handle registered
with
> the
> > > oilpan heap infrastructure so until the WebFrame is deallocated the frame
> will
> > > be alive. The embedder doesn't have to worry about oilpan. The embedding
> layer
> > > takes care of that. Any WebXYZ object keeps the underlying blink object
> alive.
> > > 
> > > Right, I'm saying that after the call to removeChild(), I don't believe
> there
> > > will be any such handles registered with Oilpan.
> > 
> > The persistent reference is only cleared upon close() (cf.
> WebLocalFrameImpl::close()) - does that reduce your concern?
> 
> The persistent handle is only for the topmost frame, is it not?

Yes, quite so. Keep in mind that the topmost frame will have a FrameTree that'll
still be traced.

[dcheng, 2014-09-17 11:43:01]

On 2014/09/17 at 11:28:49, sigbjornf wrote:
> On 2014/09/17 11:24:00, dcheng (OOO) wrote:
> > On 2014/09/17 at 11:22:21, sigbjornf wrote:
> > > On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> > > > On 2014/09/17 at 11:14:44, ager wrote:
> > > > > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > > > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > > > > 
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> > > > m_frame->isMainFrame()
> > > > > > &&
> > > > > > > > viewImpl())
> > > > > > > > I'm a little concerned about how it's much harder to reason
about
> > memory
> > > > > > > > correctness in the Oilpan version of this code.
> > > > > > > > 
> > > > > > > > Before, the ownership model was very simple--Source/web
internally
> > holds
> > > > a
> > > > > > ref
> > > > > > > > on behalf of the embedder, which it drops once close() is
called.
> > The
> > > > > > embedder
> > > > > > > > knows that as long as it doesn't call close(), the WebFrame
remains
> > > > live, no
> > > > > > > > matter what it does.
> > > > > > > > 
> > > > > > > > With Oilpan, the only reference the Blink API holds on behalf of
the
> > > > > > embedder is
> > > > > > > > for the main frame. As part of frame detach, the embedder
> > > > (RenderFrameImpl)
> > > > > > > > removes child WebFrames from the frame tree. Today, the code for
> > > > > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like this:
> > > > > > > > 
> > > > > > > > 1798  if (is_subframe)
> > > > > > > > 1799    frame->parent()->removeChild(frame);
> > > > > > > > 1800
> > > > > > > > 1801  // |frame| is invalid after here.
> > > > > > > > 1802  frame->close();
> > > > > > > > 
> > > > > > > > Imagine that |frame| wasn't passed as a parameter to this
method,
> > and it
> > > > was
> > > > > > > > using |frame_| instead (something that's likely to happen in the
> > > > future).
> > > > > > After
> > > > > > > > the call to removeChild(), I don't think there would actually be
any
> > > > > > references
> > > > > > > > to keep this child WebFrame alive. While this isn't technically
an
> > issue
> > > > > > > > (because I don't think a GC can run between line 1799 and 1802),
it
> > > > seems
> > > > > > > > conceptually wrong.
> > > > > > > 
> > > > > > > 'frame' would be on the stack, so a GC that scans the stack
between
> > those
> > > > two
> > > > > > points shouldn't pose a problem.
> > > > > > 
> > > > > > It's a problem because:
> > > > > > 1) It might not be on the stack in the future--it's a parameter
that's
> > > > probably
> > > > > > going to get removed at some point.
> > > > > > 2) I don't think the embedder should have to know about Oilpan
> > conventions
> > > > (it
> > > > > > might only be held in a member, and the embedder shouldn't have to
spill
> > it
> > > > to
> > > > > > the stack to keep it alive).
> > > > > 
> > > > > As long as a WebFrame exists that has a persistent handle registered
with
> > the
> > > > oilpan heap infrastructure so until the WebFrame is deallocated the
frame
> > will
> > > > be alive. The embedder doesn't have to worry about oilpan. The embedding
> > layer
> > > > takes care of that. Any WebXYZ object keeps the underlying blink object
> > alive.
> > > > 
> > > > Right, I'm saying that after the call to removeChild(), I don't believe
> > there
> > > > will be any such handles registered with Oilpan.
> > > 
> > > The persistent reference is only cleared upon close() (cf.
> > WebLocalFrameImpl::close()) - does that reduce your concern?
> > 
> > The persistent handle is only for the topmost frame, is it not?
> 
> Yes, quite so. Keep in mind that the topmost frame will have a FrameTree
that'll still be traced.

Right, but let's consider what happens for a child frame (let's call it WebFrame
X). Let's suppose some script is removing an iframe which corresponds to X. From
my understanding, Oilpan is keeping X alive because it can trace it through
WebView::m_mainLocalFrame.

During frame detach, a bunch of stuff happens. But eventually we end up in
RenderFrameImpl::frameDetached(). RenderFrameImpl::frameDetached() removes X
from the frame tree by calling X->parent()->removeChild(X). At this point,
Oilpan has no reachable references to X. It can't trace it from
WebView::m_mainLocalFrame because we just removed it from the frame tree.

Yes, it's technically safe because X is on the stack/in a register. But if the
embedder were to try to call X->close() in a task posted to the message loop,
then there are no guarantees that X would still be live at that point. We
previously guaranteed that the WebFrame would not be destroyed if close() had
not yet been called, but I'm pretty sure that would no longer be true in this
case.

[Mads Ager (chromium), 2014-09-17 11:54:01]

On 2014/09/17 11:43:01, dcheng (OOO) wrote:
> On 2014/09/17 at 11:28:49, sigbjornf wrote:
> > On 2014/09/17 11:24:00, dcheng (OOO) wrote:
> > > On 2014/09/17 at 11:22:21, sigbjornf wrote:
> > > > On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> > > > > On 2014/09/17 at 11:14:44, ager wrote:
> > > > > > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > > > > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > > > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> > > > > m_frame->isMainFrame()
> > > > > > > &&
> > > > > > > > > viewImpl())
> > > > > > > > > I'm a little concerned about how it's much harder to reason
> about
> > > memory
> > > > > > > > > correctness in the Oilpan version of this code.
> > > > > > > > > 
> > > > > > > > > Before, the ownership model was very simple--Source/web
> internally
> > > holds
> > > > > a
> > > > > > > ref
> > > > > > > > > on behalf of the embedder, which it drops once close() is
> called.
> > > The
> > > > > > > embedder
> > > > > > > > > knows that as long as it doesn't call close(), the WebFrame
> remains
> > > > > live, no
> > > > > > > > > matter what it does.
> > > > > > > > > 
> > > > > > > > > With Oilpan, the only reference the Blink API holds on behalf
of
> the
> > > > > > > embedder is
> > > > > > > > > for the main frame. As part of frame detach, the embedder
> > > > > (RenderFrameImpl)
> > > > > > > > > removes child WebFrames from the frame tree. Today, the code
for
> > > > > > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like
this:
> > > > > > > > > 
> > > > > > > > > 1798  if (is_subframe)
> > > > > > > > > 1799    frame->parent()->removeChild(frame);
> > > > > > > > > 1800
> > > > > > > > > 1801  // |frame| is invalid after here.
> > > > > > > > > 1802  frame->close();
> > > > > > > > > 
> > > > > > > > > Imagine that |frame| wasn't passed as a parameter to this
> method,
> > > and it
> > > > > was
> > > > > > > > > using |frame_| instead (something that's likely to happen in
the
> > > > > future).
> > > > > > > After
> > > > > > > > > the call to removeChild(), I don't think there would actually
be
> any
> > > > > > > references
> > > > > > > > > to keep this child WebFrame alive. While this isn't
technically
> an
> > > issue
> > > > > > > > > (because I don't think a GC can run between line 1799 and
1802),
> it
> > > > > seems
> > > > > > > > > conceptually wrong.
> > > > > > > > 
> > > > > > > > 'frame' would be on the stack, so a GC that scans the stack
> between
> > > those
> > > > > two
> > > > > > > points shouldn't pose a problem.
> > > > > > > 
> > > > > > > It's a problem because:
> > > > > > > 1) It might not be on the stack in the future--it's a parameter
> that's
> > > > > probably
> > > > > > > going to get removed at some point.
> > > > > > > 2) I don't think the embedder should have to know about Oilpan
> > > conventions
> > > > > (it
> > > > > > > might only be held in a member, and the embedder shouldn't have to
> spill
> > > it
> > > > > to
> > > > > > > the stack to keep it alive).
> > > > > > 
> > > > > > As long as a WebFrame exists that has a persistent handle registered
> with
> > > the
> > > > > oilpan heap infrastructure so until the WebFrame is deallocated the
> frame
> > > will
> > > > > be alive. The embedder doesn't have to worry about oilpan. The
embedding
> > > layer
> > > > > takes care of that. Any WebXYZ object keeps the underlying blink
object
> > > alive.
> > > > > 
> > > > > Right, I'm saying that after the call to removeChild(), I don't
believe
> > > there
> > > > > will be any such handles registered with Oilpan.
> > > > 
> > > > The persistent reference is only cleared upon close() (cf.
> > > WebLocalFrameImpl::close()) - does that reduce your concern?
> > > 
> > > The persistent handle is only for the topmost frame, is it not?
> > 
> > Yes, quite so. Keep in mind that the topmost frame will have a FrameTree
> that'll still be traced.
> 
> Right, but let's consider what happens for a child frame (let's call it
WebFrame
> X). Let's suppose some script is removing an iframe which corresponds to X.
From
> my understanding, Oilpan is keeping X alive because it can trace it through
> WebView::m_mainLocalFrame.
> 
> During frame detach, a bunch of stuff happens. But eventually we end up in
> RenderFrameImpl::frameDetached(). RenderFrameImpl::frameDetached() removes X
> from the frame tree by calling X->parent()->removeChild(X). At this point,
> Oilpan has no reachable references to X. It can't trace it from
> WebView::m_mainLocalFrame because we just removed it from the frame tree.
> 
> Yes, it's technically safe because X is on the stack/in a register. But if the
> embedder were to try to call X->close() in a task posted to the message loop,
> then there are no guarantees that X would still be live at that point. We
> previously guaranteed that the WebFrame would not be destroyed if close() had
> not yet been called, but I'm pretty sure that would no longer be true in this
> case.

Thanks for spelling it out; I see where you are going now. I had misunderstood
this. I thought that we had a self-persistent in the frames that is only
deallocated when close is called but you are right that we don't and therefore
it is not necessarily true that a Frame is alive until you have called close on
the WebFrame. We can maintain that invariant by keeping persistents instead of
using tracing of the tree. That will match the use of ref counting that is
currently in the system. Sigbjorn, do you see any major concerns with taking
that route?

[sof, 2014-09-17 11:58:31]

On 2014/09/17 11:54:01, Mads Ager (chromium) wrote:
> On 2014/09/17 11:43:01, dcheng (OOO) wrote:
> > On 2014/09/17 at 11:28:49, sigbjornf wrote:
> > > On 2014/09/17 11:24:00, dcheng (OOO) wrote:
> > > > On 2014/09/17 at 11:22:21, sigbjornf wrote:
> > > > > On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> > > > > > On 2014/09/17 at 11:14:44, ager wrote:
> > > > > > > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > > > > > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > > > > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> > > > > > m_frame->isMainFrame()
> > > > > > > > &&
> > > > > > > > > > viewImpl())
> > > > > > > > > > I'm a little concerned about how it's much harder to reason
> > about
> > > > memory
> > > > > > > > > > correctness in the Oilpan version of this code.
> > > > > > > > > > 
> > > > > > > > > > Before, the ownership model was very simple--Source/web
> > internally
> > > > holds
> > > > > > a
> > > > > > > > ref
> > > > > > > > > > on behalf of the embedder, which it drops once close() is
> > called.
> > > > The
> > > > > > > > embedder
> > > > > > > > > > knows that as long as it doesn't call close(), the WebFrame
> > remains
> > > > > > live, no
> > > > > > > > > > matter what it does.
> > > > > > > > > > 
> > > > > > > > > > With Oilpan, the only reference the Blink API holds on
behalf
> of
> > the
> > > > > > > > embedder is
> > > > > > > > > > for the main frame. As part of frame detach, the embedder
> > > > > > (RenderFrameImpl)
> > > > > > > > > > removes child WebFrames from the frame tree. Today, the code
> for
> > > > > > > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like
> this:
> > > > > > > > > > 
> > > > > > > > > > 1798  if (is_subframe)
> > > > > > > > > > 1799    frame->parent()->removeChild(frame);
> > > > > > > > > > 1800
> > > > > > > > > > 1801  // |frame| is invalid after here.
> > > > > > > > > > 1802  frame->close();
> > > > > > > > > > 
> > > > > > > > > > Imagine that |frame| wasn't passed as a parameter to this
> > method,
> > > > and it
> > > > > > was
> > > > > > > > > > using |frame_| instead (something that's likely to happen in
> the
> > > > > > future).
> > > > > > > > After
> > > > > > > > > > the call to removeChild(), I don't think there would
actually
> be
> > any
> > > > > > > > references
> > > > > > > > > > to keep this child WebFrame alive. While this isn't
> technically
> > an
> > > > issue
> > > > > > > > > > (because I don't think a GC can run between line 1799 and
> 1802),
> > it
> > > > > > seems
> > > > > > > > > > conceptually wrong.
> > > > > > > > > 
> > > > > > > > > 'frame' would be on the stack, so a GC that scans the stack
> > between
> > > > those
> > > > > > two
> > > > > > > > points shouldn't pose a problem.
> > > > > > > > 
> > > > > > > > It's a problem because:
> > > > > > > > 1) It might not be on the stack in the future--it's a parameter
> > that's
> > > > > > probably
> > > > > > > > going to get removed at some point.
> > > > > > > > 2) I don't think the embedder should have to know about Oilpan
> > > > conventions
> > > > > > (it
> > > > > > > > might only be held in a member, and the embedder shouldn't have
to
> > spill
> > > > it
> > > > > > to
> > > > > > > > the stack to keep it alive).
> > > > > > > 
> > > > > > > As long as a WebFrame exists that has a persistent handle
registered
> > with
> > > > the
> > > > > > oilpan heap infrastructure so until the WebFrame is deallocated the
> > frame
> > > > will
> > > > > > be alive. The embedder doesn't have to worry about oilpan. The
> embedding
> > > > layer
> > > > > > takes care of that. Any WebXYZ object keeps the underlying blink
> object
> > > > alive.
> > > > > > 
> > > > > > Right, I'm saying that after the call to removeChild(), I don't
> believe
> > > > there
> > > > > > will be any such handles registered with Oilpan.
> > > > > 
> > > > > The persistent reference is only cleared upon close() (cf.
> > > > WebLocalFrameImpl::close()) - does that reduce your concern?
> > > > 
> > > > The persistent handle is only for the topmost frame, is it not?
> > > 
> > > Yes, quite so. Keep in mind that the topmost frame will have a FrameTree
> > that'll still be traced.
> > 
> > Right, but let's consider what happens for a child frame (let's call it
> WebFrame
> > X). Let's suppose some script is removing an iframe which corresponds to X.
> From
> > my understanding, Oilpan is keeping X alive because it can trace it through
> > WebView::m_mainLocalFrame.
> > 
> > During frame detach, a bunch of stuff happens. But eventually we end up in
> > RenderFrameImpl::frameDetached(). RenderFrameImpl::frameDetached() removes X
> > from the frame tree by calling X->parent()->removeChild(X). At this point,
> > Oilpan has no reachable references to X. It can't trace it from
> > WebView::m_mainLocalFrame because we just removed it from the frame tree.
> > 
> > Yes, it's technically safe because X is on the stack/in a register. But if
the
> > embedder were to try to call X->close() in a task posted to the message
loop,
> > then there are no guarantees that X would still be live at that point. We
> > previously guaranteed that the WebFrame would not be destroyed if close()
had
> > not yet been called, but I'm pretty sure that would no longer be true in
this
> > case.
> 
> Thanks for spelling it out; I see where you are going now. I had misunderstood
> this. I thought that we had a self-persistent in the frames that is only
> deallocated when close is called but you are right that we don't and therefore
> it is not necessarily true that a Frame is alive until you have called close
on
> the WebFrame. We can maintain that invariant by keeping persistents instead of
> using tracing of the tree. That will match the use of ref counting that is
> currently in the system. Sigbjorn, do you see any major concerns with taking
> that route?

Adding a level of indirection that handles that persistence shouldn't pose a
problem.

[sof, 2014-09-17 14:22:47]

On 2014/09/17 11:58:31, sof wrote:
> On 2014/09/17 11:54:01, Mads Ager (chromium) wrote:
> > On 2014/09/17 11:43:01, dcheng (OOO) wrote:
> > > On 2014/09/17 at 11:28:49, sigbjornf wrote:
> > > > On 2014/09/17 11:24:00, dcheng (OOO) wrote:
> > > > > On 2014/09/17 at 11:22:21, sigbjornf wrote:
> > > > > > On 2014/09/17 11:17:06, dcheng (OOO) wrote:
> > > > > > > On 2014/09/17 at 11:14:44, ager wrote:
> > > > > > > > On 2014/09/17 11:06:50, dcheng (OOO) wrote:
> > > > > > > > > On 2014/09/17 at 11:04:11, sigbjornf wrote:
> > > > > > > > > > On 2014/09/17 10:58:09, dcheng (OOO) wrote:
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > > > > File Source/web/WebLocalFrameImpl.cpp (right):
> > > > > > > > > > > 
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> >
>
https://codereview.chromium.org/517043003/diff/680001/Source/web/WebLocalFram...
> > > > > > > > > > > Source/web/WebLocalFrameImpl.cpp:505: if (m_frame &&
> > > > > > > m_frame->isMainFrame()
> > > > > > > > > &&
> > > > > > > > > > > viewImpl())
> > > > > > > > > > > I'm a little concerned about how it's much harder to
reason
> > > about
> > > > > memory
> > > > > > > > > > > correctness in the Oilpan version of this code.
> > > > > > > > > > > 
> > > > > > > > > > > Before, the ownership model was very simple--Source/web
> > > internally
> > > > > holds
> > > > > > > a
> > > > > > > > > ref
> > > > > > > > > > > on behalf of the embedder, which it drops once close() is
> > > called.
> > > > > The
> > > > > > > > > embedder
> > > > > > > > > > > knows that as long as it doesn't call close(), the
WebFrame
> > > remains
> > > > > > > live, no
> > > > > > > > > > > matter what it does.
> > > > > > > > > > > 
> > > > > > > > > > > With Oilpan, the only reference the Blink API holds on
> behalf
> > of
> > > the
> > > > > > > > > embedder is
> > > > > > > > > > > for the main frame. As part of frame detach, the embedder
> > > > > > > (RenderFrameImpl)
> > > > > > > > > > > removes child WebFrames from the frame tree. Today, the
code
> > for
> > > > > > > > > > > RenderFrameImpl::frameDetached(WebFrame* frame) looks like
> > this:
> > > > > > > > > > > 
> > > > > > > > > > > 1798  if (is_subframe)
> > > > > > > > > > > 1799    frame->parent()->removeChild(frame);
> > > > > > > > > > > 1800
> > > > > > > > > > > 1801  // |frame| is invalid after here.
> > > > > > > > > > > 1802  frame->close();
> > > > > > > > > > > 
> > > > > > > > > > > Imagine that |frame| wasn't passed as a parameter to this
> > > method,
> > > > > and it
> > > > > > > was
> > > > > > > > > > > using |frame_| instead (something that's likely to happen
in
> > the
> > > > > > > future).
> > > > > > > > > After
> > > > > > > > > > > the call to removeChild(), I don't think there would
> actually
> > be
> > > any
> > > > > > > > > references
> > > > > > > > > > > to keep this child WebFrame alive. While this isn't
> > technically
> > > an
> > > > > issue
> > > > > > > > > > > (because I don't think a GC can run between line 1799 and
> > 1802),
> > > it
> > > > > > > seems
> > > > > > > > > > > conceptually wrong.
> > > > > > > > > > 
> > > > > > > > > > 'frame' would be on the stack, so a GC that scans the stack
> > > between
> > > > > those
> > > > > > > two
> > > > > > > > > points shouldn't pose a problem.
> > > > > > > > > 
> > > > > > > > > It's a problem because:
> > > > > > > > > 1) It might not be on the stack in the future--it's a
parameter
> > > that's
> > > > > > > probably
> > > > > > > > > going to get removed at some point.
> > > > > > > > > 2) I don't think the embedder should have to know about Oilpan
> > > > > conventions
> > > > > > > (it
> > > > > > > > > might only be held in a member, and the embedder shouldn't
have
> to
> > > spill
> > > > > it
> > > > > > > to
> > > > > > > > > the stack to keep it alive).
> > > > > > > > 
> > > > > > > > As long as a WebFrame exists that has a persistent handle
> registered
> > > with
> > > > > the
> > > > > > > oilpan heap infrastructure so until the WebFrame is deallocated
the
> > > frame
> > > > > will
> > > > > > > be alive. The embedder doesn't have to worry about oilpan. The
> > embedding
> > > > > layer
> > > > > > > takes care of that. Any WebXYZ object keeps the underlying blink
> > object
> > > > > alive.
> > > > > > > 
> > > > > > > Right, I'm saying that after the call to removeChild(), I don't
> > believe
> > > > > there
> > > > > > > will be any such handles registered with Oilpan.
> > > > > > 
> > > > > > The persistent reference is only cleared upon close() (cf.
> > > > > WebLocalFrameImpl::close()) - does that reduce your concern?
> > > > > 
> > > > > The persistent handle is only for the topmost frame, is it not?
> > > > 
> > > > Yes, quite so. Keep in mind that the topmost frame will have a FrameTree
> > > that'll still be traced.
> > > 
> > > Right, but let's consider what happens for a child frame (let's call it
> > WebFrame
> > > X). Let's suppose some script is removing an iframe which corresponds to
X.
> > From
> > > my understanding, Oilpan is keeping X alive because it can trace it
through
> > > WebView::m_mainLocalFrame.
> > > 
> > > During frame detach, a bunch of stuff happens. But eventually we end up in
> > > RenderFrameImpl::frameDetached(). RenderFrameImpl::frameDetached() removes
X
> > > from the frame tree by calling X->parent()->removeChild(X). At this point,
> > > Oilpan has no reachable references to X. It can't trace it from
> > > WebView::m_mainLocalFrame because we just removed it from the frame tree.
> > > 
> > > Yes, it's technically safe because X is on the stack/in a register. But if
> the
> > > embedder were to try to call X->close() in a task posted to the message
> loop,
> > > then there are no guarantees that X would still be live at that point. We
> > > previously guaranteed that the WebFrame would not be destroyed if close()
> had
> > > not yet been called, but I'm pretty sure that would no longer be true in
> this
> > > case.
> > 
> > Thanks for spelling it out; I see where you are going now. I had
misunderstood
> > this. I thought that we had a self-persistent in the frames that is only
> > deallocated when close is called but you are right that we don't and
therefore
> > it is not necessarily true that a Frame is alive until you have called close
> on
> > the WebFrame. We can maintain that invariant by keeping persistents instead
of
> > using tracing of the tree. That will match the use of ref counting that is
> > currently in the system. Sigbjorn, do you see any major concerns with taking
> > that route?
> 
> Adding a level of indirection that handles that persistence shouldn't pose a
> problem.

Added a self-referring Persistent to address this, which is explicitly cleared
upon close().

We will have to keep GC plugin exceptions/ignores on those self-refs to quell
protests - acceptable?

[sof, 2014-09-17 21:52:28]

Determining what subset of the frame finalization actions that must be preserved
and recast in an Oilpan setting, is not proving to be workable (nor particularly
maintainable, I suspect.)

So, I've switched approach. The finalization that FrameLoader::dispose()
performs during non-Oilpan LocalFrame finalization, is now performed in an
Oilpan setting slightly earlier when the FrameOwner is detached&disconnected
from the LocalFrame.

I've encountered one failing test with that switch, an xhtml document that
synchronously cancels its parser was missing a now-required extra test for
parser cancellation. 

The latest patchset (38 (!)) makes that switch.

[Mads Ager (chromium), 2014-09-18 12:00:59]

Only minor comments. I think we are getting there. Overall this is looking good
to me. dcheng, what do you think about the move of parts of the teardown that
Sigbjorn went with in his latest patchset. Do you see any immediate issues with
that?

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

https://codereview.chromium.org/517043003/diff/800001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:208: page()->decrementSubframeCount();
Does this decrementSubframeCount call never happen? We don't call
disconnectOwnerElement during destruction in the oilpan build and therefore
could miss this decrementSubframeCount call. It looks like the code has just
changed here so page() is always 0 when we get here? I can replace this check
with RELEASE_ASSERT(!page()) and run all layout tests without trouble. I wonder
if there are other tests we should be running to verify?

So, maybe the subframe count is always decremented in
FrameLoader::detachFromParent?

https://codereview.chromium.org/517043003/diff/800001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:722: loader().dispose();
Just verifying my understanding: This will lead to the
document.prepareForDestruction call that we now do not have via setView(0) in
the destructor, right?

https://codereview.chromium.org/517043003/diff/800001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:154: // As yet unknown if we can "get
away" with releasing it slightly earlier..
Let's remove this part of the comment. We will have to test our way out of this.
It looks like we can get away with it at least based on layout tests.

https://codereview.chromium.org/517043003/diff/800001/Source/core/loader/Prog...
File Source/core/loader/ProgressTracker.h (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/loader/Prog...
Source/core/loader/ProgressTracker.h:69: // This bare frame pointer is safe; it
refers to the
Are there finalization order issues with moving ProgressTracker to the heap and
tracing the local frame pointer? I agree that this is a safe raw pointer and
that we can postpone more work on this until this patch has successfully landed.
:)

https://codereview.chromium.org/517043003/diff/800001/Source/core/page/FrameT...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/page/FrameT...
Source/core/page/FrameTree.cpp:53: #if !ENABLE(OILPAN)
Does this imply that we can avoid doing this non-oilpan as well (as the FIXME
comment below indicates)?

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

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:234: void WebFrame::traceChildren(Visitor* visitor,
WebFrame* frame)
It is probably a good idea to continue to trace the children this way to be
completely safe. With the self-persistents it is probably not strictly
necessary, but with conservative stack scanning in the picture it is nice to be
sure that all the children are kept alive if we find one of the nodes.

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1536: frame->m_selfKeepAlive = frame;
Maybe move this to the constructor to make sure that a new create method cannot
accidentally forget to set up the keep alive handle?

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:120: frame->m_selfKeepAlive = frame;
Maybe move to constructor?

: m_selfKeepAlive(this)

[sof, 2014-09-18 13:31:16]

Thanks again!

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

https://codereview.chromium.org/517043003/diff/800001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:208: page()->decrementSubframeCount();
On 2014/09/18 12:00:59, Mads Ager (chromium) wrote:
> Does this decrementSubframeCount call never happen? We don't call
> disconnectOwnerElement during destruction in the oilpan build and therefore
> could miss this decrementSubframeCount call. It looks like the code has just
> changed here so page() is always 0 when we get here? I can replace this check
> with RELEASE_ASSERT(!page()) and run all layout tests without trouble. I
wonder
> if there are other tests we should be running to verify?
> 
> So, maybe the subframe count is always decremented in
> FrameLoader::detachFromParent?

I had a look at this check at some point; some of the WebFrame unit tests for
frame swapping will have page() set here. I assumed that was as it should be &
the accounting was needed.

I notice that https://codereview.chromium.org/551973005/ changes frame count
handling.

https://codereview.chromium.org/517043003/diff/800001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:722: loader().dispose();
On 2014/09/18 12:00:59, Mads Ager (chromium) wrote:
> Just verifying my understanding: This will lead to the
> document.prepareForDestruction call that we now do not have via setView(0) in
> the destructor, right?

Yes, it will lead to prepareForDestruction(). If setView(0) doesn't trigger the
call over an active document, FrameLoader::dispose() has a separate&stronger pop
at it while destructing, non-Oilpan :)

https://codereview.chromium.org/517043003/diff/800001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:154: // As yet unknown if we can "get
away" with releasing it slightly earlier..
On 2014/09/18 12:00:59, Mads Ager (chromium) wrote:
> Let's remove this part of the comment. We will have to test our way out of
this.
> It looks like we can get away with it at least based on layout tests.

Removed; testing and considering moving Widget to the heap.

https://codereview.chromium.org/517043003/diff/800001/Source/core/loader/Prog...
File Source/core/loader/ProgressTracker.h (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/loader/Prog...
Source/core/loader/ProgressTracker.h:69: // This bare frame pointer is safe; it
refers to the
On 2014/09/18 12:00:59, Mads Ager (chromium) wrote:
> Are there finalization order issues with moving ProgressTracker to the heap
and
> tracing the local frame pointer? I agree that this is a safe raw pointer and
> that we can postpone more work on this until this patch has successfully
landed.
> :)

Yes, that was where I landed in the end -- there are no issues, I did this in
patchset #36 but thought it could wait, hence backed it out.

https://codereview.chromium.org/517043003/diff/800001/Source/core/page/FrameT...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/core/page/FrameT...
Source/core/page/FrameTree.cpp:53: #if !ENABLE(OILPAN)
On 2014/09/18 12:00:59, Mads Ager (chromium) wrote:
> Does this imply that we can avoid doing this non-oilpan as well (as the FIXME
> comment below indicates)?

Maybe, I haven't investigated if the clearing needs to happen right here _if_
the frame still has a FrameClient attached.

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

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:234: void WebFrame::traceChildren(Visitor* visitor,
WebFrame* frame)
On 2014/09/18 12:00:59, Mads Ager (chromium) wrote:
> It is probably a good idea to continue to trace the children this way to be
> completely safe. With the self-persistents it is probably not strictly
> necessary, but with conservative stack scanning in the picture it is nice to
be
> sure that all the children are kept alive if we find one of the nodes.

Very good point, I hadn't realized the implication here of that self-sustaining
change. Hmm, fine either way, but tracing errs on the side of caution, so let's
keep it.

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/517043003/diff/800001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1536: frame->m_selfKeepAlive = frame;
On 2014/09/18 12:00:59, Mads Ager (chromium) wrote:
> Maybe move this to the constructor to make sure that a new create method
cannot
> accidentally forget to set up the keep alive handle?

Good, that's the better place to do it. Done, and for WebRemoteFrameImpl.

[sof, 2014-09-18 13:31:22]

Thanks again!

[sof, 2014-09-18 14:05:55]

On 2014/09/18 13:31:22, sof wrote:
> Thanks again!

A bit brief.. see https://codereview.chromium.org/517043003/#msg66 for followup
comments; looks like a double form submission gone wrong.

[Mads Ager (chromium), 2014-09-19 06:37:24]

LGTM

Thank you for taking this on and iterating through this many rounds of review
comments!

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:719: // FIXME: Oilpan: this might not be
sufficiently strong, but
Nit: Maybe have the comment just state that we dispose the loader now instead of
waiting for the LocalFrame to actually be finalized. At this point we know that
the frame doesn't need to be loaded?

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Prog...
File Source/core/loader/ProgressTracker.h (right):

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Prog...
Source/core/loader/ProgressTracker.h:69: // This bare frame pointer is safe; it
refers to the
Add FIXME: Move ProgressTracker to the oilpan heap and trace the frame pointer?

[dcheng, 2014-09-19 06:55:48]

I am really excited about this patch and I'm cautiously optimistic that this
will help make frame detach saner in the future. I have two main comments and a
few contextual things:
1. If possible, consolidate more things to early dispose() so we don't need an
Oilpan/non-Oilpan way (e.g. my question in
https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...).
I think this will make permanently enabling Oilpan at a later date easier as
well.
2. Please do add a comment to the top of WebLocalFrameImpl.cpp describing the
OIlpan ownership model. This comment was a really useful starting point for me
when I was trying to figure out the frame detach mess, and having an equivalent
version for the OIlpan flow will be helpful for future maintainers.

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:719: // FIXME: Oilpan: this might not be
sufficiently strong, but
Would it be possible to do this unconditionally even in non-Oilpan builds?

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

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:262: if (FrameView* view = m_frame->view())
Is there any particular significance to pulling this into a local?

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:527: // accessible, so bail early.
Out of curiosity, what does the callstack look like?

[sof, 2014-09-19 07:10:08]

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:719: // FIXME: Oilpan: this might not be
sufficiently strong, but
On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> Would it be possible to do this unconditionally even in non-Oilpan builds?

I'm having a look at that atm & if we can float out more from the destructor.

[tkent, 2014-09-19 07:47:16]

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

[tkent, 2014-09-19 07:47:20]

lgtm.

nit: The latest patch contains some reference->pointer conversions.  We had
better keep references by introducing |LocalFrame& frame()| accessor.

nit: The patch is huge. Some parts of the patch can be landed separately if
you'd like to reduce risk of the huge patch.

[sof, 2014-09-19 09:05:31]

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

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:527: // accessible, so bail early.
On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> Out of curiosity, what does the callstack look like?

The failing stack isn't all that interesting, just accessing
DOMMimeType::enabledPlugin() after the frame is detached. Without Oilpan,
enabledPlugin() will just bail out upon seeing !m_frame. It seemed preferable to
address the condition here, rather than in the enabledPlugin() caller.

This is with plugins/access-after-page-destroyed.html

[dcheng, 2014-09-19 09:09:38]

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

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:527: // accessible, so bail early.
On 2014/09/19 09:05:31, sof wrote:
> On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> > Out of curiosity, what does the callstack look like?
> 
> The failing stack isn't all that interesting, just accessing
> DOMMimeType::enabledPlugin() after the frame is detached. Without Oilpan,
> enabledPlugin() will just bail out upon seeing !m_frame. It seemed preferable
to
> address the condition here, rather than in the enabledPlugin() caller.
> 
> This is with plugins/access-after-page-destroyed.html

I guess I was just hoping that this would allow us to remove a check elsewhere.
It sounds like that might be possible in an Oilpan world?

[sof, 2014-09-19 09:18:08]

On 2014/09/19 09:09:38, dcheng (OOO) wrote:
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> File Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> Source/core/loader/FrameLoader.cpp:527: // accessible, so bail early.
> On 2014/09/19 09:05:31, sof wrote:
> > On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> > > Out of curiosity, what does the callstack look like?
> > 
> > The failing stack isn't all that interesting, just accessing
> > DOMMimeType::enabledPlugin() after the frame is detached. Without Oilpan,
> > enabledPlugin() will just bail out upon seeing !m_frame. It seemed
preferable
> to
> > address the condition here, rather than in the enabledPlugin() caller.
> > 
> > This is with plugins/access-after-page-destroyed.html
> 
> I guess I was just hoping that this would allow us to remove a check
elsewhere.
> It sounds like that might be possible in an Oilpan world?

If I understand you correctly, don't think so - the m_frame weak reference on
FrameDestructionObserver will remain non-null until the next Oilpan GC strikes
(at the earliest.)

Only thing I can think of is to add FrameDestructionObserver::hasBeenDetached()
and start using it instead of "!m_frame" checks.

[dcheng, 2014-09-19 09:22:54]

On 2014/09/19 at 09:18:08, sigbjornf wrote:
> On 2014/09/19 09:09:38, dcheng (OOO) wrote:
> >
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> > File Source/core/loader/FrameLoader.cpp (right):
> > 
> >
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> > Source/core/loader/FrameLoader.cpp:527: // accessible, so bail early.
> > On 2014/09/19 09:05:31, sof wrote:
> > > On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> > > > Out of curiosity, what does the callstack look like?
> > > 
> > > The failing stack isn't all that interesting, just accessing
> > > DOMMimeType::enabledPlugin() after the frame is detached. Without Oilpan,
> > > enabledPlugin() will just bail out upon seeing !m_frame. It seemed
preferable
> > to
> > > address the condition here, rather than in the enabledPlugin() caller.
> > > 
> > > This is with plugins/access-after-page-destroyed.html
> > 
> > I guess I was just hoping that this would allow us to remove a check
elsewhere.
> > It sounds like that might be possible in an Oilpan world?
> 
> If I understand you correctly, don't think so - the m_frame weak reference on
FrameDestructionObserver will remain non-null until the next Oilpan GC strikes
(at the earliest.)
> 
> Only thing I can think of is to add
FrameDestructionObserver::hasBeenDetached() and start using it instead of
"!m_frame" checks.

If I understand the change correctly, FrameDestructionObserver will never
observe a null m_frame in an Oilpan world. So shouldn't we be able to remove all
FrameDestructionObserver implementations that check !m_frame?

[dcheng, 2014-09-19 09:23:55]

On 2014/09/19 at 09:22:54, dcheng (OOO) wrote:
> On 2014/09/19 at 09:18:08, sigbjornf wrote:
> > On 2014/09/19 09:09:38, dcheng (OOO) wrote:
> > >
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> > > File Source/core/loader/FrameLoader.cpp (right):
> > > 
> > >
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> > > Source/core/loader/FrameLoader.cpp:527: // accessible, so bail early.
> > > On 2014/09/19 09:05:31, sof wrote:
> > > > On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> > > > > Out of curiosity, what does the callstack look like?
> > > > 
> > > > The failing stack isn't all that interesting, just accessing
> > > > DOMMimeType::enabledPlugin() after the frame is detached. Without
Oilpan,
> > > > enabledPlugin() will just bail out upon seeing !m_frame. It seemed
preferable
> > > to
> > > > address the condition here, rather than in the enabledPlugin() caller.
> > > > 
> > > > This is with plugins/access-after-page-destroyed.html
> > > 
> > > I guess I was just hoping that this would allow us to remove a check
elsewhere.
> > > It sounds like that might be possible in an Oilpan world?
> > 
> > If I understand you correctly, don't think so - the m_frame weak reference
on FrameDestructionObserver will remain non-null until the next Oilpan GC
strikes (at the earliest.)
> > 
> > Only thing I can think of is to add
FrameDestructionObserver::hasBeenDetached() and start using it instead of
"!m_frame" checks.
> 
> If I understand the change correctly, FrameDestructionObserver will never
observe a null m_frame in an Oilpan world. So shouldn't we be able to remove all
FrameDestructionObserver implementations that check !m_frame?

Err, all checks for !m_frame in FrameDestructionObserver implementations.

[sof, 2014-09-19 09:32:46]

On 2014/09/19 09:23:55, dcheng (OOO) wrote:
> On 2014/09/19 at 09:22:54, dcheng (OOO) wrote:
> > On 2014/09/19 at 09:18:08, sigbjornf wrote:
> > > On 2014/09/19 09:09:38, dcheng (OOO) wrote:
> > > >
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> > > > File Source/core/loader/FrameLoader.cpp (right):
> > > > 
> > > >
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
> > > > Source/core/loader/FrameLoader.cpp:527: // accessible, so bail early.
> > > > On 2014/09/19 09:05:31, sof wrote:
> > > > > On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> > > > > > Out of curiosity, what does the callstack look like?
> > > > > 
> > > > > The failing stack isn't all that interesting, just accessing
> > > > > DOMMimeType::enabledPlugin() after the frame is detached. Without
> Oilpan,
> > > > > enabledPlugin() will just bail out upon seeing !m_frame. It seemed
> preferable
> > > > to
> > > > > address the condition here, rather than in the enabledPlugin() caller.
> > > > > 
> > > > > This is with plugins/access-after-page-destroyed.html
> > > > 
> > > > I guess I was just hoping that this would allow us to remove a check
> elsewhere.
> > > > It sounds like that might be possible in an Oilpan world?
> > > 
> > > If I understand you correctly, don't think so - the m_frame weak reference
> on FrameDestructionObserver will remain non-null until the next Oilpan GC
> strikes (at the earliest.)
> > > 
> > > Only thing I can think of is to add
> FrameDestructionObserver::hasBeenDetached() and start using it instead of
> "!m_frame" checks.
> > 
> > If I understand the change correctly, FrameDestructionObserver will never
> observe a null m_frame in an Oilpan world. So shouldn't we be able to remove
all
> FrameDestructionObserver implementations that check !m_frame?
> 
> Err, all checks for !m_frame in FrameDestructionObserver implementations.

It is possible to observe it as null in detached cases, as
FrameDestructionObserver::m_frame is a WeakMember, so observers won't on their
own keep the Frame alive.

plugins/access-after-page-destroyed.html is one example of that :)

[sof, 2014-09-19 10:14:56]

On 2014/09/19 07:10:08, sof wrote:
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
> File Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
> Source/core/frame/LocalFrame.cpp:719: // FIXME: Oilpan: this might not be
> sufficiently strong, but
> On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> > Would it be possible to do this unconditionally even in non-Oilpan builds?
> 
> I'm having a look at that atm & if we can float out more from the destructor.

This appears to work equally well non-Oilpan,

 https://codereview.chromium.org/517043003/patch/860001/870043

if we make sure to also disconnect the children when detaching (as they might be
released earlier non-Oilpan),

 https://codereview.chromium.org/517043003/patch/860001/870032

Seems reasonable? Assuming so, I would prefer not to do it as part of this
Oilpan-specific CL; risks and that. wdyt?

[sof, 2014-09-19 11:56:43]

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:719: // FIXME: Oilpan: this might not be
sufficiently strong, but
On 2014/09/19 06:37:24, Mads Ager (chromium) wrote:
> Nit: Maybe have the comment just state that we dispose the loader now instead
of
> waiting for the LocalFrame to actually be finalized. At this point we know
that
> the frame doesn't need to be loaded?

Updated the comment + moved out the FrameView detach from the destructor also.

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

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:262: if (FrameView* view = m_frame->view())
On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> Is there any particular significance to pulling this into a local?

There is not, just a linear use of a FrameView I considered a minor eyesore
initially.

Backed out + renamed this method back to clear(), to reduce footprint ever so
slightly.

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Prog...
File Source/core/loader/ProgressTracker.h (right):

https://codereview.chromium.org/517043003/diff/820001/Source/core/loader/Prog...
Source/core/loader/ProgressTracker.h:69: // This bare frame pointer is safe; it
refers to the
On 2014/09/19 06:37:24, Mads Ager (chromium) wrote:
> Add FIXME: Move ProgressTracker to the oilpan heap and trace the frame
pointer?

FIXME: Oilpan: added.

[sof, 2014-09-19 11:59:20]

On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> I am really excited about this patch and I'm cautiously optimistic that this
> will help make frame detach saner in the future. I have two main comments and
a
> few contextual things:
> 1. If possible, consolidate more things to early dispose() so we don't need an
> Oilpan/non-Oilpan way (e.g. my question in
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...).
> I think this will make permanently enabling Oilpan at a later date easier as
> well.
> 2. Please do add a comment to the top of WebLocalFrameImpl.cpp describing the
> OIlpan ownership model. This comment was a really useful starting point for me
> when I was trying to figure out the frame detach mess, and having an
equivalent
> version for the OIlpan flow will be helpful for future maintainers.
> 

That diagram and comment was helpful to me also. Added an Oilpan paragraph at
the end.

[sof, 2014-09-19 12:01:56]

On 2014/09/19 10:14:56, sof wrote:
> On 2014/09/19 07:10:08, sof wrote:
> >
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
> > File Source/core/frame/LocalFrame.cpp (right):
> > 
> >
>
https://codereview.chromium.org/517043003/diff/820001/Source/core/frame/Local...
> > Source/core/frame/LocalFrame.cpp:719: // FIXME: Oilpan: this might not be
> > sufficiently strong, but
> > On 2014/09/19 06:55:48, dcheng (OOO) wrote:
> > > Would it be possible to do this unconditionally even in non-Oilpan builds?
> > 
> > I'm having a look at that atm & if we can float out more from the
destructor.
> 
> This appears to work equally well non-Oilpan,
> 
>  https://codereview.chromium.org/517043003/patch/860001/870043
> 
> if we make sure to also disconnect the children when detaching (as they might
be
> released earlier non-Oilpan),
> 
>  https://codereview.chromium.org/517043003/patch/860001/870032
> 
> Seems reasonable? Assuming so, I would prefer not to do it as part of this
> Oilpan-specific CL; risks and that. wdyt?

Decided to back it out from here for that reason, but left behind a FIXME in
~LocalFrame.

[sof, 2014-09-19 12:10:22]

On 2014/09/19 07:47:20, tkent (overloaded) wrote:
> lgtm.
> 
> nit: The latest patch contains some reference->pointer conversions.  We had
> better keep references by introducing |LocalFrame& frame()| accessor.
> 

Yes, thanks. Added frame() accessors to Editor(::Command), InputController,
SpellChecker and FrameConsole.

> nit: The patch is huge. Some parts of the patch can be landed separately if
> you'd like to reduce risk of the huge patch.

I think I will just have to take that risk & follow this monster CL through.

[sof, 2014-09-19 14:21:59]

Thanks for the reviews; will try to land next.

[sof, 2014-09-19 15:05:27]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-19 15:06:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/517043003/880001

[commit-bot: I haz the power, 2014-09-19 15:56:45]

Committed patchset #45 (id:880001) as 182337

[haraken, 2014-09-22 05:35:23]

Added a couple of too late comments :) This is a great CL!

https://codereview.chromium.org/517043003/diff/880001/Source/core/editing/Spe...
File Source/core/editing/SpellChecker.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/editing/Spe...
Source/core/editing/SpellChecker.h:95: LocalFrame& frame() const { return
*m_frame; }

Add ASSERT(m_frame).

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

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:88: #endif

Just help me understand: We don't need to call disconnectOwnerElement() here
because disconnectOwnerElement() should have already been called when the Frame
is detached from the host, right?

Why can we remove setDOMWindow(nullptr)?

Can we add ASSERT(!m_owner) here?

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Frame...
File Source/core/frame/FrameConsole.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Frame...
Source/core/frame/FrameConsole.h:76: LocalFrame& frame() const { return
*m_frame; }

Add ASSERT(m_frame).

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:548: reset();

Just to confirm: The reason you can remove the reset() call from non-oilpan
builds is that the reset() is already called in Frame::setDOMWindow(). Right?

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:282: // performed (accessing the m_domWindow
heap object is unsafe then.)

This comment now looks a bit strange. Now that setDOMWindow() is not called in
any destructor, can we probably just drop this comment?

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:720: loader().clear();

Don't we need to call setDOMWindow(nullptr) here?

https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:169: #endif

Just help me understand:

- In oilpan builds, it is guaranteed that
HTMLFrameOwnerElement::disconnectContentFrame() is called before the
HTMLFrameOwnerElement gets destructed.

- In non-oilpan builds, it is not guaranteed.

Where does the difference come from?

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

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:251: // a new Document within it
(DocumentLoader::createWriterFor().)

This comment looks out-dated. FrameLoader::clear() is now called in
LocalFrame::disconnectOwnerElement().

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.h:68: ALLOW_ONLY_INLINE_ALLOCATION();

DISALLOW_ALLOCATION() ?

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Navi...
File Source/core/loader/NavigationScheduler.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Navi...
Source/core/loader/NavigationScheduler.h:73: ALLOW_ONLY_INLINE_ALLOCATION();

DISALLOW_ALLOCATION() ?

https://codereview.chromium.org/517043003/diff/880001/Source/core/page/FrameT...
File Source/core/page/FrameTree.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/page/FrameT...
Source/core/page/FrameTree.h:33: ALLOW_ONLY_INLINE_ALLOCATION();

DISALLOW_ALLOCATION() ?

https://codereview.chromium.org/517043003/diff/880001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.h:32: class ScopedPageLoadDeferrer FINAL
: public NoBaseWillBeGarbageCollectedFinalized<ScopedPageLoadDeferrer> {

Can we add ALLOW_ONLY_INLINE_ALLOCATION() ?

https://codereview.chromium.org/517043003/diff/880001/Source/core/xml/parser/...
File Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/xml/parser/...
Source/core/xml/parser/XMLDocumentParser.cpp:1011: // document and cancellation
of this parser.

Just help me understand: What's the relationship between this change and moving
Frame to the heap?

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

https://codereview.chromium.org/517043003/diff/880001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:234: void WebFrame::traceChildren(Visitor* visitor,
WebFrame* frame)

Just to confirm: We just need to trace children and don't need to trace a
parent, right?

As we discussed before, this traceChildren is not needed for correctness because
each Web(Local,Remote)Frame keeps a self-keep-alive persistent handle. Am I
understanding things correctly?

[sof, 2014-09-22 07:25:49]

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.h:68: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/09/22 05:35:23, haraken wrote:
> 
> DISALLOW_ALLOCATION() ?

That would be more precise. 

Possible to come up with a better name for this macro? PART_OR_STACK_ALLOCATED()
?

https://codereview.chromium.org/517043003/diff/880001/Source/core/page/Scoped...
File Source/core/page/ScopedPageLoadDeferrer.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/page/Scoped...
Source/core/page/ScopedPageLoadDeferrer.h:32: class ScopedPageLoadDeferrer FINAL
: public NoBaseWillBeGarbageCollectedFinalized<ScopedPageLoadDeferrer> {
On 2014/09/22 05:35:23, haraken wrote:
> 
> Can we add ALLOW_ONLY_INLINE_ALLOCATION() ?

Not readily; the presence of a destructor and the heap vector makes that tricky
when used by pageLoadDeferrerStack(). I can have another look at this type at
some point later.

[sof, 2014-09-22 07:47:44]

https://codereview.chromium.org/517043003/diff/880001/Source/core/xml/parser/...
File Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/xml/parser/...
Source/core/xml/parser/XMLDocumentParser.cpp:1011: // document and cancellation
of this parser.
On 2014/09/22 05:35:23, haraken wrote:
> 
> Just help me understand: What's the relationship between this change and
moving
> Frame to the heap?

The frame might now be detached as a result of sync'ly running some event
handlers during the append operation above, making this parser advance to a
stopped state.

This didn't happen before. Well, it still won't non-Oilpan, but always
performing this extra check made sense. We similarly check across a number of
other tree operations in XMLDocumentParser.

[haraken, 2014-09-22 08:19:03]

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.h:68: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/09/22 07:25:49, sof wrote:
> On 2014/09/22 05:35:23, haraken wrote:
> > 
> > DISALLOW_ALLOCATION() ?
> 
> That would be more precise. 
> 
> Possible to come up with a better name for this macro?
PART_OR_STACK_ALLOCATED()
> ?

Or COMPOUND_OR_STACK_ALLOCATED() ? Then we can rename
ALLOW_ONLY_INLINE_ALLOCATION to COLLECTION_OR_COMPUND_OR_STACK_ALLOCATED.

However, I'm not sure if it's a great improvement... I'd prefer keeping the
current name until we come up with a more consistent name for the three macros.

[sof, 2014-09-22 08:26:38]

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

https://codereview.chromium.org/517043003/diff/880001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:234: void WebFrame::traceChildren(Visitor* visitor,
WebFrame* frame)
On 2014/09/22 05:35:23, haraken wrote:
> 
> Just to confirm: We just need to trace children and don't need to trace a
> parent, right?
> 

I'd say so, yes - if we assume that the topmost frame is let go of last, then I
don't see how any parent might be left untraced.

> As we discussed before, this traceChildren is not needed for correctness
because
> each Web(Local,Remote)Frame keeps a self-keep-alive persistent handle. Am I
> understanding things correctly?

Should we ever reach a state where the self-persistent is released by a Frame,
but the child frame is otherwise still reachable via a frame tree somehow, then
this trace will ensure safety.

If the "safety first" argument feels inconsistent across the two questions asked
here, then let's add a trace of the parent. Or none.

[haraken, 2014-09-22 08:32:55]

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

https://codereview.chromium.org/517043003/diff/880001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:234: void WebFrame::traceChildren(Visitor* visitor,
WebFrame* frame)
On 2014/09/22 08:26:38, sof wrote:
> On 2014/09/22 05:35:23, haraken wrote:
> > 
> > Just to confirm: We just need to trace children and don't need to trace a
> > parent, right?
> > 
> 
> I'd say so, yes - if we assume that the topmost frame is let go of last, then
I
> don't see how any parent might be left untraced.
> 
> > As we discussed before, this traceChildren is not needed for correctness
> because
> > each Web(Local,Remote)Frame keeps a self-keep-alive persistent handle. Am I
> > understanding things correctly?
> 
> Should we ever reach a state where the self-persistent is released by a Frame,
> but the child frame is otherwise still reachable via a frame tree somehow,
then
> this trace will ensure safety.
> 
> If the "safety first" argument feels inconsistent across the two questions
asked
> here, then let's add a trace of the parent. Or none.

Yeah, that's what I wanted to ask :)

If this tracing is needed, it would be more consistent to trace a parent as well
(even though the parent tracing is not needed for some non-trivial reason).
Otherwise, we can just remove the tracing entirely.

[sof, 2014-09-22 08:41:52]

Too many tricky questions :)

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

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:88: #endif
On 2014/09/22 05:35:22, haraken wrote:
> 
> Just help me understand: We don't need to call disconnectOwnerElement() here
> because disconnectOwnerElement() should have already been called when the
Frame
> is detached from the host, right?
> 

Yes, we assume so for local frames. For remote frames, see below.

> Why can we remove setDOMWindow(nullptr)?
> 

For the same reason that it is acceptable not to do it for LocalFrame. i.e.,
there's no need to clear Members on the LocalDOMWindow & registered
DOMWindowProperty objects keep a weak Frame reference, so they'll be cleared (if
needed.)

> Can we add ASSERT(!m_owner) here?

We've covered this previously, this cannot be assumed for remote frames --
https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...

There are ongoing changes elsewhere to frame detach (
https://codereview.chromium.org/551973005/ ); I'd like to see where that ends up
before investing any more time going after detach details.

[dcheng, 2014-09-22 09:15:04]

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

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Frame...
Source/core/frame/Frame.cpp:88: #endif
On 2014/09/22 08:41:51, sof wrote:
> On 2014/09/22 05:35:22, haraken wrote:
> > 
> > Just help me understand: We don't need to call disconnectOwnerElement() here
> > because disconnectOwnerElement() should have already been called when the
> Frame
> > is detached from the host, right?
> > 
> 
> Yes, we assume so for local frames. For remote frames, see below.
> 
> > Why can we remove setDOMWindow(nullptr)?
> > 
> 
> For the same reason that it is acceptable not to do it for LocalFrame. i.e.,
> there's no need to clear Members on the LocalDOMWindow & registered
> DOMWindowProperty objects keep a weak Frame reference, so they'll be cleared
(if
> needed.)
> 
> > Can we add ASSERT(!m_owner) here?
> 
> We've covered this previously, this cannot be assumed for remote frames --
>
https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
> 

Hm. That seems inconsistent. If you don't mind, can you file a bug with label
Cr-Internals-Sandbox-SiteIsolation and assign it to me?
> There are ongoing changes elsewhere to frame detach (
> https://codereview.chromium.org/551973005/ ); I'd like to see where that ends
up
> before investing any more time going after detach details.

[sof, 2014-09-22 09:52:51]

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:548: reset();
On 2014/09/22 05:35:22, haraken wrote:
> 
> Just to confirm: The reason you can remove the reset() call from non-oilpan
> builds is that the reset() is already called in Frame::setDOMWindow(). Right?

Yes.

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:282: // performed (accessing the m_domWindow
heap object is unsafe then.)
On 2014/09/22 05:35:23, haraken wrote:
> 
> This comment now looks a bit strange. Now that setDOMWindow() is not called in
> any destructor, can we probably just drop this comment?

I think it makes some sense to document the reason why it is acceptable to leave
out those setDOMWindow() calls during Oilpan finalization. And having it here
next to the details, rather than at the call site(s), seems also reasonable.

I've refreshed and updated the comment a bit.

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:720: loader().clear();
On 2014/09/22 05:35:23, haraken wrote:
> 
> Don't we need to call setDOMWindow(nullptr) here?

That's a bit too strong, as you then will observably have detached frames with
frame->domWindow() being 0. This breaks various detached tests.

https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:169: #endif
On 2014/09/22 05:35:23, haraken wrote:
> 
> Just help me understand:
> 
> - In oilpan builds, it is guaranteed that
> HTMLFrameOwnerElement::disconnectContentFrame() is called before the
> HTMLFrameOwnerElement gets destructed.
> 
> - In non-oilpan builds, it is not guaranteed.
> 
> Where does the difference come from?

For this CL, it was decided not to start assuming that for non-Oilpan also. See
the ~LocalFrame non-Oilpan FIXME.

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

https://codereview.chromium.org/517043003/diff/880001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:234: void WebFrame::traceChildren(Visitor* visitor,
WebFrame* frame)
On 2014/09/22 08:32:55, haraken wrote:
> On 2014/09/22 08:26:38, sof wrote:
> > On 2014/09/22 05:35:23, haraken wrote:
> > > 
> > > Just to confirm: We just need to trace children and don't need to trace a
> > > parent, right?
> > > 
> > 
> > I'd say so, yes - if we assume that the topmost frame is let go of last,
then
> I
> > don't see how any parent might be left untraced.
> > 
> > > As we discussed before, this traceChildren is not needed for correctness
> > because
> > > each Web(Local,Remote)Frame keeps a self-keep-alive persistent handle. Am
I
> > > understanding things correctly?
> > 
> > Should we ever reach a state where the self-persistent is released by a
Frame,
> > but the child frame is otherwise still reachable via a frame tree somehow,
> then
> > this trace will ensure safety.
> > 
> > If the "safety first" argument feels inconsistent across the two questions
> asked
> > here, then let's add a trace of the parent. Or none.
> 
> Yeah, that's what I wanted to ask :)
> 
> If this tracing is needed, it would be more consistent to trace a parent as
well
> (even though the parent tracing is not needed for some non-trivial reason).
> Otherwise, we can just remove the tracing entirely.

Hmm. I'm going to try to keep this WebFrame tracing around for now; incoming CL
to extend it slightly.

[sof, 2014-09-22 10:03:54]

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

https://codereview.chromium.org/517043003/diff/880001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:251: // a new Document within it
(DocumentLoader::createWriterFor().)
On 2014/09/22 05:35:23, haraken wrote:
> 
> This comment looks out-dated. FrameLoader::clear() is now called in
> LocalFrame::disconnectOwnerElement().

Yes, thanks - refreshed the comment via
https://codereview.chromium.org/594483002/

[haraken, 2014-09-22 10:07:47]

Thanks for all the claritication; LGTM!

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:720: loader().clear();
On 2014/09/22 09:52:51, sof wrote:
> On 2014/09/22 05:35:23, haraken wrote:
> > 
> > Don't we need to call setDOMWindow(nullptr) here?
> 
> That's a bit too strong, as you then will observably have detached frames with
> frame->domWindow() being 0. This breaks various detached tests.

Makes sense. I now understand that we should let the weak processing clear the
DOMWindow rather than explicitly calling setDOMWindow(nullptr).

https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:169: #endif
On 2014/09/22 09:52:51, sof wrote:
> On 2014/09/22 05:35:23, haraken wrote:
> > 
> > Just help me understand:
> > 
> > - In oilpan builds, it is guaranteed that
> > HTMLFrameOwnerElement::disconnectContentFrame() is called before the
> > HTMLFrameOwnerElement gets destructed.
> > 
> > - In non-oilpan builds, it is not guaranteed.
> > 
> > Where does the difference come from?
> 
> For this CL, it was decided not to start assuming that for non-Oilpan also.
See
> the ~LocalFrame non-Oilpan FIXME.

It looks like this is the final part I don't yet fully understand :)

I'm fine with the FIXME, but I don't fully understand the relationship between
the FIXME and when the disconnectOwnerElement() is called (The FIXME is saying
we should do more things in the disconnectOwnerElement(); my question is when
the disconnectOwnerElement() is called).

As far as I scan the code base, there is no difference between oilpan and
non-oilpan about when the disconnectOwnerElement() is called. If so, why does
the following difference appear?

- In oilpan builds, it is guaranteed that
HTMLFrameOwnerElement::disconnectContentFrame() is called before the
HTMLFrameOwnerElement gets destructed.

- In non-oilpan builds, it is not guaranteed.

[sof, 2014-09-22 11:19:07]

On 2014/09/22 09:15:04, dcheng (OOO) wrote:
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Frame...
> File Source/core/frame/Frame.cpp (right):
> 
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Frame...
> Source/core/frame/Frame.cpp:88: #endif
> On 2014/09/22 08:41:51, sof wrote:
> > On 2014/09/22 05:35:22, haraken wrote:
> > > 
> > > Just help me understand: We don't need to call disconnectOwnerElement()
here
> > > because disconnectOwnerElement() should have already been called when the
> > Frame
> > > is detached from the host, right?
> > > 
> > 
> > Yes, we assume so for local frames. For remote frames, see below.
> > 
> > > Why can we remove setDOMWindow(nullptr)?
> > > 
> > 
> > For the same reason that it is acceptable not to do it for LocalFrame. i.e.,
> > there's no need to clear Members on the LocalDOMWindow & registered
> > DOMWindowProperty objects keep a weak Frame reference, so they'll be cleared
> (if
> > needed.)
> > 
> > > Can we add ASSERT(!m_owner) here?
> > 
> > We've covered this previously, this cannot be assumed for remote frames --
> >
>
https://codereview.chromium.org/517043003/diff/260001/Source/core/frame/Frame...
> > 
> 
> Hm. That seems inconsistent. If you don't mind, can you file a bug with label
> Cr-Internals-Sandbox-SiteIsolation and assign it to me?

I now cannot reproduce the failing asserts when running unit tests (I'm quite
certain that's where they came from, but this is weeks in the past.) No other
remote frame related changes during that time could have altered that? I can't
see subsequent changes here altering that.

Let me test some more; it would clearly be worth having !m_owner hold here.

> > There are ongoing changes elsewhere to frame detach (
> > https://codereview.chromium.org/551973005/ ); I'd like to see where that
ends
> up
> > before investing any more time going after detach details.

[sof, 2014-09-22 11:28:53]

On 2014/09/22 10:07:47, haraken wrote:
> Thanks for all the claritication; LGTM!
> 
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
> File Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
> Source/core/frame/LocalFrame.cpp:720: loader().clear();
> On 2014/09/22 09:52:51, sof wrote:
> > On 2014/09/22 05:35:23, haraken wrote:
> > > 
> > > Don't we need to call setDOMWindow(nullptr) here?
> > 
> > That's a bit too strong, as you then will observably have detached frames
with
> > frame->domWindow() being 0. This breaks various detached tests.
> 
> Makes sense. I now understand that we should let the weak processing clear the
> DOMWindow rather than explicitly calling setDOMWindow(nullptr).
> 
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
> File Source/core/html/HTMLFrameOwnerElement.cpp (right):
> 
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
> Source/core/html/HTMLFrameOwnerElement.cpp:169: #endif
> On 2014/09/22 09:52:51, sof wrote:
> > On 2014/09/22 05:35:23, haraken wrote:
> > > 
> > > Just help me understand:
> > > 
> > > - In oilpan builds, it is guaranteed that
> > > HTMLFrameOwnerElement::disconnectContentFrame() is called before the
> > > HTMLFrameOwnerElement gets destructed.
> > > 
> > > - In non-oilpan builds, it is not guaranteed.
> > > 
> > > Where does the difference come from?
> > 
> > For this CL, it was decided not to start assuming that for non-Oilpan also.
> See
> > the ~LocalFrame non-Oilpan FIXME.
> 
> It looks like this is the final part I don't yet fully understand :)
> 
> I'm fine with the FIXME, but I don't fully understand the relationship between
> the FIXME and when the disconnectOwnerElement() is called (The FIXME is saying
> we should do more things in the disconnectOwnerElement(); my question is when
> the disconnectOwnerElement() is called).
> 
> As far as I scan the code base, there is no difference between oilpan and
> non-oilpan about when the disconnectOwnerElement() is called. If so, why does
> the following difference appear?
> 
> - In oilpan builds, it is guaranteed that
> HTMLFrameOwnerElement::disconnectContentFrame() is called before the
> HTMLFrameOwnerElement gets destructed.
> 
> - In non-oilpan builds, it is not guaranteed.

It is not _assumed_ to hold here non-Oilpan, yes. But that doesn't mean it
doesn't hold.

For a CL this size, it is simply too risky to impose non-Oilpan changes in this
area, which is why it was backed out and that FIXME was put there instead.

Test results were fine, so once this CL has settled down and really proven
itself for Oilpan, I think we should consider addressing the FIXME.

[haraken, 2014-09-22 12:18:50]

On 2014/09/22 11:28:53, sof wrote:
> On 2014/09/22 10:07:47, haraken wrote:
> > Thanks for all the claritication; LGTM!
> > 
> >
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
> > File Source/core/frame/LocalFrame.cpp (right):
> > 
> >
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/frame/Local...
> > Source/core/frame/LocalFrame.cpp:720: loader().clear();
> > On 2014/09/22 09:52:51, sof wrote:
> > > On 2014/09/22 05:35:23, haraken wrote:
> > > > 
> > > > Don't we need to call setDOMWindow(nullptr) here?
> > > 
> > > That's a bit too strong, as you then will observably have detached frames
> with
> > > frame->domWindow() being 0. This breaks various detached tests.
> > 
> > Makes sense. I now understand that we should let the weak processing clear
the
> > DOMWindow rather than explicitly calling setDOMWindow(nullptr).
> > 
> >
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
> > File Source/core/html/HTMLFrameOwnerElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/517043003/diff/880001/Source/core/html/HTMLFr...
> > Source/core/html/HTMLFrameOwnerElement.cpp:169: #endif
> > On 2014/09/22 09:52:51, sof wrote:
> > > On 2014/09/22 05:35:23, haraken wrote:
> > > > 
> > > > Just help me understand:
> > > > 
> > > > - In oilpan builds, it is guaranteed that
> > > > HTMLFrameOwnerElement::disconnectContentFrame() is called before the
> > > > HTMLFrameOwnerElement gets destructed.
> > > > 
> > > > - In non-oilpan builds, it is not guaranteed.
> > > > 
> > > > Where does the difference come from?
> > > 
> > > For this CL, it was decided not to start assuming that for non-Oilpan
also.
> > See
> > > the ~LocalFrame non-Oilpan FIXME.
> > 
> > It looks like this is the final part I don't yet fully understand :)
> > 
> > I'm fine with the FIXME, but I don't fully understand the relationship
between
> > the FIXME and when the disconnectOwnerElement() is called (The FIXME is
saying
> > we should do more things in the disconnectOwnerElement(); my question is
when
> > the disconnectOwnerElement() is called).
> > 
> > As far as I scan the code base, there is no difference between oilpan and
> > non-oilpan about when the disconnectOwnerElement() is called. If so, why
does
> > the following difference appear?
> > 
> > - In oilpan builds, it is guaranteed that
> > HTMLFrameOwnerElement::disconnectContentFrame() is called before the
> > HTMLFrameOwnerElement gets destructed.
> > 
> > - In non-oilpan builds, it is not guaranteed.
> 
> It is not _assumed_ to hold here non-Oilpan, yes. But that doesn't mean it
> doesn't hold.

Ah, that makes sense. If it is _assumed_ that disconnectContentFrame() is always
called in both oilpan and non-oilpan, that aligns with my understanding and I'm
fine :)