719841 - Do not innerize an object assigned to __proto__

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

https://developer.mozilla.org/en/Split_object sayeth:
> If JavaScript code explicitly tries to set an object's __proto__ to an
> outer object, it is set to the corresponding inner object instead. (A
> comment in the source tries to explain this, but it's greek to
> me. Something about with statements. mrbkap wrote the comment in a
> patch for bug 317250 but can't remember what it means.)

Here is the code:

    if (pobj) {
        /*
         * Innerize pobj here to avoid sticking unwanted properties on the
         * outer object. This ensures that any with statements only grant
         * access to the inner object.
         */
        OBJ_TO_INNER_OBJECT(cx, pobj);
        if (!pobj)
            return JS_FALSE;
    }

I confirmed with mrbkap this morning that he doesn't remember what all that was about.

      <mrbkap> jorendorff: Well, we always had a bunch of bugs with
               property duplication between the outer and inner windows.
      <mrbkap> jorendorff: any time someone touched the outer window, I
               held my breath.
      <mrbkap> jorendorff: it's *so* *much* better now with proxies.
  <jorendorff> wait but - the outer window was the one people were
               allowed to touch
  <jorendorff> you just basically always held your breath :)
      <mrbkap> Yep :p

Things are better now. Object.create(window) does not have this hack. Webkit does not duplicate the hack. It is time to give it up at last.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Luke Wagner [:luke]]

Further supporting this decision: I thought the invariant was "if JS can stick it in a variable, it must be an outer window, if it's on a scope chain, it's an inner window" and, since the __proto__ getter doesn't outerize, this definitely seems like a way to break the invariant.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 590211 [details] [diff] [review]
v1

If I'm remembering correctly, I think the problem that we were trying to work around was that variables and other things that used JS_DefineProperty(JS_PropertyStub, JS_PropertyStub) were never correctly forwarded... And for some reason with statements and having outer windows on the prototype exacerbated the problem.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Bug 719841 - Assert that inner objects don't appear in prototype chains. v1

And for good measure. Flagging jorendorff.

Also, this should all land with the test from bug 696489.

Comment 5

[Bobby Holley (:bholley)]

Once this lands, the following lines will need to be changed on https://developer.mozilla.org/en/Split_object:

> (Note: The preceding is slightly inaccurate in that inner objects are also allowed
> to appear in objects' prototype chains. Per jst, that may be an XPConnect-ism,
> where we need to share the XPConnect prototypes between the inner window and the
> outer window.)

and

> If JavaScript code explicitly tries to set an object's __proto__ to an
> outer object, it is set to the corresponding inner object instead. (A
> comment in the source tries to explain this, but it's greek to
> me. Something about with statements. mrbkap wrote the comment in a
> patch for bug 317250 but can't remember what it means.)

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 590220 [details] [diff] [review]
Bug 719841 - Assert that inner objects don't appear in prototype chains. v1

I think this is fine, but add the test case.

Comment 7

[Bobby Holley (:bholley)]

Pushed this all to try: https://tbpl.mozilla.org/?tree=Try&rev=3ae11f88ae67

Comment 8

[Bobby Holley (:bholley)]

This hit some sort of segfault on some of the try builders unpacking the dictionaries, but that seems unrelated. Looks green otherwise.

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2059cef905c4
http://hg.mozilla.org/integration/mozilla-inbound/rev/febc999231e4
http://hg.mozilla.org/integration/mozilla-inbound/rev/86da174de8df

Comment 10

[Geoff Lankow (:darktrojan)]

Sorry, it's broken. Backing out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/26678d3cbf83
https://hg.mozilla.org/integration/mozilla-inbound/rev/8689400e67a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/13b29452a036

(In reply to Bobby Holley (:bholley) from comment #8)
> This hit some sort of segfault on some of the try builders unpacking the
> dictionaries, but that seems unrelated. Looks green otherwise.

BTW, it's segfaulting at run-mozilla.sh, which includes your code :)

Comment 11

[Bobby Holley (:bholley)]

(In reply to Geoff Lankow (:darktrojan) from comment #10)
> BTW, it's segfaulting at run-mozilla.sh, which includes your code :)

Whoops, my bad. There was some simultaneous across-the-board infra-red on standard8's push on inbound, so I just figured it was that. Looking into it now. :-)

Comment 12

[Bobby Holley (:bholley)]

Oh, duh. s/JS_ASSERT(proto->foo)/JS_ASSERT_IF(proto, proto->foo)/g

Pushed to try again, debug builds only this time: https://tbpl.mozilla.org/?tree=Try&rev=78dde0eda03e

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Which, as it happens, tickles your assertion:

Assertion failure: !proto->getClass()->ext.outerObject, at ../../../js/src/jsinferinlines.h:1154

from, apparently, js::WithObject::create.

Comment 14

[Jason Orendorff [:jorendorff]]

Hmm. The test that triggers this is basically just:

try
{
    with (this) {
        throw <x/>;
    }
}
catch(ex)
{
}

We used to have a way to test this from the shell. You could pass the -z option and you would get a split object for the global. But the last time we refactored all that stuff, it basically got disabled; at the moment it's dead code. cdleary says it shouldn't be hard to re-enable.

Comment 15

[Jason Orendorff [:jorendorff]]

Just thinking out loud, another way to re-enable would be to hack newGlobal() so that newGlobal('split') returns a split global; then you could do:

  g = newGlobal('split');
  g.eval("with (this) { throw <x/>; }");

This seems a little nicer than a command-line option to me.

Comment 16

[Luke Wagner [:luke]]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
Yes, that sounds much nicer.

Comment 17

[Jason Orendorff [:jorendorff]]

mrbkap pointed out that the shell split-object stuff is out of date, now that the outer window in the browser is a proxy. I'd have to look at it and see if it's close enough or what... it might be better to reimplement that stuff from scratch.

For now, recommend building the browser to debug this; if bholley can't take it, I'll take.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #17)
> For now, recommend building the browser to debug this; if bholley can't take
> it, I'll take.

I'm up to my neck in DOM binding stuff at the moment, so this would be most awesome. :-)

Comment 19

[Jason Orendorff [:jorendorff]]

Oh, this occurs because we innerize in EnterWith.

  * If the expression of a with statement evaluates to an outer object, the
    corresponding inner object is used instead.

You know... I'm doubling down. Innerizing is observably nonstandard behavior there too (!) so I'm going to delete that code and see where we stand.

mrbkap, what do you think? Wrappers got our back, right?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Jason Orendorff [:jorendorff] from comment #19)
> You know... I'm doubling down. Innerizing is observably nonstandard behavior
> there too (!) so I'm going to delete that code and see where we stand.
> 
> mrbkap, what do you think? Wrappers got our back, right?

I'd already assumed that this was what you're doing.

Comment 21

[Jason Orendorff [:jorendorff]]

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

Comment 22

[Jason Orendorff [:jorendorff]]

That wasn't the right version of the patch, d'oh.

Try server: https://tbpl.mozilla.org/?tree=Try&rev=26c393cff410

Comment 23

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88145df4191a

Comment 24

[Joe Drew (not getting mail)]

Philor backed bug 695922, bug 678086, and bug 719841 out of mozilla-inbound for causing Tp crashes.
https://hg.mozilla.org/mozilla-central/rev/117f2280bd37

Comment 25

[Jason Orendorff [:jorendorff]]

Trying again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8303c3a060

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e8303c3a060

Comment 27

[Jason Orendorff [:jorendorff]]

Updated the split objects page as proposed in comment 5.