Closed
Bug 575997
Opened 15 years ago
Closed 14 years ago
Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: brendan)
References
Details
(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__")
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?
Reporter | ||
Comment 1•15 years ago
|
||
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•15 years ago
|
||
Outside jseng, looks like also ctypes.
Inside jseng, what about the arguments stuff in jsfun.cpp?
Using SHARED without PERMANENT isn't an issue?
Reporter | ||
Comment 3•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Reporter | ||
Comment 7•14 years ago
|
||
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.
Assignee | ||
Comment 8•14 years ago
|
||
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
Reporter | ||
Comment 9•14 years ago
|
||
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.
Assignee | ||
Comment 10•14 years ago
|
||
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
Keywords: meta
Assignee | ||
Updated•14 years ago
|
Reporter | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
(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
Assignee | ||
Comment 13•14 years ago
|
||
Calling this fixed-in-tm.
Renaming JSPROP_SHARED to JSPROP_NOSLOT can be done in a non-blocker for this bug.
/be
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 14•14 years ago
|
||
All dependency bugs fixed.
/be
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•