652401 - Push cx to stack before calling mozRequestAnimationFrame callbacks

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

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

See bug 652317

Comment 1

[Boris Zbarsky [:bzbarsky]]

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][[e-mail]]]

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][[e-mail]]]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

So...  Does this mean that use of nsIDOMNodeFilter in the treewalker is also unsafe?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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) No longer reading bugmail consistently]

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 [:bzbarsky]]

> 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][[e-mail]]]

IIRC, either geolocation or desktop notifications should behave ok already.

Comment 10

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

or both...but better to make sure.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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) No longer reading bugmail consistently]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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

(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 [:bzbarsky]]

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][[e-mail]]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/fc99d98f36ab