Remove DOMWindowProperty from Performance

Remove DOMWindowProperty from Performance

We're deprecating DOMWindowProperty, which is equivalent to ContextLifecycleObserver.
This CL does the replacement for Performance.

BUG=610176
Committed: https://crrev.com/0aa1063996215029b25ec69a78a5027854b20b29
Cr-Commit-Position: refs/heads/master@{#438155}

[haraken, 2016-12-13 07:52:10]

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

[haraken, 2016-12-13 07:52:11]

PTAL

[haraken, 2016-12-13 08:15:22]

https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:108:
frame()->performanceMonitor()->unsubscribeAll(this);

BTW, this is problematic. The destructor is touching other on-heap objects...
I'll fix in a follow up CL.

[sof, 2016-12-13 08:33:12]

lgtm

https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:108:
frame()->performanceMonitor()->unsubscribeAll(this);
On 2016/12/13 08:15:21, haraken wrote:
> 
> BTW, this is problematic. The destructor is touching other on-heap objects...
> I'll fix in a follow up CL.

Good catch; it's dead code though?

[trackermaster6, 2016-12-13 08:35:38]

Thanks for the valuable information.I have a
website-https://www.vouchermedia.com/  based on voucher codes.And this
information is help me to solve some problems in my website.Keep posting such
type of informative post.Thank you.

[haraken, 2016-12-13 08:41:47]

>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> 
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/timing/Performance.cpp:108:
> frame()->performanceMonitor()->unsubscribeAll(this);
> On 2016/12/13 08:15:21, haraken wrote:
> > 
> > BTW, this is problematic. The destructor is touching other on-heap
objects...
> > I'll fix in a follow up CL.
> 
> Good catch; it's dead code though?

Probably. However, in general, there's no guarantee that contextDestroyed() is
called before ContextLifecycleObserver is garbage-collected, right?

[haraken, 2016-12-13 08:41:52]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-13 08:42:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2016-12-13 09:03:22]

On 2016/12/13 08:41:47, haraken wrote:
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/timing/Performance.cpp:108:
> > frame()->performanceMonitor()->unsubscribeAll(this);
> > On 2016/12/13 08:15:21, haraken wrote:
> > > 
> > > BTW, this is problematic. The destructor is touching other on-heap
> objects...
> > > I'll fix in a follow up CL.
> > 
> > Good catch; it's dead code though?
> 
> Probably. However, in general, there's no guarantee that contextDestroyed() is
> called before ContextLifecycleObserver is garbage-collected, right?

Yes, could happen for documents that won't be explicitly detached/shutdown. But
those shouldn't have a frame to start with.. which raises the question, how does
this change cope with Performance objects created with a null LocalFrame* ?

[haraken, 2016-12-13 09:05:50]

On 2016/12/13 09:03:22, sof wrote:
> On 2016/12/13 08:41:47, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/timing/Performance.cpp:108:
> > > frame()->performanceMonitor()->unsubscribeAll(this);
> > > On 2016/12/13 08:15:21, haraken wrote:
> > > > 
> > > > BTW, this is problematic. The destructor is touching other on-heap
> > objects...
> > > > I'll fix in a follow up CL.
> > > 
> > > Good catch; it's dead code though?
> > 
> > Probably. However, in general, there's no guarantee that contextDestroyed()
is
> > called before ContextLifecycleObserver is garbage-collected, right?
> 
> Yes, could happen for documents that won't be explicitly detached/shutdown.
But
> those shouldn't have a frame to start with.. which raises the question, how
does
> this change cope with Performance objects created with a null LocalFrame* ?

You're right. I'm not worried about a document that isn't associated with a
frame.

A Performance object can die before Document shuts down. Then Performance's
destructor is called before contextDestroyed() is called, right?

[sof, 2016-12-13 09:16:16]

On 2016/12/13 09:05:50, haraken wrote:
> On 2016/12/13 09:03:22, sof wrote:
> > On 2016/12/13 08:41:47, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/timing/Performance.cpp:108:
> > > > frame()->performanceMonitor()->unsubscribeAll(this);
> > > > On 2016/12/13 08:15:21, haraken wrote:
> > > > > 
> > > > > BTW, this is problematic. The destructor is touching other on-heap
> > > objects...
> > > > > I'll fix in a follow up CL.
> > > > 
> > > > Good catch; it's dead code though?
> > > 
> > > Probably. However, in general, there's no guarantee that
contextDestroyed()
> is
> > > called before ContextLifecycleObserver is garbage-collected, right?
> > 
> > Yes, could happen for documents that won't be explicitly detached/shutdown.
> But
> > those shouldn't have a frame to start with.. which raises the question, how
> does
> > this change cope with Performance objects created with a null LocalFrame* ?
> 
> You're right. I'm not worried about a document that isn't associated with a
> frame.
> 

What happens here if you try to touch window.performance on a document created
via DOMImplementation.createDocument() ?

> A Performance object can die before Document shuts down. Then Performance's
> destructor is called before contextDestroyed() is called, right?

contextDestroyed() is called via Document::shutdown(), so that should have
happened already.

And if the Document is in a shape that shutdown() won't be called, then I
believe this Performance object won't have a LocalFrame* to start with.

[haraken, 2016-12-13 09:20:56]

On 2016/12/13 09:16:16, sof wrote:
> On 2016/12/13 09:05:50, haraken wrote:
> > On 2016/12/13 09:03:22, sof wrote:
> > > On 2016/12/13 08:41:47, haraken wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > > > File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > > > third_party/WebKit/Source/core/timing/Performance.cpp:108:
> > > > > frame()->performanceMonitor()->unsubscribeAll(this);
> > > > > On 2016/12/13 08:15:21, haraken wrote:
> > > > > > 
> > > > > > BTW, this is problematic. The destructor is touching other on-heap
> > > > objects...
> > > > > > I'll fix in a follow up CL.
> > > > > 
> > > > > Good catch; it's dead code though?
> > > > 
> > > > Probably. However, in general, there's no guarantee that
> contextDestroyed()
> > is
> > > > called before ContextLifecycleObserver is garbage-collected, right?
> > > 
> > > Yes, could happen for documents that won't be explicitly
detached/shutdown.
> > But
> > > those shouldn't have a frame to start with.. which raises the question,
how
> > does
> > > this change cope with Performance objects created with a null LocalFrame*
?
> > 
> > You're right. I'm not worried about a document that isn't associated with a
> > frame.
> > 
> 
> What happens here if you try to touch window.performance on a document created
> via DOMImplementation.createDocument() ?

Ah, you're right.

> > A Performance object can die before Document shuts down. Then Performance's
> > destructor is called before contextDestroyed() is called, right?
> 
> contextDestroyed() is called via Document::shutdown(), so that should have
> happened already.
> 
> And if the Document is in a shape that shutdown() won't be called, then I
> believe this Performance object won't have a LocalFrame* to start with.

Can the following scenario happen?

1) Performance object is created.
2) Performance object becomes unreachable.
3) GC runs. GC calls Performance's destructor.
4) Document is still alive.

This may not happen in the case of Performance, but I guess it could happen in
general.

In any case, DOMImplementation.createDocument() means that we cannot assume that
contextDestroyed() gets called before the destructor gets called.

[sof, 2016-12-13 09:29:32]

On 2016/12/13 09:20:56, haraken wrote:
> On 2016/12/13 09:16:16, sof wrote:
> > On 2016/12/13 09:05:50, haraken wrote:
> > > On 2016/12/13 09:03:22, sof wrote:
> > > > On 2016/12/13 08:41:47, haraken wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > > > > File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2569153002/diff/1/third_party/WebKit/Source/c...
> > > > > > third_party/WebKit/Source/core/timing/Performance.cpp:108:
> > > > > > frame()->performanceMonitor()->unsubscribeAll(this);
> > > > > > On 2016/12/13 08:15:21, haraken wrote:
> > > > > > > 
> > > > > > > BTW, this is problematic. The destructor is touching other on-heap
> > > > > objects...
> > > > > > > I'll fix in a follow up CL.
> > > > > > 
> > > > > > Good catch; it's dead code though?
> > > > > 
> > > > > Probably. However, in general, there's no guarantee that
> > contextDestroyed()
> > > is
> > > > > called before ContextLifecycleObserver is garbage-collected, right?
> > > > 
> > > > Yes, could happen for documents that won't be explicitly
> detached/shutdown.
> > > But
> > > > those shouldn't have a frame to start with.. which raises the question,
> how
> > > does
> > > > this change cope with Performance objects created with a null
LocalFrame*
> ?
> > > 
> > > You're right. I'm not worried about a document that isn't associated with
a
> > > frame.
> > > 
> > 
> > What happens here if you try to touch window.performance on a document
created
> > via DOMImplementation.createDocument() ?
> 
> Ah, you're right.
> 
> > > A Performance object can die before Document shuts down. Then
Performance's
> > > destructor is called before contextDestroyed() is called, right?
> > 
> > contextDestroyed() is called via Document::shutdown(), so that should have
> > happened already.
> > 
> > And if the Document is in a shape that shutdown() won't be called, then I
> > believe this Performance object won't have a LocalFrame* to start with.
> 
> Can the following scenario happen?
> 
> 1) Performance object is created.
> 2) Performance object becomes unreachable.
> 3) GC runs. GC calls Performance's destructor.
> 4) Document is still alive.
> 
> This may not happen in the case of Performance, but I guess it could happen in
> general.
> 

This can happen today for all DOMWindowProperty instances, as they're weakly
held.

So, you're correct, it isn't dead code. But I don't see how it can happen when
both Document & Performance die in the same cycle, where that dtor would cause a
crash in general.

> In any case, DOMImplementation.createDocument() means that we cannot assume
that
> contextDestroyed() gets called before the destructor gets called.

Yes. But Performance wouldn't have a frame to start with, so no harm caused.

[commit-bot: I haz the power, 2016-12-13 10:03:02]

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

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

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-12-13 12:12:48]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-12-13 12:12:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2569153002/#ps20001
(title: "temp")

[commit-bot: I haz the power, 2016-12-13 12:13:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 13:49:02]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481631168711680,
"parent_rev": "aea96407304a13830cc75398c20cd92a960d881f", "commit_rev":
"1a7e8be67d171b6111b132cded069f90cc8237a5"}

[commit-bot: I haz the power, 2016-12-13 13:49:35]

Description was changed from

==========
Remove DOMWindowProperty from Performance

We're deprecating DOMWindowProperty, which is equivalent to
ContextLifecycleObserver.
This CL does the replacement for Performance.

BUG=610176
==========

to

==========
Remove DOMWindowProperty from Performance

We're deprecating DOMWindowProperty, which is equivalent to
ContextLifecycleObserver.
This CL does the replacement for Performance.

BUG=610176

Review-Url: https://codereview.chromium.org/2569153002
==========

[commit-bot: I haz the power, 2016-12-13 13:49:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-13 13:51:51]

Description was changed from

==========
Remove DOMWindowProperty from Performance

We're deprecating DOMWindowProperty, which is equivalent to
ContextLifecycleObserver.
This CL does the replacement for Performance.

BUG=610176

Review-Url: https://codereview.chromium.org/2569153002
==========

to

==========
Remove DOMWindowProperty from Performance

We're deprecating DOMWindowProperty, which is equivalent to
ContextLifecycleObserver.
This CL does the replacement for Performance.

BUG=610176

Committed: https://crrev.com/0aa1063996215029b25ec69a78a5027854b20b29
Cr-Commit-Position: refs/heads/master@{#438155}
==========

[commit-bot: I haz the power, 2016-12-13 13:51:52]

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