675121 - mozRequestAnimationFrame does not run synchronously with other JavaScript code

Status: RESOLVED FIXED

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

Description

[yeecheng.chin]

Attachment: 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.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is the nested event loop stuff again.

Boris, can you dupe this to whatever bug is tracking this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

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

Yes, I think so.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 5

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

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

Shouldn't you check also if timeouts are suppressed?
(or if top->mModalStateDepth is >0)

Comment 6

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

Could EventHandlingSuppressed return false when !mScriptGlobalObject?

Comment 7

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

EventHandlingSuppressed returns PRUint32 to indicate the depth of suppression.

Comment 8

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

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 10

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

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 13

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

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 14

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

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?

Comment 15

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

Perhaps we should fix comment #13 by suppressing events in non-chrome documents while there's a modal dialog up.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, I also moved IsEventHandlingEnabled to nsIDocument, since some of the callers are nsIDocument methods.

Comment 18

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

(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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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....

Comment 20

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

What bz said.

Comment 21

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

Ok, what comment 15 says is ok

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

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

ok, followup bug is ok.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So is the attached patch ok given that?  Or was there something else you wanted changed?

Comment 25

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

Comment on attachment 550189 [details] [diff] [review]
Updated to comments

r=me if you fix the patch to compile and file the followup.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Filed bug 677134.

Comment 27

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9954f0235bc8

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/9954f0235bc8