Last Comment Bug 675121 - mozRequestAnimationFrame does not run synchronously with other JavaScript code
: mozRequestAnimationFrame does not run synchronously with other JavaScript code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All Other
: P2 normal (vote)
: mozilla8
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-28 18:22 PDT by yeecheng.chin
Modified: 2011-08-08 10:39 PDT (History)
7 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testRequestAnimationFrame.html (2.59 KB, text/plain)
2011-07-28 18:22 PDT, yeecheng.chin
no flags Details
Proposed fix (10.26 KB, patch)
2011-08-01 22:49 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Updated to comments (10.25 KB, patch)
2011-08-02 13:49 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
bugs: review+
roc: review+
Details | Diff | Review

Description yeecheng.chin 2011-07-28 18:22:42 PDT
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.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-28 19:49:27 PDT
This is the nested event loop stuff again.

Boris, can you dupe this to whatever bug is tracking this.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-28 20:23:51 PDT
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) (Exited; email my personal email if necessary) 2011-07-31 22:48:18 PDT
Yes, I think so.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-01 22:49:23 PDT
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.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-02 01:26:14 PDT
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) (Exited; email my personal email if necessary) 2011-08-02 02:58:01 PDT
Could EventHandlingSuppressed return false when !mScriptGlobalObject?
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-02 03:02:28 PDT
EventHandlingSuppressed returns PRUint32 to indicate the depth of suppression.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-02 03:42:56 PDT
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 [:bz] (Out June 25-July 6) 2011-08-02 06:27:08 PDT
> 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] (high review load, please consider other reviewers) 2011-08-02 06:29:51 PDT
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 [:bz] (Out June 25-July 6) 2011-08-02 13:45:46 PDT
> 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 [:bz] (Out June 25-July 6) 2011-08-02 13:49:26 PDT
Created attachment 550189 [details] [diff] [review]
Updated to comments
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-02 15:06:24 PDT
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) (Exited; email my personal email if necessary) 2011-08-02 15:27:46 PDT
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) (Exited; email my personal email if necessary) 2011-08-02 15:31:22 PDT
Perhaps we should fix comment #13 by suppressing events in non-chrome documents while there's a modal dialog up.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-02 21:20:21 PDT
> 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 [:bz] (Out June 25-July 6) 2011-08-02 21:23:23 PDT
Oh, I also moved IsEventHandlingEnabled to nsIDocument, since some of the callers are nsIDocument methods.
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-03 01:44:50 PDT
(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 [:bz] (Out June 25-July 6) 2011-08-03 08:05:42 PDT
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) (Exited; email my personal email if necessary) 2011-08-03 18:04:01 PDT
What bz said.
Comment 21 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-05 04:07:54 PDT
Ok, what comment 15 says is ok
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-05 07:58:14 PDT
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] (high review load, please consider other reviewers) 2011-08-05 08:58:57 PDT
ok, followup bug is ok.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-05 09:03:40 PDT
So is the attached patch ok given that?  Or was there something else you wanted changed?
Comment 25 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-05 09:03:46 PDT
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 [:bz] (Out June 25-July 6) 2011-08-07 19:25:58 PDT
Filed bug 677134.
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-07 22:05:15 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/9954f0235bc8
Comment 28 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-08 05:49:07 PDT
http://hg.mozilla.org/mozilla-central/rev/9954f0235bc8

Note You need to log in before you can comment on or make changes to this bug.