Last Comment Bug 575997 - Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)
: Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)
: meta
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
-- normal (vote)
: ---
Assigned To: Brendan Eich [:brendan]
: Jason Orendorff [:jorendorff]
Depends on: 548671 570551 636989 637994
Blocks: es5
  Show dependency treegraph
Reported: 2010-06-30 09:08 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-06-20 17:33 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Description User image Jason Orendorff [:jorendorff] 2010-06-30 09:08:56 PDT
JSPROP_SHARED and JSPROP_PERMANENT combined make a property appear to be an own property of each object that inherits it.

The illusion is not quite perfect in the face of mutable __proto__:

  js> var x = {};
  js> x.hasOwnProperty("__proto__")
  js> x.__proto__ = null;
  js> "__proto__" in x

But more importantly, this hack causes us to violate ES5:

  var x = Object.create({}, {p: {get: function () { return 3; }}});
  var y = Object.create(x);
  assertEq(y.hasOwnProperty("p"), false);

Fixing this by removing the hack, as this bug proposes, depends on individually fixing everything that currently depends on it. Waldo has already checked in fixes for array.length, function.length, and some RegExp properties. __proto__ remains. Anything else?
Comment 1 User image Jason Orendorff [:jorendorff] 2010-06-30 13:33:30 PDT
Inside the engine:
  - __proto__
  - String.prototype.length
  - typed arrays
  - RegExp statics
  - E4X

I don't think RegExp and E4X need the shared-permanent hack, though: the properties in question just need to be slotless and non-configurable. When bug 552432 is fixed, these properties should no longer be JSPROP_SHARED.

__proto__ needs JSPROP_SHARED assignment behavior, but in my opinion it still doesn't need the shared permanent hack. Would it break anyone if we regressed ({}).hasOwnProperty("__proto__")? Surely not.

The other two need fixes along the lines of bug 548671.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 00:35:40 PDT
Outside jseng, looks like also ctypes.

Inside jseng, what about the arguments stuff in jsfun.cpp?

Using SHARED without PERMANENT isn't an issue?
Comment 3 User image Jason Orendorff [:jorendorff] 2010-07-01 12:09:47 PDT
Using SHARED without PERMANENT isn't an issue.

Using SHARED with PERMANENT probably isn't an issue either... unless you specifically care about whether hasOwnProperty returns true or false for that property. ctypes presumably doesn't.
Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-01 15:05:02 PDT
Typed arrays use this too, from what I remember.

The spec there is not yet defined in terms of ES5isms last I remember, so it's impossible to say what properties should and should not be own.  [0, length) U {"length"} is all Object.getOwnPropertyNames will expose initially.  Special trickery will be required if .buffer, .byteOffset, .byteLength must appear to be own (they use the hack as I recall); I don't think we're better-served by their being own than by their being getters/setters on the prototype that happen to act upon the original object.
Comment 5 User image Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-01 15:06:37 PDT
From a non-final version of the Object.getOwnPropertyNames patch, moved here as requested by jorendorff:

>+ * FIXME: The shared-permanent hack makes own-property-only enumeration
>+ *        O(protoChainLength * propsPerProto), rather than simply
>+ *        O(propsOnObject) as one would expect.  In the short run we
>+ *        find it expedient to not spend the time to eliminate the
>+ *        hack; in the (hopefully sufficiently) long run this quadratic
>+ *        behavior will come back to bite us.
Comment 6 User image Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-01 15:08:31 PDT
Oh, typed arrays were pointed out in comment 1, skimming fail.  :-)  The point that those properties currently have no reason to be own due to spec imprecision is still new, however.
Comment 7 User image Jason Orendorff [:jorendorff] 2011-02-15 09:39:51 PST
I nearly filed this as a separate bug before Waldo pointed out that it's the same bug again...

  js> var m = {};
  js> Object.getOwnPropertyDescriptor(m, '__proto__')
  ({value:{}, writable:true, enumerable:false, configurable:false})

__proto__ here claims to be an own, non-configurable property of m. But then:

  js> Object.defineProperty(m, '__proto__', {enumerable:true})
  ({__proto__:(void 0)})
  js> Object.getOwnPropertyDescriptor(m, '__proto__')
  ({value:(void 0), writable:false, enumerable:true, configurable:false})

Sigh. Can't wait for FF4 to ship so we can blow this thing out of the water.
Comment 8 User image Brendan Eich [:brendan] 2011-06-15 09:20:44 PDT
This bug is mostly fixed by the fix for bug 637994, which landed last night.

Typed arrays and ctypes need investigation. The typed arrays spec is changing, IIRC.

Anyone know of anything else that needs shared-permanent proto-properties to be moved into each instance?


Comment 9 User image Jason Orendorff [:jorendorff] 2011-06-15 09:30:32 PDT
I don't think typed arrays actually rely on the shared-permanent hack: typed array instances are a different js::Class from their prototype.

ctypes I'm not worried about at all.

Thrilled to see this go.
Comment 10 User image Brendan Eich [:brendan] 2011-06-15 10:06:00 PDT
What does read through the lens of the latest WebIDL spec (the one in Last Call) say regarding byteLength -- accessor on a prototype, or data property on each instance? IIRC it's accessor on a proto. We have work to do there, but not for this bug.

Jason, anyone: please confirm.

Comment 11 User image Jason Orendorff [:jorendorff] 2011-06-15 10:23:35 PDT
Yes, my reading of this spec and Web IDL is that both .byteLength and .length must be accessor properties on the prototype object. That is what we implement for .byteLength, but not for .length.
Comment 12 User image Brendan Eich [:brendan] 2011-06-15 12:35:48 PDT
(In reply to comment #11)
> Yes, my reading of this spec and Web IDL is that both .byteLength and
> .length must be accessor properties on the prototype object. That is what we
> implement for .byteLength, but not for .length.

Filed yet?

Comment 13 User image Brendan Eich [:brendan] 2011-06-17 17:32:26 PDT
Calling this fixed-in-tm.

Renaming JSPROP_SHARED to JSPROP_NOSLOT can be done in a non-blocker for this bug.

Comment 14 User image Brendan Eich [:brendan] 2011-06-20 17:33:17 PDT
All dependency bugs fixed.


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