mozRequestAnimationFrame does not run synchronously with other JavaScript code

RESOLVED FIXED in mozilla8

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: yeecheng.chin, Assigned: bz)

Tracking

Trunk
mozilla8
All
Other
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 549286 [details]
testRequestAnimationFrame.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

I was using mozRequestAnimationFrame to schedule a tick, replacing the old behavior of using setInterval/setTimeout


Actual results:

mozRequestAnimationFrame seems to have the ability to jump into currently running JavaScript code and disturbs the callstack. I have been under the impression JS/DOM code should be running in a single thread context, instead of other code being able to run until the current context has completed.

I've attached a test page. The test code schedules a callback either using setTimeout or mozRequestAnimationFrame, then makes 10 synchronous XHR calls. If I'm using setTimeout, the callback is correctly called after the current function has returned. If I'm using mozRequestAnimationFrame however, it will be called before the function has returned.

There actually seems to be a related bug in setTimeout. If you modify the attachment and modify line 72/73 by switching the SendXHR to be the alert instead, you will see that the callback is called immediately before all 10 alerts have been triggered.
This is the nested event loop stuff again.

Boris, can you dupe this to whatever bug is tracking this.
Whiteboard: DUPEME
I dunno that there's a bug on it.

We could skip animation frame callbacks when events and timeouts are suppressed, though.  That would be simple enough.  Should we, though?
Yes, I think so.
Created attachment 550012 [details] [diff] [review]
Proposed fix

Maybe I should just add a boolean tracking whether we're in a state where we should be registered with the refresh driver, so the revoking code can also move the checks into the revoke method.  Then the posting code could use that boolean too.  Anyway, let me know.
Attachment #550012 - Flags: review?(roc)
Attachment #550012 - Flags: review?(Olli.Pettay)
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
Priority: -- → P2
Product: Firefox → Core
QA Contact: general → general
Whiteboard: DUPEME → [need review]
Version: 5 Branch → Trunk

Comment 5

6 years ago
Comment on attachment 550012 [details] [diff] [review]
Proposed fix

Shouldn't you check also if timeouts are suppressed?
(or if top->mModalStateDepth is >0)
Could EventHandlingSuppressed return false when !mScriptGlobalObject?

Comment 7

6 years ago
EventHandlingSuppressed returns PRUint32 to indicate the depth of suppression.
Well, should we have a new method IsEventHandlingEnabled or something like that that checks the script global as well as EventHandlingSuppressed and whatever else is needed?
> Shouldn't you check also if timeouts are suppressed?
> (or if top->mModalStateDepth is >0)

Good question.  When are timeouts suppressed but events not suppressed?

I'd rather not mess with modal state depth unless we change everything to follow it.

> should we have a new method IsEventHandlingEnabled or something like that

We could do that, yes.  Olli, any objections?
At least in the old days modal dialogs didn't suppress event handling, since
user events would anyway go to the dialog.

> We could do that, yes.  Olli, any objections?
No objections.
> At least in the old days modal dialogs didn't suppress event handling

And they don't suppress timeouts either; see comment 0.

Patch with IsEventHandlingEnabled() coming up.
Created attachment 550189 [details] [diff] [review]
Updated to comments
Attachment #550189 - Flags: review?(roc)
Attachment #550189 - Flags: review?(Olli.Pettay)
Attachment #550012 - Attachment is obsolete: true
Attachment #550012 - Flags: review?(roc)
Attachment #550012 - Flags: review?(Olli.Pettay)
If the page is in modal state, timeouts won't run.
See nsGlobalWindow::RunTimeout.


"If you modify the attachment and modify line 72/73 by switching the SendXHR to be the alert instead, you will see that the callback is called immediately before all 10 alerts have been triggered" sounds like a bug to investigate.
Comment on attachment 550189 [details] [diff] [review]
Updated to comments

Review of attachment 550189 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that fixed

::: content/base/src/nsDocument.h
@@ +870,5 @@
> +    MaybeRescheduleAnimationFrameNotifications();
> +  }
> +
> +  bool IsEventHandlingEnabled() {
> +    return !mEventsSuppressed() && mScriptGlobalObject;

This surely doesn't compile?
Attachment #550189 - Flags: review?(roc) → review+
Perhaps we should fix comment #13 by suppressing events in non-chrome documents while there's a modal dialog up.
> sounds like a bug to investigate

For what it's worth, I can't reproduce that part.  So yeah, the modal state check seems to be working.

> This surely doesn't compile?

Indeed.  I was sure I'd compiled this, but clearly not!

Olli, how do you feel about comment 15?
Oh, I also moved IsEventHandlingEnabled to nsIDocument, since some of the callers are nsIDocument methods.
(In reply to comment #15)
> Perhaps we should fix comment #13 by suppressing events in non-chrome
> documents while there's a modal dialog up.

Why only in non-chrome? We sure want to have the same behavior in chrome.
I'm not entirely convinced we do.  I think if chrome is rendering something it wants to keep rendering it even if there's a modal alert up.  Declarative animations sure don't stop when that happens!

We need to break the web behavior due to the braindead non-reentrancy expectations websites have, for now, but in the future I fully expect that we may want to implement sync XHR in content by blocking the content thread.  That's clearly not the way to go for chrome....
What bz said.
Ok, what comment 15 says is ok

Updated

6 years ago
Attachment #550189 - Flags: review?(Olli.Pettay) → review-
So do you want me to do comment 15 in this bug too?  I was figuring that would be a separate bug, since this bug's code would automatically pick up that change, no?
ok, followup bug is ok.
So is the attached patch ok given that?  Or was there something else you wanted changed?
Comment on attachment 550189 [details] [diff] [review]
Updated to comments

r=me if you fix the patch to compile and file the followup.
Attachment #550189 - Flags: review- → review+
Filed bug 677134.
Whiteboard: [need review] → [need landing]
http://hg.mozilla.org/integration/mozilla-inbound/rev/9954f0235bc8
Flags: in-testsuite+
Whiteboard: [need landing]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/9954f0235bc8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 months ago
Blocks: 629192
You need to log in before you can comment on or make changes to this bug.