792280 - function.caller returns null when called from different iframe

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mahks]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

upgraded to FF 15.0.1


Actual results:

Javascript failed on reference to function.caller.name


Expected results:

returned function name

Comment 1

[Mahks]

under FF 14 same code returned function name,
under FF 14 returns NULL

Comment 2

[Mahks]

should read 
under FF 15 returns NULL

Comment 3

[Matthias Versen [:Matti]]

Please provide a testcase and steps to reproduce.

Comment 4

[Mahks]

It seems there is something that changed.
function.caller still works when the calling function is in same iframe,
but it is NULL when called from another iframe.

This has changed from v14 & previous, you used to get the caller.name even when called from another iframe.

Test case : call the following from a function in a different iframe

function test(){
    console.debug('Test caller.name '+test.caller.name)
}

This will return the caller.name in v14 but test.caller=NULL in v15

Comment 5

[Loic]

Possible to have an HTML testcase?

Comment 6

[Paul Silaghi, QA [:pauly]]

Please try to reproduce the issue using the latest FF 18 Nightly ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 8

[Boris Zbarsky [:bzbarsky]]

This is a cpg regression.  Quoting Waldo:

<Waldo> bz: hmm; /* Censor the caller if it is from another compartment. */
<Waldo> bz: that made sense pre-cpg
<Waldo> bz: ...now not so much
<Waldo> I guess you want some sort of origin/principals check
* Waldo wonders what that even looks like in pure JS code these days

Comment 9

[Loic]

Indeed:
good=2012-05-04
bad=2012-05-05
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db9df42823d&tochange=0a48e6561534

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> This is a cpg regression.  Quoting Waldo:
> 
> <Waldo> bz: hmm; /* Censor the caller if it is from another compartment. */
> <Waldo> bz: that made sense pre-cpg
> <Waldo> bz: ...now not so much
> <Waldo> I guess you want some sort of origin/principals check
> * Waldo wonders what that even looks like in pure JS code these days

We really want to do some kind of subsumes check on the origins. But luke ripped that callback out. ;-)

We could try to do a PUNCTURE on the wrapper, see if it throws, and clear the exception if it does. But really, I think we need some better API to do this. Some kind of callback that tells us whether the wrapper is allowed to be punctured (which is roughly equivalent to asking whether the compartment of the wrapper subsumes the compartment of the wrappee).

Luke, what do you think?

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> This is a cpg regression.  Quoting Waldo:

I'm not sure the case can be made that this is a critical user regression given the fact that this was first reported weeks into the cycle. Can somebody help me understand the user impact that we're worried about here? This is likely a wontfix for FF16 at this point.

Comment 12

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #10)
> We could try to do a PUNCTURE on the wrapper, see if it throws, and clear
> the exception if it does. 

Can we just call "wrap"?  I forget whether it throws or returns some sort of opaque wrapper in the case of different-origin.

Failing that, a "subsumes" predicate on compartment makes sense.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #12)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > We could try to do a PUNCTURE on the wrapper, see if it throws, and clear
> > the exception if it does. 
> 
> Can we just call "wrap"?  I forget whether it throws or returns some sort of
> opaque wrapper in the case of different-origin.

It will be an opaque wrapper. Whether that's OK or not depends entirely on the purpose of the check. Waldo, can you elaborate on whether such an opaque wrapper would be ok?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Can somebody help me understand the user impact that we're worried about here?

Some set of sites being broken.  What that set is, I can't tell you.  :(

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Bobby Holley (:bholley) from comment #13)
> It will be an opaque wrapper. Whether that's OK or not depends entirely on
> the purpose of the check. Waldo, can you elaborate on whether such an opaque
> wrapper would be ok?

Well, the previous code returned null for the cross-compartment (== cross-origin) case.  This would be returning an object instead, one the user couldn't use except in very limited ways.  That incompatibility probably means an opaque wrapper wouldn't be enough.  But it's impossible to say for sure, the web being what it is.

Comment 16

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #14)
> > Can somebody help me understand the user impact that we're worried about here?
> 
> Some set of sites being broken.  What that set is, I can't tell you.  :(

We'll track for FF17 and on, but will wontfix for FF16 given where the investigation is and the fact that peoples' heads didn't explode when FF15 was released.

Comment 17

[Bobby Holley (:bholley)]

So basically, I think we need to modify the PUNCTURE infrastructure a little bit. As background, PUNCTURE is a way to ask a wrapper whether you can safely unwrap it from a security perspective.

I originally implemented it as an |action| for the |enter()| policy enforcement trap. This worked ok, but had the nasty side effect that you couldn't ask the question without PUNCTURE throwing if you got it wrong. That led to nasty exception-munging code like this:

http://hg.mozilla.org/mozilla-central/file/c9a8f55d8541/dom/plugins/base/nsJSNPRuntime.cpp#l469

So I think we need to rework this a little bit. I'd like to move the PUNCTURE logic to a simple, separate predicate on the proxy handler itself, which we could then query (without necessarily throwing).

So, we could implement such a predicate manually. But I've been wondering if it might align well with SecurityWrapper, which we have yet to use for anything substantial. The big question here is whether SecurityWrapper is intended to be used for all security wrappers, or just ones where the wrapper does not subsume the wrappee (I'm thinking in particular here of Xrays, where we have a special security policy to protect chrome from content, but chrome should be able to unwrap the content wrapper if it wants). Luke, can you weigh in on that? If SecurityWrapper is indeed intended only for the !Subsumes(wrapper,wrappee) case, then we can just implement CanSafelyUnwrap by checking whether we're a SecurityWrapper or not.

Comment 18

[Bobby Holley (:bholley)]

Trying out this fancy new bugzilla flag.

Comment 19

[Luke Wagner [:luke]]

[Nice flag, shows up better on radar.]

SecurityWrapper was meant to be a ProxyHandler that defaulted to "no" instead of "yes".  We later had to nerf it because of document.domain sadness, but, with cpg, we could probably turn it back into a "default to no" version of Wrapper.  Other than this vague motivation, it has no more specific semantics; feel free to mold it as you see fit; you certainly understand the wrapping/privilege situation better than me.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Bobby, this is now depending on a bug that is not tracked for 17 - what is your estimate of being able to get a speculative fix here in the next two weeks (the time we have for such things on FF 17 Beta)?

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Also - assigning to Bobby as we need to have people assigned to 17 tracked bugs at this stage.

Comment 22

[Bobby Holley (:bholley)]

Bug 800915 isn't going to be suitable for landing on beta, so let's just fix this bug the ugly way for now.

https://tbpl.mozilla.org/?tree=Try&rev=add4f9d09ff3

Comment 23

[Bobby Holley (:bholley)]

Attachment: Only censor function.caller for non-same-origin calls. v1

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 672248 [details] [diff] [review]
Only censor function.caller for non-same-origin calls. v1

Makes sense.

Nit: end of comment should be:

 * NB - This will get much nicer with bug 800915
 */

Comment 25

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb12b546fad

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 672248 [details] [diff] [review]
Only censor function.caller for non-same-origin calls. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): CPG
User impact if declined: potential web breakage.
Testing completed (on m-c, etc.): Just pushed to m-i. Please sit on this approval request until the m-c bake time has happened. I'm a bit overloaded right now and I don't want to risk forgetting to nom this.
Risk to taking this patch (and alternatives if risky): Low risk 
String or UUID changes made by this patch: None

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive by - sitting on approval until this has baked on m-c a bit.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eb12b546fad

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 672248 [details] [diff] [review]
Only censor function.caller for non-same-origin calls. v1

Feel free to let this sit on m-c a little longer if you need to, but let's make sure this gets landed by Monday Oct 22nd so it's in our next Beta.

Comment 30

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f0543a41201b

Comment 31

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db1b67466326

Comment 32

[Paul Silaghi, QA [:pauly]]

Reproducible on FF 15.0.1.
Verified fixed on FF 17b5, FF 18.0a2 (2012-11-12) on Win 7.