Adam Barth suggested that in the spec thread I started. Opera, Safari, and WebKit certainly do that. I haven't tested IE to see what it does here.

We certainly could. Our native property resolution code doesn't check things on Object.prototype because it doesn't need to, in general - the cross-compartment callers always just get something clean when looking things up on their holder objects. But we could just special-case either valueOf or all the non-own holder properties when determining whether we're shadowing or not.

Or could we just stick a valueOf in the Location IDL and implement it in C++? Not sure what it should do, exactly...

Given the current wrapper situation, would it even be called?

I don't know. I also don't know whether the current wrapper situation will persist... Feel free to follow up on the whatwg/public-script-coord thread!

so sec-high because we (or plugins) get confused about some other window's origin?

We (browser chrome) will never be confused thanks to cross-compartment Xrays. As for plugins, do we have any reason to believe any plugins end up invoking valueOf() rather than toString()?

I don't know. Ask Adam Barth?

@bholley: We did a survey of NPAPI plugins, and a whole bunch of them try to determine the security context of their container by evaling JavaScript. It's scary stuff. I don't remember which all variations we saw, but I remember we tried to be a bit conservative.

> As for plugins, do we have any reason to believe any plugins end up invoking valueOf() They didn't use to, but now they do, yay.

So the obvious fix I know for this involves an IID rev on nsIDOMLocation. How happy are we with that on releases and betas and such? :( Bobby, if you can think of a better idea Friday morning, please do.

Alex, we need to decide what we're doing with 16 here.... :(

This lets as avoid an IID rev.

Comment on attachment 670663 [details] [diff] [review] Add a valueOf method to Location. This patch is probably the way to go for branches where we don't care about the IID rev btw.

Comment on attachment 670711 [details] [diff] [review] Redirect Location valueOf() calls to toString() at the Xray layer. v1 r=me, assuming it fixes the bug.

Over to Bobby, since he's got a better handle on this.

Awaiting instructions on how to proceed. Should I go ahead and push this to try and / or inbound?

two patches here, which were you going to push? Do we care about changing the IID for such an internal implementation detail as Location? bz's patch looks cleaner if we can take it everywhere.

For testcases for historical problems we've had with Location see (in part) bug 765527 bug 756719 bug 735073 bug 665548 bug 541530

(In reply to Daniel Veditz [:dveditz] from comment #19) > two patches here, which were you going to push? Do we care about changing > the IID for such an internal implementation detail as Location? bz's patch > looks cleaner if we can take it everywhere. Yes, but I don't think we want to rev the IID on beta. It's against policy (especially for DOM interfaces) and only likely to draw attention to the issue. If we want this anywhere outside of Nightly and Aurora, I think we should push my patch, and possibly follow up with bz's patch on Nightly.

Ok, I've been told to push to try only. I've got it ready to go, but try is closed. http://sadtrombone.vorb.is/

There we go: https://tbpl.mozilla.org/?tree=Try&rev=4b2fc0ba7fdc

That looks green, but there's a bunch of infra red cluttering things up. Let's try this again: https://tbpl.mozilla.org/?tree=Try&rev=9e26cac83f2d

Can we get a testcase patch ready to land after we've shipped this fix?

(In reply to Daniel Veditz [:dveditz] from comment #25) > Can we get a testcase patch ready to land after we've shipped this fix? I can write such a testcase when the time comes. Please ping me whenever that is. I'd like to avoid juggling too many patches whose landing is deferred for security reasons.

This is the safer approach we discussed over vidyo - just make valueOf an identity transformation.

Feel free to suggest other tests here, blake.

https://tbpl.mozilla.org/?tree=Try&rev=0ea992ee533f Once this all blows over, I'm going to back out the Xray stuff and just define this as a property on nsIDOMWindow so that this all just works naturally.

All green. Given how this week has been, I'm going to go spend the next couple of hours outside. Blake, if you don't have any significant review comments, feel free to push the fix (but not the tests) to inbound. Otherwise, I can push it this evening.

Comment on attachment 673146 [details] [diff] [review] Define an identity transformation at the Xray layer. v2 Review of attachment 673146 [details] [diff] [review]: ----------------------------------------------------------------- One question, but this looks good. ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +769,5 @@ > > +static JSBool > +IdentityValueOf(JSContext *cx, unsigned argc, jsval *vp) > +{ > + JS_SET_RVAL(cx, vp, JS_THIS(cx, vp)); Should we insist on |this| being an object (as opposed to undefined, which can happen in strict mode) so that this follows the implementation of Object.prototype.valueOf?

Comment on attachment 673146 [details] [diff] [review] Define an identity transformation at the Xray layer. v2 Review of attachment 673146 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +811,5 @@ > + { > + JSFunction *fun = JS_NewFunctionById(cx, &IdentityValueOf, 0, 0, NULL, id); > + if (!fun) > + return false; > + desc->obj = wrapper; This patch makes valueOf an own property on location. I'm OK with that (given how weird the location object is in general, one more oddity won't doesn't seem like it makes a difference) but we should probably add a comment on that here.

mrbkap and I discussed these comments on IRC. comment 32 was based on mistaken premises, and we decided to skip comment 31 for lack of an easy API to do that in XPConnect.

Some branch approval noms when you have time would be great here.

Akeybl gave me blanket approval to land this bug on all branches on monday.

Confirmed broken on build 2012-9-24, Firefox 18.0a1. Verified fixed on build 2012-10-22, Firefox 19.0a1. Mac 10.8.2, Windows 7 and Ubuntu 11.10

https://hg.mozilla.org/releases/mozilla-aurora/rev/dbdae3188556 https://hg.mozilla.org/releases/mozilla-beta/rev/2dc642d4daef https://hg.mozilla.org/releases/mozilla-esr10/rev/d8a766d176fd

Comment on attachment 673146 [details] [diff] [review] Define an identity transformation at the Xray layer. v2 This still needs to land on the FIREFOX_10_0_9esr_RELEASE branch of mozilla-esr10 as well as mozilla-release.

Matt, can you please verify if this is fixed for... latest-mozilla-aurora: ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora 17.0b3 (should be available by tomorrow morning): ftp://ftp.mozilla.org/pub/firefox/nightly/17.0b3-candidates Same test as you did in comment 38, thanks.

Pushed to esr10 relbranch: https://hg.mozilla.org/releases/mozilla-esr10/rev/246c665f50d3

Verified fixed on build 2012-10-24, 17.0b3 Verified fixed on build 2012-10-24, Aurora 18.0a2 Mac 10.8.2, Windows 7 and Ubuntu 11.10

Verified fixed on build 2012-10-24, 10.0.10esr Verified fixed on build 2012-10-24, 16.0.2 Mac 10.8.2, Windows 7 and Ubuntu 11.10

Comment on attachment 673148 [details] [diff] [review] Tests. v1 This should be fixed everywhere, so requesting approval to land the mochitest on m-i.

Comment on attachment 673148 [details] [diff] [review] Tests. v1 sec-approval+

(In reply to Daniel Veditz [:dveditz] from comment #47) > Comment on attachment 673148 [details] [diff] [review] > Tests. v1 https://hg.mozilla.org/mozilla-central/rev/5165ee49c7db