Closed Bug 575997 Opened 11 years ago Closed 10 years ago

Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)


(Core :: JavaScript Engine, defect)

Other Branch
Not set





(Reporter: jorendorff, Assigned: brendan)



(Keywords: meta, Whiteboard: fixed-in-tracemonkey)

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?
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.
Outside jseng, looks like also ctypes.

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

Using SHARED without PERMANENT isn't an issue?
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.
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.
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.
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.
Blocks: es5
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.
Blocks: 637994
Blocks: 636989
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?


Assignee: general → brendan
No longer blocks: 637994
Depends on: 637994
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.
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.

Keywords: meta
No longer blocks: 636989
Depends on: 636989
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.
(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?

Calling this fixed-in-tm.

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

Whiteboard: fixed-in-tracemonkey
All dependency bugs fixed.

Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.