1232829 - Crash [@ nsRefreshDriver::RemoveRefreshObserver] with CSS transition, navigation

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox in 2-3 seconds if popups are enabled)

@ mozilla::dom::DocumentTimeline::WillRefresh

> -> 148      MOZ_ASSERT(GetRefreshDriver(),
>    149                 "Refresh driver should still be valid inside WillRefresh");
>    150      GetRefreshDriver()->RemoveRefreshObserver(this, Flush_Style);

Assertion failure: GetRefreshDriver() (Refresh driver should still be valid inside WillRefresh), at dom/animation/DocumentTimeline.cpp:149

In non-debug builds, it reaches the next line, calls nsRefreshDriver::RemoveRefreshObserver with null |this|, and quickly crashes within that function.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Daniel Holbert [:dholbert]]

Tagging birtles, as he added this assertion (and the subsequent line that derefs the null refresh-driver pointer) in bug 1195180. Seems likely that this is a regression from that bug, though I suppose it's also possible that this assertion was valid at that point in time & has become invalid since.

Comment 3

[Brian Birtles (:birtles)]

Actually there was a null-check and no assertion when I wrote the patch but during review it got removed :/ (bug 1195180 comment 23).

I'll have to work out if something changed since then.

Comment 4

[Brian Birtles (:birtles)]

It looks like at the *start* of the call to WillRefresh() mDocument->GetShell() is returning null.

nsRefreshDriver checks for a null pres shell before calling WillRefresh[1] so I guess we're looking up a different pres context when we go via mDocument. I guess the combination of document.write() followed by history.back() and the presence of a pop-up window triggers a situation where mDocument on DocumentTimeline no longer has a pres shell.

[1] https://dxr.mozilla.org/mozilla-central/rev/cb66ffeb6725e8344818e8e2f707ae2eaeb953b4/layout/base/nsRefreshDriver.cpp#1653

Comment 5

[Brian Birtles (:birtles)]

Attachment: Further reduced test case (crashes Firefox)

Comment 6

[Brian Birtles (:birtles)]

My current theory is that using document.write() + history.back() means we end up getting evicted from the bfcache and hence mDocument->GetShell() returns null.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jesse Ruderman from comment #0)
> Created attachment 8698691 [details]
> testcase (crashes Firefox in 2-3 seconds if popups are enabled)
> 
> @ mozilla::dom::DocumentTimeline::WillRefresh
> 
> > -> 148      MOZ_ASSERT(GetRefreshDriver(),
> >    149                 "Refresh driver should still be valid inside WillRefresh");
> >    150      GetRefreshDriver()->RemoveRefreshObserver(this, Flush_Style);
> 
> Assertion failure: GetRefreshDriver() (Refresh driver should still be valid
> inside WillRefresh), at dom/animation/DocumentTimeline.cpp:149

I could reproduce this crash on non E10S. The DocumentTimeline was nullified (caused by document.write) at [1], but someone held its reference so the DocumentTimeline was alive at that time. 
https://dxr.mozilla.org/mozilla-central/rev/e6463ae7eda2775bc84593bb4a0742940bb87379/dom/base/nsDocument.cpp#2120

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> (In reply to Jesse Ruderman from comment #0)
> > Created attachment 8698691 [details]
> > testcase (crashes Firefox in 2-3 seconds if popups are enabled)
> > 
> > @ mozilla::dom::DocumentTimeline::WillRefresh
> > 
> > > -> 148      MOZ_ASSERT(GetRefreshDriver(),
> > >    149                 "Refresh driver should still be valid inside WillRefresh");
> > >    150      GetRefreshDriver()->RemoveRefreshObserver(this, Flush_Style);
> > 
> > Assertion failure: GetRefreshDriver() (Refresh driver should still be valid
> > inside WillRefresh), at dom/animation/DocumentTimeline.cpp:149
> 
> I could reproduce this crash on non E10S. The DocumentTimeline was nullified
> (caused by document.write) at [1], but someone held its reference so the
> DocumentTimeline was alive at that time. 

I was wrong. Nobody held the reference. The DocumentTimeline was just alive because cycle collector did not run at the time.
So, we should call RemoveRefreshObserver for DocumentTimeline just before nullifying it.

Comment 9

[Brian Birtles (:birtles)]

Attachment: Detach obsolete DocumentTimeline from refresh driver when the document is reset

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31af8ffc81ac

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Brian Birtles (:birtles) from comment #6)
> My current theory is that using document.write() + history.back() means we
> end up getting evicted from the bfcache and hence mDocument->GetShell()
> returns null.
Yes, any mutation to the DOM tree evicts bfcache for that entry.
And document.write() removes all the child nodes from document before adding new content.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8699365 [details] [diff] [review]
Detach obsolete DocumentTimeline from refresh driver when the document is reset

>+  if (mDocumentTimeline) {
>+    nsRefreshDriver* rd = mPresShell->GetPresContext()->RefreshDriver();
You need to null check mPresShell and GetPresContext()

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Do we need the same stuff for animation controller?

Comment 13

[Brian Birtles (:birtles)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Do we need the same stuff for animation controller?

I haven't been able to reproduce a similar crash using the same test with SVG animation elements. I'm not sure exactly why except that:

* We don't release the animation controller in nsDocument::Reset
* We pause the controller on page hide which disconnects it from the refresh driver
* We don't try to access the refresh driver in WillRefresh

I suspect it's that second point that is particularly pertinent here.

We've been fuzzing the SMIL code for quite a few years and haven't come across a bug in that area yet so I suspect it's ok. I'm not sure it's worth digging into too much further since long-term I expect that class to go away (we'll either drop SMIL altogether or rewrite it JS using the Web Animations API).

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84169b40100b

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84169b40100b

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

And we need this on branches too

Comment 17

[Brian Birtles (:birtles)]

Attachment: Detach obsolete DocumentTimeline from refresh driver when the document is reset

This is the version of the patch that landed on m-c (which includes the review feedback from comment 11).

Approval Request Comment
[Feature/regressing bug #]: bug 1195180
[User impact if declined]: Crash when using document.write() in combination with CSS animations/transitions in some situations (e.g. when pop-ups are present)
[Describe test coverage new/current, TreeHerder]: Patch includes regression test, has baked on mozilla-central for 2 weeks.
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8703912 [details] [diff] [review]
Detach obsolete DocumentTimeline from refresh driver when the document is reset

Taking it in aurora.
Could you detail why you think that there is absolutely no risk? To me, every single change carries risk.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8703912 [details] [diff] [review]
Detach obsolete DocumentTimeline from refresh driver when the document is reset

Crash fix that includes an automated test, beta44+

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/624eb61f2c31

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/aaa41a664b41

Comment 22

[Brian Birtles (:birtles)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> Comment on attachment 8703912 [details] [diff] [review]
> Detach obsolete DocumentTimeline from refresh driver when the document is
> reset
> 
> Taking it in aurora.
> Could you detail why you think that there is absolutely no risk? To me,
> every single change carries risk.

None here is just shorthand for "none other than the usual possibility that this introduces some otherwise unforeseen adverse side effects" since I noticed others were no longer writing that. That is, I don't know of any specific risks here.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/aaa41a664b41

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi guys, this crash signature is spiking up on 47.0b. This has jumped 136 spots up and is ranked at #15 at the moment. Could you please help find an owner who can investigate the crash reports? Thanks!

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

This or similar stacks? Do you have links to some crash reports?

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Olli Pettay [:smaug] (pto-ish May 4 - May 8) from comment #26)
> This or similar stacks? Do you have links to some crash reports?

They re-mapped crash-stats version info and since that change was made an hour ago, I don't see this in the top ranked crashes on 47.0b. Sorry for the confusion!