Last Comment Bug 719841 - Do not innerize an object assigned to __proto__
: Do not innerize an object assigned to __proto__
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 696489 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-20 07:56 PST by Jason Orendorff [:jorendorff]
Modified: 2013-12-19 14:28 PST (History)
7 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.19 KB, patch)
2012-01-20 08:36 PST, Jason Orendorff [:jorendorff]
mrbkap: review+
Details | Diff | Splinter Review
Bug 719841 - Assert that inner objects don't appear in prototype chains. v1 (1.93 KB, patch)
2012-01-20 09:01 PST, Bobby Holley (:bholley) (busy with Stylo)
jorendorff: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-01-20 07:56:54 PST
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] 2012-01-20 08:36:07 PST
Created attachment 590211 [details] [diff] [review]
v1
Comment 2 Luke Wagner [:luke] 2012-01-20 08:45:09 PST
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) 2012-01-20 08:59:51 PST
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) (busy with Stylo) 2012-01-20 09:01:23 PST
Created attachment 590220 [details] [diff] [review]
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) (busy with Stylo) 2012-01-20 09:13:46 PST
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] 2012-01-20 09:40:11 PST
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) (busy with Stylo) 2012-01-20 10:26:41 PST
Pushed this all to try: https://tbpl.mozilla.org/?tree=Try&rev=3ae11f88ae67
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-01-20 13:34:06 PST
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 9 Bobby Holley (:bholley) (busy with Stylo) 2012-01-20 13:35:49 PST
*** Bug 696489 has been marked as a duplicate of this bug. ***
Comment 10 Geoff Lankow (:darktrojan) 2012-01-20 14:31:50 PST
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) (busy with Stylo) 2012-01-20 14:48:31 PST
(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) (busy with Stylo) 2012-01-20 15:18:48 PST
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 (⌚ UTC+1/+2) 2012-01-21 04:45:03 PST
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] 2012-01-23 13:28:17 PST
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] 2012-01-23 13:29:58 PST
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] 2012-01-23 13:36:48 PST
(In reply to Jason Orendorff [:jorendorff] from comment #15)
Yes, that sounds much nicer.
Comment 17 Jason Orendorff [:jorendorff] 2012-01-23 16:23:39 PST
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) (busy with Stylo) 2012-01-23 23:13:06 PST
(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] 2012-01-24 10:37:45 PST
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) 2012-01-24 11:56:59 PST
(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] 2012-01-24 15:50:33 PST
Try server: https://tbpl.mozilla.org/?tree=Try&rev=d25d1162116d
Comment 22 Jason Orendorff [:jorendorff] 2012-01-25 14:35:10 PST
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] 2012-01-27 12:18:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/88145df4191a
Comment 24 Joe Drew (not getting mail) 2012-01-28 20:01:54 PST
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] 2012-02-02 05:31:12 PST
Trying again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8303c3a060
Comment 26 Ed Morley [:emorley] 2012-02-03 11:42:12 PST
https://hg.mozilla.org/mozilla-central/rev/8e8303c3a060
Comment 27 Jason Orendorff [:jorendorff] 2013-12-19 14:28:06 PST
Updated the split objects page as proposed in comment 5.

Note You need to log in before you can comment on or make changes to this bug.