Last Comment Bug 607863 - Try to reimplement __proto__ to avoid JSPROP_SHARED, and with enough compatibility
: Try to reimplement __proto__ to avoid JSPROP_SHARED, and with enough compatib...
Status: ASSIGNED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 607113 607764
  Show dependency treegraph
 
Reported: 2010-10-27 20:41 PDT by Drew Willcoxon :adw
Modified: 2014-07-24 11:07 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
just throwing this together for comments (3.06 KB, patch)
2010-10-28 12:50 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
hide __proto__ for null-proto objects (3.09 KB, patch)
2010-10-28 15:01 PDT, Brendan Eich [:brendan]
jwalden+bmo: review-
Details | Diff | Splinter Review

Description Drew Willcoxon :adw 2010-10-27 20:41:50 PDT
Jetpack does this weird thing with prototypes.  I can explain if you want.  The unit test for this weird thing broke in recent Firefox nightlies.

Here's a pared-down example that still works as expected:

  var proto = {};
  function F() {}
  F.prototype = proto;
  var o = new F();
  o.__proto__ == F.prototype; /* true */

The problem is when F is inside a sandbox and proto is outside:

  var sp = Components.classes["@mozilla.org/systemprincipal;1"].
           createInstance(Components.interfaces.nsIPrincipal);
  var s = Components.utils.Sandbox(sp);
  s.proto = {};
  Components.utils.evalInSandbox(
    'function F() {}' +
    'F.prototype = proto;',
    s
  );
  var o = new s.F();
  o.__proto__ == s.F.prototype; /* false */

s.F.prototype.isPrototypeOf(o) returns true as expected though (which is maybe what the unit test should be checking instead of __proto__ anyway).  But shouldn't the sandboxed and non-sandboxed cases be consistent?

(If it matters, we're actually setting F.prototype to { __proto__: proto }.  But the issue still occurs with the simpler case above.)
Comment 1 Andreas Gal :gal 2010-10-27 23:31:24 PDT
This is no longer supported. You can construct the sandbox with a specific proto. Blake, whats the syntax?
Comment 2 Blake Kaplan (:mrbkap) 2010-10-28 00:51:24 PDT
(In reply to comment #1)
> This is no longer supported. You can construct the sandbox with a specific
> proto. Blake, whats the syntax?

This is something else. What you're thinking of is setting the sandbox's prototype to some object (to emulate that object being the global), this is the usual construct path, only with multiple compartments thrown in for good measure.
Comment 3 Blake Kaplan (:mrbkap) 2010-10-28 01:07:20 PDT
This is actually bug 596031. We lose the receiver when getting o.__proto__ when o's prototype is a proxy. Here is a relevant shell session showing the same bug:

js> var box = evalcx('')
js> var proto = { x: 42 }
js> evalcx('function F(){}', box)
js> box.F.prototype = proto
({x:42})
js> o = new box.F();
({})
js> o.__proto__ === box.F.prototype
false
js> o.x
42
js> o.__proto__ === Object.prototype
true

because looking up __proto__ on o's wrapped object finds it on a proxy, we end up calling proto->getProperty(__proto__) which returns Object.prototype.
Comment 4 Blake Kaplan (:mrbkap) 2010-10-28 01:10:59 PDT
This might be more clear:
js> var foreign = evalcx('({ get f() this })', box)
js> foreign.f
({get f () this})
js> var o = Object.create(foreign)
js> o.f
({get f () this})
Comment 5 Drew Willcoxon :adw 2010-10-28 02:17:27 PDT
Thanks Blake.  I think bug 596031 may be causing bug 607764 too.  There, a method on a prototype is being called, but the this-argument is apparently not the expected object.
Comment 6 Brendan Eich [:brendan] 2010-10-28 12:15:38 PDT
(In reply to comment #4)
> This might be more clear:
> js> var foreign = evalcx('({ get f() this })', box)
> js> foreign.f
> ({get f () this})
> js> var o = Object.create(foreign)
> js> o.f
> ({get f () this})

That is indeed what ES5 + proxies requires for an accessor property, but the issue here is specific: __proto__. So one fix is to make __proto__ not be a JSPROP_SHARED native (JSPropertyOp) accessor.

/be
Comment 7 Blake Kaplan (:mrbkap) 2010-10-28 12:37:45 PDT
I don't understand. __proto__ doesn't appear anywhere in the second testcase.
Comment 8 Brendan Eich [:brendan] 2010-10-28 12:50:43 PDT
Created attachment 486709 [details] [diff] [review]
just throwing this together for comments

Seems to work, still testing.

/be
Comment 9 Brendan Eich [:brendan] 2010-10-28 12:59:52 PDT
Someone nominate and fix the TM if this is needed for b7.

jit-tests just passed, trying full jsreftests.

/be
Comment 10 Andreas Gal :gal 2010-10-28 14:01:08 PDT
If jetpack is blocking b7, so should all jetpack related bugs.
Comment 11 Brendan Eich [:brendan] 2010-10-28 15:01:54 PDT
Created attachment 486754 [details] [diff] [review]
hide __proto__ for null-proto objects

I think mrbkap is going to vindicate jorendorff's bug 596031 and write a better patch, but I'll finish jsreftest'ing this and leave it here for posterity. If anyone thinks it is the better way to implement __proto__, ignoring mrbkap's more general "pass the right receiver" patch, it can be spun out into its own bug.

/be
Comment 12 Brendan Eich [:brendan] 2010-10-28 15:04:18 PDT
Or, mrbkap: leave my patch in this bug, assigned to me (maybe Waldo has thoughts) and put your patch in reopened and fully vindicated operational jorendorff death star bug 596031 ?

/be
Comment 13 Brendan Eich [:brendan] 2010-10-28 15:13:37 PDT
Comment on attachment 486754 [details] [diff] [review]
hide __proto__ for null-proto objects

Waldo, interested in your thoughts here. This patch means there is no Object.prototype.__proto__ any longer, so ecma/extensions/15.2.4.js needs to be adjusted or obsoleted.

This also means that non-native objects that *do* delegate to Object.prototype no longer inherit __proto__ from Object.prototype. Reviewing non-natives:

* E4X is a don't-care anyway, but its property model is screwed up and __proto__ can't be used to get at the Object.prototype shared-permanent property that this patch removes.

* Dense arrays have to handle __proto__ already (array_getProperty).

* No other non-natives left at this point, besides proxies. And proxies do not delegate up any proto chain outside their handler. The handler's traps handle all delegation, if any.

* I'm so glad LiveConnect is dead!

/be
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2010-10-28 17:28:15 PDT
Comment on attachment 486754 [details] [diff] [review]
hide __proto__ for null-proto objects

(Warning: essay ahead!)

Reviewing the ways __proto__ is implemented in browsers, or would be with this patch:

First, there's our current way: as a shared, non-configurable property on Object.prototype with accessor-like behavior that appears own on nearly every object that hasn't severed its [[Prototype]] chain.  Our current way makes __proto__ a completely magical property: you can't make it a regular property unless you sever, which may well be undesirable for some uses.

Second, there's your way of hacking it as a first step of property lookup for any lookup on an object with a non-null [[Prototype]], intercepting [[Get]]s and [[Put]]s before any effort happens.  __proto__ thus remains completely magical for any object which hasn't severed its [[Prototype]] chain.

Third, there's the Nitro way, which differs from your way here in that in the [[Get]] case, the lookup happens only after a real lookup that finds nothing.  (I think that can only happen via Object.defineProperty, which we would reject now because of shared-permanence making __proto__ appear own, at least outside of highly contrived cases where you have a non-native object between the "definee" and Object.prototype.  They emulate us and make { __proto__: o } set the [[Prototype]], versus making it a real property.)

Fourth, there's the v8 way: __proto__ is always a magical property and can't be defined with Object.defineProperty, even if there is no [[Prototype]].

***

I don't think anyone's happy with our current way.

With your way, __proto__ is always magical on any object with a [[Prototype]].  So if you want a real __proto__ property, you *must* sever your [[Prototype]] chain.  It gets rid of the special property on Object.prototype, a laudable accomplishment, but it's still broken somewhat in that a real __proto__ property won't appear as you'd expect.

I was originally thinking that if we were going to change, the v8/Nitro way would be the way to go, in the interests of comity and consistency.  But it has its own problems because it handles __proto__ *before* lookup in the [[Put]] case.  These two examples demonstrate the problems of this.

  function examine(o)
  {
    var gOPD = Object.getOwnPropertyDescriptor;
    alert("__proto__: " + o.__proto__ + "\n" +
          "desc: "      + JSON.stringify(gOPD(o, "__proto__")) + "\n" +
          "[[Proto]]: " + Object.getPrototypeOf(o));
  }
  var o =
    Object.defineProperty({}, "__proto__", { value: 17, writable: true });
  examine(o);
  o.__proto__ = { toString: function() { return "custom"; } };
  examine(o);

The first alert is as you'd expect: o.__proto__ is 17, the descriptor's as specified, and [[Proto]] is Object.prototype.  But the second is bizarre: o.__proto__ is *still* 17, the descriptor is still what it was, and the [[Proto]] has *changed*.  So while the v8/Nitro way purports to allow non-special __proto__, in the end it really doesn't, except maybe if you only Object.defineProperty it.

(What if the defined __proto__ is an accessor?  For whatever reason the first time through in Nitro o.__proto__ is undefined, the descriptor is {writable:false, enumerable:true, configurable: true} [NB: *no* value field], [[Proto]] is an object [probably Object.prototype], and setCalls is 0.  The second time through is the same except [[Proto]] is the custom object.  I suspect this weirdness is unrelated to the method of __proto__ implementation, but I mention it for completeness.)

The v8 way essentially blacklists __proto__ from definition at all.

So in conclusion, there's no consistency, everybody does it observably differently, and I guess this means that we can do just about anything we want.

***

I tend to think that if we can't yet remove __proto__ entirely, permitting real __proto__ properties even on objects with [[Prototype]]s is a good thing.  What we do now doesn't permit it, your patch doesn't permit it, Nitro buggily permits it, and v8 doesn't permit it.  So none of the ideas on the table would support this.

But what if we took the Nitro way for [[Get]] and did the analogous way for [[Put]] (with your tweak of only doing anything special for objects with [[Prototype]]s)?  In other words, what if we first tried the property lookup for __proto__ when setting, and if we found nothing and the object was prototype-less *then* we did the magical __proto__ behavior?  That would fix the Nitro weirdness in the first example.  It'd make __proto__ overridable for objects with prototypes, too.  Can you see a reason why this isn't marginally better behavior than what you implement?

***

Unrelated to philosophical what's-right questions, you've added an exit path to js_SetPropertyHelper that doesn't notify the tracer, which guarantees crashes and bad behavior (see the comment atop the method).  So if we decided to go with your implementation technique, this patch as written isn't ready.
Comment 15 Brendan Eich [:brendan] 2010-10-28 21:06:06 PDT
> So in conclusion, there's no consistency, everybody does it observably
> differently, and I guess this means that we can do just about anything we want.

This does not follow -- we can't do "just about anything we want" if we are trying to interoperate.

There's a case for intersection semantics, since anyone using __proto__ cross-browser can't rely on more than that.

There's a stronger case for not fiddling with __proto__ just now, but it may be worth the JSPROP_SHARED reduction since that attribute is still hurting our ES5 effort.

I looked at Rhino, which specializes for __proto__ (and __parent__) at compile time. Yet another way, but not something that can be overridden. Overriding is not a goal, given the history here. The way to override is to remove __proto__ from a future opt-in version altogether, provided there are sweet-enough upgrade paths for users to port to (Object.create isn't enough).

Good point about the js_SetPropertyHelper bail-out. We want to avoid tracing the post-order effects, but jorendorff's patch is still bit-rotting. :-(

I think this bug can't take effort now unless there's a way to help remove a use of JSPROP_SHARED that doesn't risk breaking __proto__ intersection (at least) or SpiderMonkey (at most, obviously!) compatibility. Anything else is making another compatibility headache and risking breaking content.

I'll give it more thought. Thanks for the analysis of other engines (V8 and JSC seem different enough that they ought not be lumped together).

/be
Comment 16 Brendan Eich [:brendan] 2010-10-28 22:25:43 PDT
mrbkap's patch for bug 596031 fixes the original symptom reported here, and this bug has morphed.

/be
Comment 17 David Mandelin [:dmandelin] 2010-12-03 17:48:04 PST
Minusing per:

(In reply to comment #15)
> I think this bug can't take effort now unless there's a way to help remove a
> use of JSPROP_SHARED that doesn't risk breaking __proto__ intersection (at
> least) or SpiderMonkey (at most, obviously!) compatibility. Anything else is
> making another compatibility headache and risking breaking content.
Comment 18 Brendan Eich [:brendan] 2010-12-30 00:01:45 PST
Throwing back into the pool. Tempting to compile these things as Rhino does, and not recognize o[x] for x = '__proto__' at runtime. Intersection semantics may be informative on this indexed-access-needed question.

/be
Comment 19 Andreas Gal :gal 2010-12-30 00:18:34 PST
What exactly does Rhino do here?
Comment 20 Andreas Gal :gal 2010-12-30 02:32:16 PST
In particular, how is this communicated to the API layers, internal and external?

Lets assume we add a JSOP_GETPROTO and JSOP_SETPROTO, what happens to non-native objects? Is this visible through object ops and proxies? Or do we just ignore the set for non-native objects?

Wrapper code uses __proto__ in non-trivial ways (i.e. for sandboxes that get a window as proto).

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