Last Comment Bug 575997 - Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)
: Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)
Status: RESOLVED FIXED
fixed-in-tracemonkey
: meta
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Brendan Eich [:brendan]
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description 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__")
  true
  js> x.__proto__ = null;
  null
  js> "__proto__" in x
  false

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 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 Boris Zbarsky [:bz] 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 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 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 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 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 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 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?

Also todo: rename JSPROP_SHARED to JSPROP_NOSLOT.

/be
Comment 9 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 Brendan Eich [:brendan] 2011-06-15 10:06:00 PDT
What does https://www.khronos.org/registry/typedarray/specs/latest/#5 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.

/be
Comment 11 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 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?

/be
Comment 13 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.

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

/be

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