607529 - mozRequestAnimationFrame callbacks can be called after document has gone away

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Cameron McCormack (:heycam)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: 

It seems like a mozRequestAnimationFrame callback can still be invoked after a document has been navigated away from.  (I noticed because it causes window.document to be null.)


Reproducible: Sometimes

Steps to Reproduce:
1. Open the attachment.
2. Keep reloading the page until you get the alert.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: test

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this is presumably happening during paint suppression for the new document.... We do normally file event handlers during that time (and certainly reflow, restyle, etc).  So maybe we do want this behavior....

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we check GetScriptHanderObject or whatever we did for trees? I don't think we should be dispatching events in a state where window.document is null.

Comment 4

[Cameron McCormack (:heycam)]

Incidentally, I couldn't make a setInterval(blah, 1) run after the document has navigated away, but that might have been just because the timing is harder to hit.

Comment 5

[Gregg Tavares]

So this is seems to still be true

Go to any of the samples here
http://www.khronos.org/webgl/wiki/Demo_Repository

The ones that animate all use mozRequestAnimationFrame.

Open the Web Console. Run a demo that animates. Pick "refresh". Notice the error messages.

Comment 6

[Gregg Tavares]

Oh, I'm running FF4.0b11

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah.  I'm going to look into this after we ship Fx4, but not before then.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This should do the trick.  Asking smaug for review of whether I'm using the right nsIDocument bit here, just in case.  With this patch, there is still a way an animation callback could get called after its document is in paint suppression: if another callback during the same refresh spins the event loop just until the document gets there.  The bookkeeping needed to avoid that doesn't seem worthwhile, honestly.  If someone really wants to call a JS function in that state, they can just grab a function reference in JS, unload the document, then call that function; they don't need help from mozRequestAnimationFrame.

Comment 9

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

Comment on attachment 520120 [details] [diff] [review]
Proposed fix

You could just use GetScriptGlobalObject, since you don't care about the
out param and data documents can't use mozRequestAnimationFrame, right?

Comment 10

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

oh, right, GetScriptGlobalObject has still the bug that it may
get the object from the docshell.

So, yes, the patch is right.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, that's why I didn't use GetScriptGlobalObject there.

Comment 12

[Boris Zbarsky [:bzbarsky]]

The attached patch doesn't play well with bfcache.  The old state of the world doesn't either, but I think that's buggy.  I'll work on a followup to fix that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think this just needs a different approach.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Better fix

Now even with tests.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/30e38f8752c1