Last Comment Bug 652401 - Push cx to stack before calling mozRequestAnimationFrame callbacks
: Push cx to stack before calling mozRequestAnimationFrame callbacks
Status: RESOLVED FIXED
[sg:moderate] dangerous if web expose...
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 All
: P1 normal (vote)
: mozilla5
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-24 01:39 PDT by Olli Pettay [:smaug]
Modified: 2012-06-08 19:01 PDT (History)
6 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
unaffected
unaffected
wontfix


Attachments
I think this is a sufficient fix for this bug. (1.97 KB, patch)
2011-04-29 13:59 PDT, Boris Zbarsky [:bz]
bugs: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2011-04-24 01:39:21 PDT
See bug 652317
Comment 1 Boris Zbarsky [:bz] 2011-04-24 13:51:53 PDT
This is extra fun because the callbacks might come from different origins, so we need to keep track of where each one came from...

I guess I get to own this one.  :(
Comment 2 Olli Pettay [:smaug] 2011-04-24 14:27:24 PDT
Isn't this just the same what event handlers have had for years
(longer than I've been hacking gecko) - use the context from the relevant window.
If the callback is coming from chrome, it gets chrome privileges anyway, *IIRC*.
Comment 3 Olli Pettay [:smaug] 2011-04-24 14:42:37 PDT
Or, hmm, do we handle all the callbacks related to a top level window in a same
place? Then when adding a new callback, take reference to the window on
which the method was called.
Comment 4 Boris Zbarsky [:bz] 2011-04-24 18:41:22 PDT
> Or, hmm, do we handle all the callbacks related to a top level window in a same
> place? 

Refresh drivers are per-tab, effectively.

We do know the document the callback comes, from, sort of.  If we try a bit.  I'll try to post a patch tomorrow.
Comment 5 Boris Zbarsky [:bz] 2011-04-28 22:06:27 PDT
So...  Does this mean that use of nsIDOMNodeFilter in the treewalker is also unsafe?
Comment 6 Boris Zbarsky [:bz] 2011-04-28 22:14:02 PDT
And also geolocation, the xpath nsresolver....

Can we seriously not do the right thing somehow in the xpconnect wrappedjs code?
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-04-28 23:02:20 PDT
nsIDOMNodeFilter and xpath resolver are always called back synchronously as a result from other js-calls. So the right context should already be on the stack.

geolocation is a different story though. As are desktop notifications (implemented on fennec).
Comment 8 Boris Zbarsky [:bz] 2011-04-28 23:19:28 PDT
> nsIDOMNodeFilter and xpath resolver are always called back synchronously

Ah, good point.

Sounds like we need another bug on geolocation and desktop notifications?  :(
Comment 9 Olli Pettay [:smaug] 2011-04-29 05:27:03 PDT
IIRC, either geolocation or desktop notifications should behave ok already.
Comment 10 Olli Pettay [:smaug] 2011-04-29 05:27:25 PDT
or both...but better to make sure.
Comment 11 Boris Zbarsky [:bz] 2011-04-29 10:53:58 PDT
OK, dunno about desktop notifications (can't find where it calls its callbacks offhand), but in bug 452762 geolocation was changed to push null precisely to address this sort of issue.  And from my reading of the XPCWrappedJSClass code, that looks correct in terms of the obvious "what else might be on the stack" issue: if there's nothing on the stack or the top thing on the stack is null, XPCWrappedJSClass will find the right JSContext for the callee object and then init an XPCCallContext with it, which will push it on the stack.

In the case of refresh driver, we can only enter Notify() in the following ways:

1)  AdvanceTimeAndRefresh called.
2)  RestoreNormalRefresh called.
3)  Timer fires.
4)  DoRefresh called.

#3 and #4 can only happen directly from the event loop, and when the event loop is spinning we're guaranteed a null on the JSContext stack (see bug 326777).

We should probably push null for #1 and #2, but that doesn't involve branches.

Are there things we need to protect against here other than "something is already on the stack"?
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2011-04-29 11:05:13 PDT
In the case of sync-xhr and alert()s, can't there be things on the JS stack even when called from the event loop?
Comment 13 Boris Zbarsky [:bz] 2011-04-29 11:07:03 PDT
OK, I talked to smaug.  He's worried about bfcache interactions when a callback spins the event loop to put one of the documents with a pending callback into bfcache, but I think the last part of bug 607529 comment 8 applies.  I don't see a security issue there.
Comment 14 Boris Zbarsky [:bz] 2011-04-29 11:08:07 PDT
Jonas, that's what bug 326777 was about.  Any time nsThread proceses the event loop it will push a null JSContext on the stack before actually calling Run() on the runnable.  Thus there will be things on the JS stack, but the _top_ thing on the stack will be null.
Comment 15 Boris Zbarsky [:bz] 2011-04-29 11:18:20 PDT
So I would like to claim there is no 4.0 or earlier issue here, and that the issue in 5.0 is limited to the APIs bug 435442 added.  Testing a patch for those now, but since they're not web-exposed I don't think this bug needs to stay security-sensitive (though maybe we should make sure about comment 10 before we open it up?).
Comment 16 Boris Zbarsky [:bz] 2011-04-29 13:59:00 PDT
Created attachment 529202 [details] [diff] [review]
I think this is a sufficient fix for this bug.

Try says this is green on our tests in a debug build, so I think we're good.
Comment 17 Daniel Veditz [:dveditz] 2011-05-03 17:24:36 PDT
(In reply to comment #15)
> issue in 5.0 is limited to the APIs bug 435442 added.  Testing a patch for
> those now, but since they're not web-exposed [...]

They're not? I thought CSS Animations were one of the new web features for Firefox 5?

If it's truly not webexposed then I suppose we can just pick it up in fx6
Comment 18 Boris Zbarsky [:bz] 2011-05-03 17:58:24 PDT
CSS Animations are exposed.  The two functions in question that live on refresh driver are only exposed via nsIDOMWindowUtils; they were added to allow testing of animations and enforce the caller having UniversalXPConnect privileges before calling into the refresh driver.  You can see the exact code at https://bug435442.bugzilla.mozilla.org/attachment.cgi?id=523829
Comment 19 Olli Pettay [:smaug] 2011-05-05 06:34:30 PDT
Comment on attachment 529202 [details] [diff] [review]
I think this is a sufficient fix for this bug.

Review of attachment 529202 [details] [diff] [review]:
Comment 20 Boris Zbarsky [:bz] 2011-05-05 13:36:38 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/285a5ec88501

I think we should probably open this bug up sometime in the near future.
Comment 21 Boris Zbarsky [:bz] 2011-05-05 13:37:22 PDT
Comment on attachment 529202 [details] [diff] [review]
I think this is a sufficient fix for this bug.

We probably should fix this on aurora, though.

This is a safe fix that would just make sure extensions can't shoot themselves in the foot.
Comment 22 Boris Zbarsky [:bz] 2011-05-06 21:29:07 PDT
Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/fc99d98f36ab

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