825199 - Self host __defineGetter__ and friends

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Bruant]

I'm referring to:
* Object.prototype.__defineGetter__(name, f)
* Object.prototype.__defineSetter__(name, f)
* Object.prototype.__lookupGetter__(name)
* Object.prototype.__lookupSetter__(name)

I would love to see these removed (bug 647423), but I doubt it'll be possible. Self-hosting would end up being a safe idea for security and maybe performance
It will likely be a short function around Object.defineProperty or Object.getOwnPropertyDescriptor.

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: wip patch

I was just giving this a shot. All the methods are not yet implemented and there are parts of dead code to be removed as well.

Bad thing is after I build with this patch, the js shell fails to start with a segmentation fault. Here is what I got in GDB.

Program received signal SIGSEGV, Segmentation fault.
defineProperty (attrs=1, setter=
    0x8141880 <JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<int>, int, JS::MutableHandle<JS::Value>)>, getter=
    0x8141870 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<int>, JS::MutableHandle<JS::Value>)>, value=
    $jsval((JSObject *) 0xb6529010 [object Function "__defineGetter__"]), name="Object_defineGetter", obj=0x0, cx=0xb7a3d160)
    at /home/sankha/mozilla-central/js/src/jsobj.h:809
809	        return defineGeneric(cx, obj, id, value, getter, setter, attrs);

Till, can you take a look?

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 771731 [details] [diff] [review]
wip patch

Review of attachment 771731 [details] [diff] [review]:
-----------------------------------------------------------------

So what's happening here is that the Object methods get initialized very early during a Global's setup. Way earlier than the object that holds references to the self-hosting intrinsics and all values cloned over from the self-hosting compartment (called "intrinsicsHolder"). In jsapi.cpp:4981-4983, that object is used to store the uncloned function's husk.

Why is this needed? Because some self-hosted functions are called not only through the reference they're installed under in the Global (in this case, Object.prototype.__defineGetter__ and friends), but also by other self-hosted code. For that to work, they have to be available in the intrinsicsHolder.

The only way I can think of to solve this conflict in initialization order is to move the initialization of these values *after* that of the intrinsicsHolder. Try if moving them into their own JSFunctionSpec[] and calling JS_DefineFunctions(cx, objectProto, yourNewFS) after initialization of the intrinsicsHolder, i.e. after line 399, works.

::: js/src/builtin/Object.cpp
@@ +981,5 @@
>      JS_FN(js_hasOwnProperty_str,       obj_hasOwnProperty,          1,0),
>      JS_FN(js_isPrototypeOf_str,        obj_isPrototypeOf,           1,0),
>      JS_FN(js_propertyIsEnumerable_str, obj_propertyIsEnumerable,    1,0),
>  #if OLD_GETTER_SETTER_METHODS
> +    {js_defineGetter_str, {NULL, NULL}, 2, 0, "Object_defineGetter"},

Nit: align fields here, as in array_methods.

::: js/src/builtin/Object.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function Object_defineGetter (name, f) {

nit: no space between function name and opening brace.

Comment 3

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

As talked on IRC, this patch works, but has failure on the following tests:

js/src/jit-test/tests/auto-regress/bug488421.js
js/src/jit-test/tests/auto-regress/bug490191.js
js/src/jit-test/tests/auto-regress/bug521163.js
js/src/jit-test/tests/auto-regress/bug560796.js
js/src/jit-test/tests/auto-regress/bug580699.js
js/src/jit-test/tests/auto-regress/bug600128.js
js/src/jit-test/tests/auto-regress/bug653789.js
js/src/jit-test/tests/basic/bug593611.js
js/src/jit-test/tests/basic/bug641229.js
js/src/jit-test/tests/basic/testBug628564.js
js/src/jit-test/tests/pic/bug558616.js

Comment 4

[André Bargull [:anba]]

__define[GS]etter__ and __lookup[GS]etter__ have different semantics compared to Object.defineProperty() for |thisValue|. IIRC these definitions should be compatible to the current SpiderMonkey behaviour: https://github.com/anba/es6draft/blob/master/src/main/scripts/compat.js#L34-L68.

Comment 5

[André Bargull [:anba]]

Comment on attachment 822761 [details] [diff] [review]
patch v2

Review of attachment 822761 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.js
@@ +9,5 @@
> +        ThrowError(JSMSG_NOT_FUNCTION, DecompileArg(0, f));
> +
> +    var desc = { configurable: true,
> +                 enumerable: true,
> +                 get: f };

`Object.create(null)` instead of object literals for the property descriptor, otherwise accessor properties on `Object.prototype` can cause havoc. See for example InitLegacyIterators() in builtin/Iterator.js.

@@ +21,5 @@
> +        ThrowError(JSMSG_NOT_FUNCTION, DecompileArg(0, f));
> +
> +    var desc = { configurable: true,
> +                 enumerable: true,
> +                 set: f };

`Object.create(null)` here too.

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 822761 [details] [diff] [review]
patch v2

Review of attachment 822761 [details] [diff] [review]:
-----------------------------------------------------------------

So sorry for the long delay!

This mostly looks good, but a few things need doing, still:

Please also replace obj_lookup{G,S}etter, and remove the C++ implementations of all these functions.

Wrap all the relevant code in `#ifdef JS_OLD_GETTER_SETTER_METHODS` blocks, both in the C++ files and in the JS file.

Then there's a bit of feedback to be addressed, most importantly the special handling of the global object as `this`. For that, you can use `global`, which always refers to the current global object.

::: js/src/Makefile.in
@@ +663,5 @@
>    $(srcdir)/builtin/ParallelArray.js \
>    $(srcdir)/builtin/String.js \
>    $(srcdir)/builtin/Set.js \
>    $(srcdir)/builtin/TypedObject.js \
> +  $(srcdir)/builtin/Object.js \

Please move this up to come immediately after Utilities.js. Object *is* fundamental, after all.

::: js/src/builtin/Object.cpp
@@ +983,4 @@
>      JS_FN(js_defineGetter_str,         js::obj_defineGetter,        2,0),
>      JS_FN(js_defineSetter_str,         js::obj_defineSetter,        2,0),
>      JS_FN(js_lookupGetter_str,         obj_lookupGetter,            1,0),
>      JS_FN(js_lookupSetter_str,         obj_lookupSetter,            1,0),

Remove all of these.

@@ +988,5 @@
>      JS_FS_END
>  };
>  
> +const JSFunctionSpec js::object_selfhosted_methods[] = {
> +    JS_SELF_HOSTED_FN(js_defineGetter_str, "Object_defineGetter",   2,0),

Wrap these two in an `#ifdef JS_OLD_GETTER_SETTER_METHODS` block.

::: js/src/builtin/Object.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function Object_defineGetter(name, f){

Wrap all these functions in an `#ifdef JS_OLD_GETTER_SETTER_METHODS` block.

@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function Object_defineGetter(name, f){
> +    var O = this;

As André says, this has to accomodate accessors on the global.

@@ +5,5 @@
> +function Object_defineGetter(name, f){
> +    var O = this;
> +
> +    if (!IsCallable(f))
> +        ThrowError(JSMSG_NOT_FUNCTION, DecompileArg(0, f));

Can you throw the same error as the current C++ implementation does? I.e., JSMSG_BAD_GETTER_OR_SETTER.

@@ +9,5 @@
> +        ThrowError(JSMSG_NOT_FUNCTION, DecompileArg(0, f));
> +
> +    var desc = { configurable: true,
> +                 enumerable: true,
> +                 get: f };

I don't agree: Defining an object literal doesn't invoke any accessors, so this is safe.

@@ +17,5 @@
> +function Object_defineSetter(name, f){
> +    var O = this;
> +
> +    if (!IsCallable(f))
> +        ThrowError(JSMSG_NOT_FUNCTION, DecompileArg(0, f));

Wrong error message here, too.

@@ +23,5 @@
> +    var desc = { configurable: true,
> +                 enumerable: true,
> +                 set: f };
> +    std_Object_defineProperty(O, name, desc);
> +}
\ No newline at end of file

Missing newline at file end.

Comment 7

[Tom S]

Attachment: v3 selfhost define{G,S}etter/lookup{G,S}etter

It doesn't look like you can use #ifdef JS_OLD_GETTER_SETTER_METHODS in JS code.

Comment 8

[Tom S]

Attachment: v4 selfhost define{G,S}etter/lookup{G,S}etter

I fixed two bugs, when somebody fuzzes with Object.prototype.get.

Comment 9

[Tom S]

Attachment: v4 selfhost define{G,S}etter/lookup{G,S}etter

Same patch, but with the tests included.
{set a(){}}.__lookupGetter__ should return undefined, even with Object.prototype.get.
__defineSetter__ should not define a getter, even when Object.prototype.get exists.

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8533134 [details] [diff] [review]
v4 selfhost define{G,S}etter/lookup{G,S}etter

Review of attachment 8533134 [details] [diff] [review]:
-----------------------------------------------------------------

Still very nice!

::: js/src/builtin/Object.js
@@ +128,5 @@
> +            return desc.get;
> +        object = std_Object_getPrototypeOf(object);
> +    } while (object !== null);
> +}
> +

Missed this before: redundant empty line.

::: js/src/jit-test/tests/self-hosting/object-define-hazard.js
@@ +1,1 @@
> +// We shouldn't do the wrong thing, with an evil Object.prototype

Same as in lookup-hazard.js

@@ +6,5 @@
> +x.__defineSetter__("a", setter);
> +var desc = Object.getOwnPropertyDescriptor(x, "a");
> +assertEq(desc.get, undefined);
> +assertEq(desc.set, setter);
> +delete Object.prototype.get

Missing semicolon.

::: js/src/jit-test/tests/self-hosting/object-lookup-hazard.js
@@ +1,1 @@
> +// We shouldn't do the wrong thing, with an evil Object.prototype

Drop the comma, and perhaps do s/with/in the face of/, but up to you.

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8533134 [details] [diff] [review]
v4 selfhost define{G,S}etter/lookup{G,S}etter

Review of attachment 8533134 [details] [diff] [review]:
-----------------------------------------------------------------

Only useful thing from my first review is the comment in Object.cpp.

::: js/src/builtin/Object.cpp
@@ +1205,5 @@
>      Rooted<TaggedProto> tagged(cx, TaggedProto(proto));
>      if (self->shouldSplicePrototype(cx) && !self->splicePrototype(cx, self->getClass(), tagged))
>          return false;
> +    if (!isSelfHostingGlobal)
> +        return JS_DefineFunctions(cx, proto, object_selfhosted_methods);

Please move this up to where we already do the same thing for object_static_selfhosted_methods. Also as a heads-up, this function has just moved to builtin/Object.cpp.

::: js/src/builtin/Object.js
@@ +128,5 @@
> +            return desc.get;
> +        object = std_Object_getPrototypeOf(object);
> +    } while (object !== null);
> +}
> +

Missed this before: redundant empty line.

::: js/src/jit-test/tests/self-hosting/object-define-hazard.js
@@ +1,1 @@
> +// We shouldn't do the wrong thing, with an evil Object.prototype

Same as in lookup-hazard.js

@@ +6,5 @@
> +x.__defineSetter__("a", setter);
> +var desc = Object.getOwnPropertyDescriptor(x, "a");
> +assertEq(desc.get, undefined);
> +assertEq(desc.set, setter);
> +delete Object.prototype.get

Missing semicolon.

::: js/src/jit-test/tests/self-hosting/object-lookup-hazard.js
@@ +1,1 @@
> +// We shouldn't do the wrong thing, with an evil Object.prototype

Drop the comma, and perhaps do s/with/in the face of/, but up to you.

Comment 12

[André Bargull [:anba]]

Comment on attachment 8533134 [details] [diff] [review]
v4 selfhost define{G,S}etter/lookup{G,S}etter

Review of attachment 8533134 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.js
@@ +78,5 @@
> +
> +    var key = ToPropertyKey(name);
> +
> +    var desc = {
> +        __proto__: null,

Thanks for addressing this!

@@ +111,5 @@
> +    var object = ToObject(this);
> +
> +    do {
> +        var desc = std_Object_getOwnPropertyDescriptor(object, key);
> +        if (desc && callFunction(std_Object_hasOwnProperty, desc, "set"))

To preserve compatibility (*) with the previous version, this conditional (and the equivalent code in ObjectLookupGetter) should read:
```
if (desc) {
    if (callFunction(std_Object_hasOwnProperty, desc, "set"))
        return desc.set;
    return;
}
```

(*) Unless you want to align behaviour with V8. For reference:
SM: https://gist.github.com/anba/9140946#objectprototype__lookupsetter__p
JSC: https://gist.github.com/anba/9140946#objectprototype__lookupsetter__p-1
V8: https://gist.github.com/anba/9140946#objectprototype__lookupsetter__p-2

Comment 13

[Tom S]

Thanks André!. I actually added exactly that second code with hasOwnProperty as well, but I couldn't come up with a testcase. How do you test that?

Comment 14

[Tom S]

Never mind, just realized that I had the wrong idea all along. I need to shadow with a value property, and not with an accessor property.

var x = {get prop() { return "Xxxx" }};
var y = {__proto__: x, prop: 1};
print(y.__lookupGetter__("prop"));

Comment 15

[Tom S]

Attachment: v5 selfhost define{G,S}etter/lookup{G,S}etter

Now with the shadowing case fixed.

Comment 16

[Till Schneidereit [:till]]

Comment on attachment 8533146 [details] [diff] [review]
v5 selfhost define{G,S}etter/lookup{G,S}etter

Review of attachment 8533146 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, we shouldn't change this. Aligning with v8 might be nice, but there would almost certainly be subtle breakage, and people should really just use Object.defineProperty.

::: js/src/jit-test/tests/self-hosting/object-define-hazard.js
@@ +1,1 @@
> +// We shouldn't do the wrong thing in the face of an evi Object.prototype

s/evi/evil/

Comment 17

[André Bargull [:anba]]

Comment on attachment 8533146 [details] [diff] [review]
v5 selfhost define{G,S}etter/lookup{G,S}etter

Review of attachment 8533146 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.js
@@ +70,5 @@
>      return to;
>  }
>  
> +function ObjectDefineSetter(name, setter) {
> +    var object = this != null ? ToObject(this) : global;

Just realized the conditional here and the other one in ObjectDefineGetter probably need to be changed, because document.all ... :-/
```
var object = (this !== null && this !== undefined) ? ToObject(this) : global;
```

Comment 18

[Tom S]

Bobby, I see some weird failures in XrayToJS: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bfa96b004008. Any idea what that is? Maybe |global| doesn't work right?

Comment 19

[Bobby Holley (:bholley)]

The failure indicates that __defineGetter__ is not an own property of Object.prototype. Does that match what you see when testing standalone with xpcshell?

var sb = new Cu.Sandbox('http://www.example.com');
Object.getOwnPropertyDescriptor(sb.Object.prototype, '__defineGetter__');

Comment 20

[Tom S]

Indeed: "undefined".

Comment 21

[Tom S]

So Xrays don't work, because we removed the function from the object_methods array. I am not sure what the best way for fixing this is. Till do you have any suggestions?

Comment 22

[Tom S]

Attachment: v1 - Allow selfhosted functions to be defined later

Naming suggestions welcome, this still has to go through try, but xraytojs passes with this as a base.

Comment 23

[Tom S]

Attachment: v6 - selfhost define{G,S}etter/lookup{G,S}etter

Uses the new late definition code that I just wrote. Also fixes the document.all problem by using strict equality compares.

Comment 24

[Till Schneidereit [:till]]

Comment on attachment 8533821 [details] [diff] [review]
v1 - Allow selfhosted functions to be defined later

Review of attachment 8533821 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

::: js/src/jsapi.h
@@ +966,5 @@
>                                             set of the same-named property in an
>                                             object that delegates to a prototype
>                                             containing this property */
>  #define JSPROP_INDEX            0x80    /* name is actually (int) index */
> +#define JSPROP_DEFINE_LATE      0x100   /* Don't define property when initially creating

uber-nit: move the 0x100 one char left to correctly right-align it with the other flags.

@@ +3508,5 @@
>   */
>  extern JS_PUBLIC_API(JSObject*)
>  JS_BindCallable(JSContext *cx, JS::Handle<JSObject*> callable, JS::Handle<JSObject*> newThis);
>  
> +enum DefineBehavior {

This is an awfully generic name. Maybe "PropertyDefinitionBehavior" and add a comment explaining what it does?

Comment 25

[Till Schneidereit [:till]]

Comment on attachment 8533825 [details] [diff] [review]
v6 - selfhost define{G,S}etter/lookup{G,S}etter

Review of attachment 8533825 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent!

I didn't see this before, but theoretically you could change the Proxy.create stuff to ES6 proxies while you're at it. Then again, we should make a big push for changing all tests to that anyway, so maybe not worth it right now.

Comment 26

[Tom S]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54938d8a3baa
https://hg.mozilla.org/integration/mozilla-inbound/rev/4638c344364d

Just noticed that the comment I wrote late last night is terribly bad. I am going to fix it soon.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/54938d8a3baa
https://hg.mozilla.org/mozilla-central/rev/4638c344364d