575997 - Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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]]

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 [:bzbarsky]]

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]]

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]]

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]]

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]]

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]]

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]]

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]]

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]]

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]]

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]]

(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]]

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]]

All dependency bugs fixed.

/be