mozRequestAnimationFrame callbacks can be called after document has gone away

RESOLVED FIXED in mozilla5

Status

()

defect
P1
normal
RESOLVED FIXED
9 years ago
4 months ago

People

(Reporter: heycam, Assigned: bzbarsky)

Tracking

unspecified
mozilla5
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Posted file test
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....
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.
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.
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.
Oh, I'm running FF4.0b11
Yeah.  I'm going to look into this after we ship Fx4, but not before then.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Posted patch Proposed fix (obsolete) — Splinter Review
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.
Attachment #520120 - Flags: review?(roc)
Attachment #520120 - Flags: review?(Olli.Pettay)
Whiteboard: [need review]
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?
Attachment #520120 - Flags: review?(Olli.Pettay) → review+
oh, right, GetScriptGlobalObject has still the bug that it may
get the object from the docshell.

So, yes, the patch is right.
Yeah, that's why I didn't use GetScriptGlobalObject there.
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.
I think this just needs a different approach.
Attachment #520120 - Attachment is obsolete: true
Posted patch Better fixSplinter Review
Now even with tests.
Attachment #520206 - Flags: review?(roc)
Attachment #520206 - Flags: review?(Olli.Pettay)
Attachment #520206 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [need review] → [need landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/30e38f8752c1
Flags: in-testsuite+
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.