Don't update the active cue set in detached Documents

Don't update the active cue set in detached Documents

After (or during) detaching the document, it makes very little sense to
keep updating the timeline and the set of active cues. Instead just punt
and ignore. Most importantly, this avoids trying to dispatch events to
Nodes (HTMLTrackElements precisely) during the disposal of the Document.
Remove the similar check in CueTimeline::endIgnoringUpdateRequests,
that was originally added for WK bug 105606 [1] which fixed a similar
issue, since the check in updateActiveCues should cover the relevant
part of that too.

[1] https://bugs.webkit.org/show_bug.cgi?id=105606

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

[fs, 2015-08-11 16:44:48]

fs@opera.com changed reviewers:
+ philipj@opera.com

[fs, 2015-08-11 16:44:48]

There's a few different ways/places we could use to check this condition
(ignoreUpdateRequests is another example). This is fairly minimal change though.

[philipj_slow, 2015-08-12 07:39:34]

Is a non-flaky test case possible? Creating a video and track element inside an
iframe and then getting that iframe GC'd might do the trick.

https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
File Source/core/html/track/CueTimeline.cpp (right):

https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
Source/core/html/track/CueTimeline.cpp:138: if
(mediaElement.document().isDetached())
Do you think we should make if conditional on !OILPAN? If not this seems like
the kind of code everyone will be afraid to touch because it's no longer
possible to reproduce the original problem.

[fs, 2015-08-12 07:55:45]

On 2015/08/12 07:39:34, philipj wrote:
> Is a non-flaky test case possible? Creating a video and track element inside
an
> iframe and then getting that iframe GC'd might do the trick.

Yes, hopefully something like that should work, I haven't tested it out yet
though.

https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
File Source/core/html/track/CueTimeline.cpp (right):

https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
Source/core/html/track/CueTimeline.cpp:138: if
(mediaElement.document().isDetached())
On 2015/08/12 07:39:33, philipj wrote:
> Do you think we should make if conditional on !OILPAN? If not this seems like
> the kind of code everyone will be afraid to touch because it's no longer
> possible to reproduce the original problem.

I've been pondering whether the (somewhat similar) condition below
(endIgnoringUpdateRequests) should use inDocument() && !isDetached() (maybe even
without the first part if we think that the timeline should be running in
inactive documents). I could make a follow-up to either move this check into
ignoreUpdateRequests, or make all updates go through endIgnoringUpdateRequests
(== always use display-scope).
If we think we'll never hit this w/ Oilpan (dispose replaced by GC) I guess that
would be another possible follow-up (which then may or may not apply below as
well.)

[philipj_slow, 2015-08-12 08:10:30]

On 2015/08/12 07:55:45, fs wrote:
> On 2015/08/12 07:39:34, philipj wrote:
> > Is a non-flaky test case possible? Creating a video and track element inside
> an
> > iframe and then getting that iframe GC'd might do the trick.
> 
> Yes, hopefully something like that should work, I haven't tested it out yet
> though.
> 
>
https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
> File Source/core/html/track/CueTimeline.cpp (right):
> 
>
https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
> Source/core/html/track/CueTimeline.cpp:138: if
> (mediaElement.document().isDetached())
> On 2015/08/12 07:39:33, philipj wrote:
> > Do you think we should make if conditional on !OILPAN? If not this seems
like
> > the kind of code everyone will be afraid to touch because it's no longer
> > possible to reproduce the original problem.
> 
> I've been pondering whether the (somewhat similar) condition below
> (endIgnoringUpdateRequests) should use inDocument() && !isDetached() (maybe
even
> without the first part if we think that the timeline should be running in
> inactive documents). I could make a follow-up to either move this check into
> ignoreUpdateRequests, or make all updates go through endIgnoringUpdateRequests
> (== always use display-scope).
> If we think we'll never hit this w/ Oilpan (dispose replaced by GC) I guess
that
> would be another possible follow-up (which then may or may not apply below as
> well.)

Do you remember why the mediaElement().inActiveDocument() is part of
endIgnoringUpdateRequests() to begin with? If it's to avoid some crash or
another, it's possible that this too would go away with Oilpan.

Since the stack that we have isn't possible with Oilpan, at least I think it'd
be fine to make this fix conditional so that it'll go away eventually.

[fs, 2015-08-12 10:28:34]

On 2015/08/12 08:10:30, philipj wrote:
> On 2015/08/12 07:55:45, fs wrote:
> > On 2015/08/12 07:39:34, philipj wrote:
> > > Is a non-flaky test case possible? Creating a video and track element
inside
> > an
> > > iframe and then getting that iframe GC'd might do the trick.
> > 
> > Yes, hopefully something like that should work, I haven't tested it out yet
> > though.
> > 
> >
>
https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
> > File Source/core/html/track/CueTimeline.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1282903003/diff/1/Source/core/html/track/CueT...
> > Source/core/html/track/CueTimeline.cpp:138: if
> > (mediaElement.document().isDetached())
> > On 2015/08/12 07:39:33, philipj wrote:
> > > Do you think we should make if conditional on !OILPAN? If not this seems
> like
> > > the kind of code everyone will be afraid to touch because it's no longer
> > > possible to reproduce the original problem.
> > 
> > I've been pondering whether the (somewhat similar) condition below
> > (endIgnoringUpdateRequests) should use inDocument() && !isDetached() (maybe
> even
> > without the first part if we think that the timeline should be running in
> > inactive documents). I could make a follow-up to either move this check into
> > ignoreUpdateRequests, or make all updates go through
endIgnoringUpdateRequests
> > (== always use display-scope).
> > If we think we'll never hit this w/ Oilpan (dispose replaced by GC) I guess
> that
> > would be another possible follow-up (which then may or may not apply below
as
> > well.)
> 
> Do you remember why the mediaElement().inActiveDocument() is part of
> endIgnoringUpdateRequests() to begin with? If it's to avoid some crash or
> another, it's possible that this too would go away with Oilpan.

I don't know why it was initially added, but I'd suspect it's for reasons
similar to the one we try to deal with here. I'll see if I can dig that out of
history...

> Since the stack that we have isn't possible with Oilpan, at least I think it'd
> be fine to make this fix conditional so that it'll go away eventually.

Ok. I'll add the ifdef.

[fs, 2015-08-12 10:57:14]

On 2015/08/12 10:28:34, fs wrote:
> On 2015/08/12 08:10:30, philipj wrote:
...
> > Do you remember why the mediaElement().inActiveDocument() is part of
> > endIgnoringUpdateRequests() to begin with? If it's to avoid some crash or
> > another, it's possible that this too would go away with Oilpan.
> 
> I don't know why it was initially added, but I'd suspect it's for reasons
> similar to the one we try to deal with here. I'll see if I can dig that out of
> history...

Traced it back to https://chromium.googlesource.com/chromium/blink/+/d89954d4 /
https://bugs.webkit.org/show_bug.cgi?id=105606 - which indeed is due to pretty
much the same thing. So, unless dropping it, in favor of the newly added test,
gives me hard time (seems unlikely) I'll do that here as well - unless you think
we should play it reeeeally safe here of course...

[philipj_slow, 2015-08-12 11:31:08]

On 2015/08/12 10:57:14, fs wrote:
> On 2015/08/12 10:28:34, fs wrote:
> > On 2015/08/12 08:10:30, philipj wrote:
> ...
> > > Do you remember why the mediaElement().inActiveDocument() is part of
> > > endIgnoringUpdateRequests() to begin with? If it's to avoid some crash or
> > > another, it's possible that this too would go away with Oilpan.
> > 
> > I don't know why it was initially added, but I'd suspect it's for reasons
> > similar to the one we try to deal with here. I'll see if I can dig that out
of
> > history...
> 
> Traced it back to https://chromium.googlesource.com/chromium/blink/+/d89954d4
/
> https://bugs.webkit.org/show_bug.cgi?id=105606 - which indeed is due to pretty
> much the same thing. So, unless dropping it, in favor of the newly added test,
> gives me hard time (seems unlikely) I'll do that here as well - unless you
think
> we should play it reeeeally safe here of course...

Yeah, that sounds very similar to this crash. Consolidating the handling to a
single place with #if !OILPAN sounds good.

[fs, 2015-08-12 13:31:28]

On 2015/08/12 11:31:08, philipj wrote:
> On 2015/08/12 10:57:14, fs wrote:
> > On 2015/08/12 10:28:34, fs wrote:
> > > On 2015/08/12 08:10:30, philipj wrote:
> > ...
> > > > Do you remember why the mediaElement().inActiveDocument() is part of
> > > > endIgnoringUpdateRequests() to begin with? If it's to avoid some crash
or
> > > > another, it's possible that this too would go away with Oilpan.
> > > 
> > > I don't know why it was initially added, but I'd suspect it's for reasons
> > > similar to the one we try to deal with here. I'll see if I can dig that
out
> of
> > > history...
> > 
> > Traced it back to
https://chromium.googlesource.com/chromium/blink/+/d89954d4
> /
> > https://bugs.webkit.org/show_bug.cgi?id=105606 - which indeed is due to
pretty
> > much the same thing. So, unless dropping it, in favor of the newly added
test,
> > gives me hard time (seems unlikely) I'll do that here as well - unless you
> think
> > we should play it reeeeally safe here of course...
> 
> Yeah, that sounds very similar to this crash. Consolidating the handling to a
> single place with #if !OILPAN sounds good.

Added preprocessor check and remove inActiveDocument check. Haven't managed a
stable test yet...

[philipj_slow, 2015-08-12 14:03:12]

Change looks good. Were you eventually able to reproduce the clusterfuzz crash
to take inspiration from that?

[fs, 2015-08-12 14:14:55]

On 2015/08/12 14:03:12, philipj wrote:
> Change looks good. Were you eventually able to reproduce the clusterfuzz crash
> to take inspiration from that?

It's been difficult enough to catch in the debugger to get a "complete" view of
what references are at play etc. For the test I've been using, it reproduces
quite easily but only ~1/7 runs or so (presumably when GC kicks in?)

[philipj_slow, 2015-08-12 15:05:34]

On 2015/08/12 14:14:55, fs wrote:
> On 2015/08/12 14:03:12, philipj wrote:
> > Change looks good. Were you eventually able to reproduce the clusterfuzz
crash
> > to take inspiration from that?
> 
> It's been difficult enough to catch in the debugger to get a "complete" view
of
> what references are at play etc. For the test I've been using, it reproduces
> quite easily but only ~1/7 runs or so (presumably when GC kicks in?)

I take it you know about --exit-after-n-crashes-or-timeouts=1 and
--disable-breakpad so that you can at least insert crashes and get backtraces
wherever you want them?

[fs, 2015-08-13 12:04:19]

On 2015/08/12 15:05:34, philipj wrote:
> On 2015/08/12 14:14:55, fs wrote:
> > On 2015/08/12 14:03:12, philipj wrote:
> > > Change looks good. Were you eventually able to reproduce the clusterfuzz
> crash
> > > to take inspiration from that?
> > 
> > It's been difficult enough to catch in the debugger to get a "complete" view
> of
> > what references are at play etc. For the test I've been using, it reproduces
> > quite easily but only ~1/7 runs or so (presumably when GC kicks in?)
> 
> I take it you know about --exit-after-n-crashes-or-timeouts=1 and
> --disable-breakpad so that you can at least insert crashes and get backtraces
> wherever you want them?

I've managed to get something that crashes reliably - but not always. It seems
it's dependent on the order in which wrappers are GCd - something which I don't
know how to control (if controllable...) So, ok to land this as-is (without a
test)? (The usefulness:time spent ratio feels like it's approaching a small
number by now...)

[philipj_slow, 2015-08-13 12:11:19]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-08-13 12:11:20]

lgtm

[commit-bot: I haz the power, 2015-08-13 12:11:30]

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

[philipj_slow, 2015-08-13 12:11:53]

If the test isn't a huge pile of horror and crashes often enough, it'd still
provide some kind of value, I guess.

[fs, 2015-08-13 12:29:27]

On 2015/08/13 12:11:53, philipj wrote:
> If the test isn't a huge pile of horror and crashes often enough, it'd still
> provide some kind of value, I guess.

Hmm, actually managed to make it always crash now... It depends on waiting a
gratuitous amount of milliseconds (150) though, so adds to the cycle-times... I
can upload it though if you like (will be another CL.)

[fs, 2015-08-13 12:37:56]

On 2015/08/13 12:29:27, fs wrote:
> On 2015/08/13 12:11:53, philipj wrote:
> > If the test isn't a huge pile of horror and crashes often enough, it'd still
> > provide some kind of value, I guess.
> 
> Hmm, actually managed to make it always crash now... It depends on waiting a
> gratuitous amount of milliseconds (150) though, so adds to the cycle-times...
I
> can upload it though if you like (will be another CL.)

I uploaded it: https://codereview.chromium.org/1291063002 - let's see if the
trybots crash reliably on it too... (hopefully manage to start before this ends
up landing.)

[commit-bot: I haz the power, 2015-08-13 14:50:06]

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

[commit-bot: I haz the power, 2015-08-13 14:50:07]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2015-08-13 15:05:05]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-08-13 15:05:15]

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

[commit-bot: I haz the power, 2015-08-13 17:15:10]

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

[commit-bot: I haz the power, 2015-08-13 17:15:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2015-08-13 21:36:48]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-08-13 21:37:07]

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

[commit-bot: I haz the power, 2015-08-14 01:58:00]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200507