Oilpan: make DOMWindowProperty a GC mixin.

Oilpan: make DOMWindowProperty a GC mixin.

As all objects deriving from DOMWindowProperty are on the heap, make
it a GC mixin and have it trace its Window reference. The registered
DOMWindowProperty instances on a LocalDOMWindow live at least as long
it.

The benefit is that we can remove its destructor. Deriving objects can
as a consequence remove their empty finalizers.

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

[sof, 2014-09-09 11:41:15]

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

[sof, 2014-09-09 11:41:16]

Please take a look.

Split out from https://codereview.chromium.org/517043003/

[Mads Ager (chromium), 2014-09-09 12:09:43]

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

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

Looks good overall. One question in connection with ApplicationCache.

https://codereview.chromium.org/552733003/diff/40001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/552733003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:589: willDestroyDocumentInFrame();
Look at all of this stuff we don't have to do in the destructor with Oilpan. I
like. :)

https://codereview.chromium.org/552733003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1897: #if ENABLE(OILPAN)
nit: maybe just have the if around the entire body since it will not be called
non-oilpan anyway?

https://codereview.chromium.org/552733003/diff/40001/Source/core/loader/appca...
File Source/core/loader/appcache/ApplicationCache.cpp (right):

https://codereview.chromium.org/552733003/diff/40001/Source/core/loader/appca...
Source/core/loader/appcache/ApplicationCache.cpp:51: if (ApplicationCacheHost*
cacheHost = applicationCacheHost())
What makes sure that m_frame is alive at this point? It looks like m_frame is a
raw pointer, so it is not entirely clear to me if the m_frame access here is
safe.

How will this be dealt with when LocalFrame is garbage collected. At that point
I assume that we cannot access m_frame to get the cache host?

[sof, 2014-09-09 12:53:58]

https://codereview.chromium.org/552733003/diff/40001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/552733003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1897: #if ENABLE(OILPAN)
On 2014/09/09 12:09:44, Mads Ager (chromium) wrote:
> nit: maybe just have the if around the entire body since it will not be called
> non-oilpan anyway?

ok.

https://codereview.chromium.org/552733003/diff/40001/Source/core/loader/appca...
File Source/core/loader/appcache/ApplicationCache.cpp (right):

https://codereview.chromium.org/552733003/diff/40001/Source/core/loader/appca...
Source/core/loader/appcache/ApplicationCache.cpp:51: if (ApplicationCacheHost*
cacheHost = applicationCacheHost())
On 2014/09/09 12:09:44, Mads Ager (chromium) wrote:
> What makes sure that m_frame is alive at this point? It looks like m_frame is
a
> raw pointer, so it is not entirely clear to me if the m_frame access here is
> safe.
> 
> How will this be dealt with when LocalFrame is garbage collected. At that
point
> I assume that we cannot access m_frame to get the cache host?

Hmm, this dtor is problematic with Oilpan for two reasons:

 - there's no guarantee that m_frame will be cleared upon finalization, as the
DOMWindowProperty might not be notified.
 - And if it is, the appcache host cannot be accessed :)

There is a fair chance that the ApplicationCacheHost on a DocumentLoader dies at
the same time as the ApplicationCache it is connected to, but an unsafe
arrangement it is.

I've addressed this by moving ApplicationCacheHost to the heap & made its
appcache back-reference be weak.

Thanks for pointing out the problem here; brain freeze on my part.

[Mads Ager (chromium), 2014-09-09 13:07:25]

LGTM

https://codereview.chromium.org/552733003/diff/80001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/552733003/diff/80001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1920: #endif
Sorry, my mistake, the plugin complaints, the call chain needs to be full for
the non-oilpan build because the super calls are still checked there. so only
the ->trace lines can be #if'ed out. Feel free to take it back to what you had
initially.

[sof, 2014-09-09 13:15:45]

Thanks for the review.

(Will wait for final Oilpan test results before taking the next step.)

https://codereview.chromium.org/552733003/diff/80001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/552733003/diff/80001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1920: #endif
On 2014/09/09 13:07:25, Mads Ager (chromium) wrote:
> Sorry, my mistake, the plugin complaints, the call chain needs to be full for
> the non-oilpan build because the super calls are still checked there. so only
> the ->trace lines can be #if'ed out. Feel free to take it back to what you had
> initially. 

I left it as not tracing the local fields, non-Oilpan.

[sof, 2014-09-09 18:37:30]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-09 18:38:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/552733003/100001

[commit-bot: I haz the power, 2014-09-09 18:47:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 18:47:15]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[haraken, 2014-09-10 02:49:35]

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

[haraken, 2014-09-10 02:49:36]

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

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;

How is it guaranteed that this m_frame does not become a stale pointer?

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.h (right):

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.h:367:
WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> > m_properties;

Now I understand that this should be strong members, not weak members.
LocalDOMWindow and its DOMWindowProperties should live and die together.

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
File Source/core/loader/appcache/ApplicationCache.h (right):

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
Source/core/loader/appcache/ApplicationCache.h:54: #if !ENABLE(OILPAN)

I'm OK with this, but the ASSERT(!m_frame) is still valid until you move the
Frame to the heap in the next patch, right?

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
File Source/core/loader/appcache/ApplicationCacheHost.h (right):

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
Source/core/loader/appcache/ApplicationCacheHost.h:170:
RawPtrWillBeWeakMember<ApplicationCache> m_domApplicationCache;

I'm a bit behind. Why is making this a weak member solve the problem that
ApplicationCache can access a stale m_frame?

https://codereview.chromium.org/552733003/diff/100001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/552733003/diff/100001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.cpp:185: stopUpdating();

Why is this necessary only in oilpan?

[sof, 2014-09-10 05:49:40]

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

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;
On 2014/09/10 02:49:36, haraken wrote:
> 
> How is it guaranteed that this m_frame does not become a stale pointer?

It will be non-null if the LocalDOMWindow is somehow not frameDestroyed() before
being finalized. It can only be finalized if any of its DOMWindowProperty are no
longer references (cf. strong reference on the the next line here.)

=> LocalDOMWindow and DOMWindowProperty will die together. Not clearing m_frame
will not be observable, and it cannot be used as a stale reference.

We can loosen the connection and make the LocalDOMWindow back reference weak,
but I don't think it is called for. wdyt?

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
File Source/core/loader/appcache/ApplicationCache.h (right):

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
Source/core/loader/appcache/ApplicationCache.h:54: #if !ENABLE(OILPAN)
On 2014/09/10 02:49:36, haraken wrote:
> 
> I'm OK with this, but the ASSERT(!m_frame) is still valid until you move the
> Frame to the heap in the next patch, right?

It should be.

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
File Source/core/loader/appcache/ApplicationCacheHost.h (right):

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
Source/core/loader/appcache/ApplicationCacheHost.h:170:
RawPtrWillBeWeakMember<ApplicationCache> m_domApplicationCache;
On 2014/09/10 02:49:36, haraken wrote:
> 
> I'm a bit behind. Why is making this a weak member solve the problem that
> ApplicationCache can access a stale m_frame?

If the host outlives the appcache, someone needs to clear the frame reference.
As the appcache will not be informed of the frame disappearing (now or later),
we cannot perform the unregistration from its (==appcache) side.

https://codereview.chromium.org/552733003/diff/100001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/552733003/diff/100001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.cpp:185: stopUpdating();
On 2014/09/10 02:49:36, haraken wrote:
> 
> Why is this necessary only in oilpan?

Can't rely on willDestroyGlobalObjectInFrame() having been called.

[Mads Ager (chromium), 2014-09-10 06:34:13]

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

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;
On 2014/09/10 05:49:40, sof wrote:
> On 2014/09/10 02:49:36, haraken wrote:
> > 
> > How is it guaranteed that this m_frame does not become a stale pointer?
> 
> It will be non-null if the LocalDOMWindow is somehow not frameDestroyed()
before
> being finalized. It can only be finalized if any of its DOMWindowProperty are
no
> longer references (cf. strong reference on the the next line here.)
> 
> => LocalDOMWindow and DOMWindowProperty will die together. Not clearing
m_frame
> will not be observable, and it cannot be used as a stale reference.
> 
> We can loosen the connection and make the LocalDOMWindow back reference weak,
> but I don't think it is called for. wdyt?

My two cents: We should avoid using weakness if we can. It seems to me that a
DOMWindowProperty should keep the DOMWindow it is a property of alive. I would
keep the strong relationship.

[haraken, 2014-09-10 07:46:53]

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

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;
On 2014/09/10 06:34:13, Mads Ager (chromium) wrote:
> On 2014/09/10 05:49:40, sof wrote:
> > On 2014/09/10 02:49:36, haraken wrote:
> > > 
> > > How is it guaranteed that this m_frame does not become a stale pointer?
> > 
> > It will be non-null if the LocalDOMWindow is somehow not frameDestroyed()
> before
> > being finalized. It can only be finalized if any of its DOMWindowProperty
are
> no
> > longer references (cf. strong reference on the the next line here.)
> > 
> > => LocalDOMWindow and DOMWindowProperty will die together. Not clearing
> m_frame
> > will not be observable, and it cannot be used as a stale reference.
> > 
> > We can loosen the connection and make the LocalDOMWindow back reference
weak,
> > but I don't think it is called for. wdyt?
> 
> My two cents: We should avoid using weakness if we can. It seems to me that a
> DOMWindowProperty should keep the DOMWindow it is a property of alive. I would
> keep the strong relationship.

Agreed. I prefer keeping the bi-directional strong references between
LocalDOMWindow and DOMWindowProperty.

However, what confuses me is that you are using WeakMember<LocalFrame> in your
original move-Frame-to-the-heap CL
(https://codereview.chromium.org/517043003/diff/380001/Source/core/frame/DOMWi...).
Doesn't it mean that DOMWindowProperty outlives the LocalFrame?

What's a lifetime relationship between LocalFrame, DOMWindowProperty and
DOMWindow?

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
File Source/core/loader/appcache/ApplicationCacheHost.h (right):

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
Source/core/loader/appcache/ApplicationCacheHost.h:170:
RawPtrWillBeWeakMember<ApplicationCache> m_domApplicationCache;
On 2014/09/10 05:49:40, sof wrote:
> On 2014/09/10 02:49:36, haraken wrote:
> > 
> > I'm a bit behind. Why is making this a weak member solve the problem that
> > ApplicationCache can access a stale m_frame?
> 
> If the host outlives the appcache, someone needs to clear the frame reference.
> As the appcache will not be informed of the frame disappearing (now or later),
> we cannot perform the unregistration from its (==appcache) side.

Thanks, makes sense.

Can we remove willDestroyGlobalObjectInFrame from oilpan builds?

[sof, 2014-09-10 08:21:37]

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

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;
On 2014/09/10 07:46:53, haraken wrote:
> On 2014/09/10 06:34:13, Mads Ager (chromium) wrote:
> > On 2014/09/10 05:49:40, sof wrote:
> > > On 2014/09/10 02:49:36, haraken wrote:
> > > > 
> > > > How is it guaranteed that this m_frame does not become a stale pointer?
> > > 
> > > It will be non-null if the LocalDOMWindow is somehow not frameDestroyed()
> > before
> > > being finalized. It can only be finalized if any of its DOMWindowProperty
> are
> > no
> > > longer references (cf. strong reference on the the next line here.)
> > > 
> > > => LocalDOMWindow and DOMWindowProperty will die together. Not clearing
> > m_frame
> > > will not be observable, and it cannot be used as a stale reference.
> > > 
> > > We can loosen the connection and make the LocalDOMWindow back reference
> weak,
> > > but I don't think it is called for. wdyt?
> > 
> > My two cents: We should avoid using weakness if we can. It seems to me that
a
> > DOMWindowProperty should keep the DOMWindow it is a property of alive. I
would
> > keep the strong relationship.
> 
> Agreed. I prefer keeping the bi-directional strong references between
> LocalDOMWindow and DOMWindowProperty.
> 
> However, what confuses me is that you are using WeakMember<LocalFrame> in your
> original move-Frame-to-the-heap CL
>
(https://codereview.chromium.org/517043003/diff/380001/Source/core/frame/DOMWi...).
> Doesn't it mean that DOMWindowProperty outlives the LocalFrame?
> 

Yes, some tests (all resource leak related, i think) fail if you don't keep it
as such. i.e., if you externally close the frame, but keep other references to
its contained DOMWindow, you can observe the frame as alive. If that isn't a
weak reference.

I decided not to try to overturn that behavior.

> What's a lifetime relationship between LocalFrame, DOMWindowProperty and
> DOMWindow?

What specifically do you want to know? This is such a huge question to pose.

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
File Source/core/loader/appcache/ApplicationCacheHost.h (right):

https://codereview.chromium.org/552733003/diff/100001/Source/core/loader/appc...
Source/core/loader/appcache/ApplicationCacheHost.h:170:
RawPtrWillBeWeakMember<ApplicationCache> m_domApplicationCache;
On 2014/09/10 07:46:53, haraken wrote:
> On 2014/09/10 05:49:40, sof wrote:
> > On 2014/09/10 02:49:36, haraken wrote:
> > > 
> > > I'm a bit behind. Why is making this a weak member solve the problem that
> > > ApplicationCache can access a stale m_frame?
> > 
> > If the host outlives the appcache, someone needs to clear the frame
reference.
> > As the appcache will not be informed of the frame disappearing (now or
later),
> > we cannot perform the unregistration from its (==appcache) side.
> 
> Thanks, makes sense.
> 
> Can we remove willDestroyGlobalObjectInFrame from oilpan builds?

No. You can still be informed of this if e.g., the frame is reused.

[haraken, 2014-09-10 08:34:42]

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

https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;
On 2014/09/10 08:21:37, sof wrote:
> On 2014/09/10 07:46:53, haraken wrote:
> > On 2014/09/10 06:34:13, Mads Ager (chromium) wrote:
> > > On 2014/09/10 05:49:40, sof wrote:
> > > > On 2014/09/10 02:49:36, haraken wrote:
> > > > > 
> > > > > How is it guaranteed that this m_frame does not become a stale
pointer?
> > > > 
> > > > It will be non-null if the LocalDOMWindow is somehow not
frameDestroyed()
> > > before
> > > > being finalized. It can only be finalized if any of its
DOMWindowProperty
> > are
> > > no
> > > > longer references (cf. strong reference on the the next line here.)
> > > > 
> > > > => LocalDOMWindow and DOMWindowProperty will die together. Not clearing
> > > m_frame
> > > > will not be observable, and it cannot be used as a stale reference.
> > > > 
> > > > We can loosen the connection and make the LocalDOMWindow back reference
> > weak,
> > > > but I don't think it is called for. wdyt?
> > > 
> > > My two cents: We should avoid using weakness if we can. It seems to me
that
> a
> > > DOMWindowProperty should keep the DOMWindow it is a property of alive. I
> would
> > > keep the strong relationship.
> > 
> > Agreed. I prefer keeping the bi-directional strong references between
> > LocalDOMWindow and DOMWindowProperty.
> > 
> > However, what confuses me is that you are using WeakMember<LocalFrame> in
your
> > original move-Frame-to-the-heap CL
> >
>
(https://codereview.chromium.org/517043003/diff/380001/Source/core/frame/DOMWi...).
> > Doesn't it mean that DOMWindowProperty outlives the LocalFrame?
> > 
> 
> Yes, some tests (all resource leak related, i think) fail if you don't keep it
> as such. i.e., if you externally close the frame, but keep other references to
> its contained DOMWindow, you can observe the frame as alive. If that isn't a
> weak reference.
> 
> I decided not to try to overturn that behavior.
> 
> > What's a lifetime relationship between LocalFrame, DOMWindowProperty and
> > DOMWindow?
> 
> What specifically do you want to know? This is such a huge question to pose.

Thanks. What I want to understand is why the following situation cannot happen:

- LocalFrame dies but LocalDOMWindow and DOMWindowProperties are still alive.
Then the m_frame becomes a stale pointer (until you move the Frame to the heap
and use a WeakMember).

[sof, 2014-09-10 08:39:12]

On 2014/09/10 08:34:42, haraken wrote:
>
https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
> File Source/core/frame/DOMWindowProperty.h (right):
> 
>
https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
> Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;
> On 2014/09/10 08:21:37, sof wrote:
> > On 2014/09/10 07:46:53, haraken wrote:
> > > On 2014/09/10 06:34:13, Mads Ager (chromium) wrote:
> > > > On 2014/09/10 05:49:40, sof wrote:
> > > > > On 2014/09/10 02:49:36, haraken wrote:
> > > > > > 
> > > > > > How is it guaranteed that this m_frame does not become a stale
> pointer?
> > > > > 
> > > > > It will be non-null if the LocalDOMWindow is somehow not
> frameDestroyed()
> > > > before
> > > > > being finalized. It can only be finalized if any of its
> DOMWindowProperty
> > > are
> > > > no
> > > > > longer references (cf. strong reference on the the next line here.)
> > > > > 
> > > > > => LocalDOMWindow and DOMWindowProperty will die together. Not
clearing
> > > > m_frame
> > > > > will not be observable, and it cannot be used as a stale reference.
> > > > > 
> > > > > We can loosen the connection and make the LocalDOMWindow back
reference
> > > weak,
> > > > > but I don't think it is called for. wdyt?
> > > > 
> > > > My two cents: We should avoid using weakness if we can. It seems to me
> that
> > a
> > > > DOMWindowProperty should keep the DOMWindow it is a property of alive. I
> > would
> > > > keep the strong relationship.
> > > 
> > > Agreed. I prefer keeping the bi-directional strong references between
> > > LocalDOMWindow and DOMWindowProperty.
> > > 
> > > However, what confuses me is that you are using WeakMember<LocalFrame> in
> your
> > > original move-Frame-to-the-heap CL
> > >
> >
>
(https://codereview.chromium.org/517043003/diff/380001/Source/core/frame/DOMWi...).
> > > Doesn't it mean that DOMWindowProperty outlives the LocalFrame?
> > > 
> > 
> > Yes, some tests (all resource leak related, i think) fail if you don't keep
it
> > as such. i.e., if you externally close the frame, but keep other references
to
> > its contained DOMWindow, you can observe the frame as alive. If that isn't a
> > weak reference.
> > 
> > I decided not to try to overturn that behavior.
> > 
> > > What's a lifetime relationship between LocalFrame, DOMWindowProperty and
> > > DOMWindow?
> > 
> > What specifically do you want to know? This is such a huge question to pose.
> 
> Thanks. What I want to understand is why the following situation cannot
happen:
> 
> - LocalFrame dies but LocalDOMWindow and DOMWindowProperties are still alive.
> Then the m_frame becomes a stale pointer (until you move the Frame to the heap
> and use a WeakMember).

With this CL, if the frame does die, then frameDestroyed() is called, which
clears LocalDOMWindow's FrameDestructionObserver::m_frame and (all)
DOMWindowProperty::m_frame.

That prevents the above condition from being a problem - agreed?

[haraken, 2014-09-10 08:41:27]

On 2014/09/10 08:39:12, sof wrote:
> On 2014/09/10 08:34:42, haraken wrote:
> >
>
https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
> > File Source/core/frame/DOMWindowProperty.h (right):
> > 
> >
>
https://codereview.chromium.org/552733003/diff/100001/Source/core/frame/DOMWi...
> > Source/core/frame/DOMWindowProperty.h:52: LocalFrame* m_frame;
> > On 2014/09/10 08:21:37, sof wrote:
> > > On 2014/09/10 07:46:53, haraken wrote:
> > > > On 2014/09/10 06:34:13, Mads Ager (chromium) wrote:
> > > > > On 2014/09/10 05:49:40, sof wrote:
> > > > > > On 2014/09/10 02:49:36, haraken wrote:
> > > > > > > 
> > > > > > > How is it guaranteed that this m_frame does not become a stale
> > pointer?
> > > > > > 
> > > > > > It will be non-null if the LocalDOMWindow is somehow not
> > frameDestroyed()
> > > > > before
> > > > > > being finalized. It can only be finalized if any of its
> > DOMWindowProperty
> > > > are
> > > > > no
> > > > > > longer references (cf. strong reference on the the next line here.)
> > > > > > 
> > > > > > => LocalDOMWindow and DOMWindowProperty will die together. Not
> clearing
> > > > > m_frame
> > > > > > will not be observable, and it cannot be used as a stale reference.
> > > > > > 
> > > > > > We can loosen the connection and make the LocalDOMWindow back
> reference
> > > > weak,
> > > > > > but I don't think it is called for. wdyt?
> > > > > 
> > > > > My two cents: We should avoid using weakness if we can. It seems to me
> > that
> > > a
> > > > > DOMWindowProperty should keep the DOMWindow it is a property of alive.
I
> > > would
> > > > > keep the strong relationship.
> > > > 
> > > > Agreed. I prefer keeping the bi-directional strong references between
> > > > LocalDOMWindow and DOMWindowProperty.
> > > > 
> > > > However, what confuses me is that you are using WeakMember<LocalFrame>
in
> > your
> > > > original move-Frame-to-the-heap CL
> > > >
> > >
> >
>
(https://codereview.chromium.org/517043003/diff/380001/Source/core/frame/DOMWi...).
> > > > Doesn't it mean that DOMWindowProperty outlives the LocalFrame?
> > > > 
> > > 
> > > Yes, some tests (all resource leak related, i think) fail if you don't
keep
> it
> > > as such. i.e., if you externally close the frame, but keep other
references
> to
> > > its contained DOMWindow, you can observe the frame as alive. If that isn't
a
> > > weak reference.
> > > 
> > > I decided not to try to overturn that behavior.
> > > 
> > > > What's a lifetime relationship between LocalFrame, DOMWindowProperty and
> > > > DOMWindow?
> > > 
> > > What specifically do you want to know? This is such a huge question to
pose.
> > 
> > Thanks. What I want to understand is why the following situation cannot
> happen:
> > 
> > - LocalFrame dies but LocalDOMWindow and DOMWindowProperties are still
alive.
> > Then the m_frame becomes a stale pointer (until you move the Frame to the
heap
> > and use a WeakMember).
> 
> With this CL, if the frame does die, then frameDestroyed() is called, which
> clears LocalDOMWindow's FrameDestructionObserver::m_frame and (all)
> DOMWindowProperty::m_frame.
> 
> That prevents the above condition from being a problem - agreed?

Thanks, agreed! LGTM.

[sof, 2014-09-10 08:50:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-10 08:50:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/552733003/100001

[commit-bot: I haz the power, 2014-09-10 08:55:44]

Committed patchset #6 (id:100001) as 181717