793969 - Should we prevent redefining valueOf on Location objects too?

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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!

Comment 5

[Daniel Veditz [:dveditz]]

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

Comment 6

[Bobby Holley (:bholley)]

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()?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I don't know.  Ask Adam Barth?

Comment 8

[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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a valueOf method to Location.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Bobby Holley (:bholley)]

Attachment: Redirect Location valueOf() calls to toString() at the Xray layer. v1

This lets as avoid an IID rev.

Comment 15

[Bobby Holley (:bholley)]

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 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Bobby Holley (:bholley)]

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

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 20

[Daniel Veditz [:dveditz]]

For testcases for historical problems we've had with Location see (in part)

bug 765527
bug 756719
bug 735073
bug 665548
bug 541530

Comment 21

[Bobby Holley (:bholley)]

(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.

Comment 22

[Bobby Holley (:bholley)]

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/

Comment 23

[Bobby Holley (:bholley)]

There we go:

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

Comment 24

[Bobby Holley (:bholley)]

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

Comment 25

[Daniel Veditz [:dveditz]]

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

Comment 26

[Bobby Holley (:bholley)]

(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.

Comment 27

[Bobby Holley (:bholley)]

Attachment: Define an identity transformation at the Xray layer. v2

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

Comment 28

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Feel free to suggest other tests here, blake.

Comment 29

[Bobby Holley (:bholley)]

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.

Comment 30

[Bobby Holley (:bholley)]

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 31

[Blake Kaplan (:mrbkap) (inactive)]

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 32

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 33

[Bobby Holley (:bholley)]

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.

Comment 34

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebdaddbe9b7b

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebdaddbe9b7b

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 37

[Bobby Holley (:bholley)]

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

Comment 38

[Matt Wobensmith [:mwobensmith][:matt:]]

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

Comment 39

[Bobby Holley (:bholley)]

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 40

[Alex Keybl [:akeybl]]

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.

Comment 41

[u279076]

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.

Comment 42

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-release/rev/7440f6c1b07e

Comment 43

[Bobby Holley (:bholley)]

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

Comment 44

[Matt Wobensmith [:mwobensmith][:matt:]]

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

Comment 45

[Matt Wobensmith [:mwobensmith][:matt:]]

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 46

[Bobby Holley (:bholley)]

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 47

[Daniel Veditz [:dveditz]]

Comment on attachment 673148 [details] [diff] [review]
Tests. v1

sec-approval+

Comment 48

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5165ee49c7db

Comment 49

[Ed Morley [:emorley]]

(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