If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Self host __defineGetter__ and friends

RESOLVED FIXED in mozilla37

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: David Bruant, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 784288
Created attachment 771731 [details] [diff] [review]
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?
Attachment #771731 - Flags: feedback?(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.
Attachment #771731 - Flags: feedback?(till)
Created attachment 822761 [details] [diff] [review]
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
Attachment #771731 - Attachment is obsolete: true
Attachment #822761 - Flags: feedback?(till)

Comment 4

4 years ago
__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

4 years ago
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 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.
Attachment #822761 - Flags: feedback?(till)
Assignee: general → nobody
(Assignee)

Updated

3 years ago
Assignee: nobody → evilpies
(Assignee)

Comment 7

3 years ago
Created attachment 8532229 [details] [diff] [review]
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.
Attachment #822761 - Attachment is obsolete: true
Attachment #8532229 - Flags: review?(till)
(Assignee)

Comment 8

3 years ago
Created attachment 8533133 [details] [diff] [review]
v4 selfhost define{G,S}etter/lookup{G,S}etter

I fixed two bugs, when somebody fuzzes with Object.prototype.get.
Attachment #8532229 - Attachment is obsolete: true
Attachment #8532229 - Flags: review?(till)
Attachment #8533133 - Flags: review?(till)
(Assignee)

Comment 9

3 years ago
Created attachment 8533134 [details] [diff] [review]
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.
Attachment #8533133 - Attachment is obsolete: true
Attachment #8533133 - Flags: review?(till)
Attachment #8533134 - Flags: review?(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.
Attachment #8533134 - Flags: review?(till) → review+
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

3 years ago
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
(Assignee)

Comment 13

3 years ago
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?
(Assignee)

Comment 14

3 years ago
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"));
(Assignee)

Comment 15

3 years ago
Created attachment 8533146 [details] [diff] [review]
v5 selfhost define{G,S}etter/lookup{G,S}etter

Now with the shadowing case fixed.
Attachment #8533134 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8533146 - Flags: review?(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/
Attachment #8533146 - Flags: review?(till) → review+

Comment 17

3 years ago
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;
```
(Assignee)

Comment 18

3 years ago
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?
Flags: needinfo?(bobbyholley)
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__');
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 20

3 years ago
Indeed: "undefined".
(Assignee)

Comment 21

3 years ago
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?
(Assignee)

Comment 22

3 years ago
Created attachment 8533821 [details] [diff] [review]
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.
Attachment #8533821 - Flags: review?(till)
(Assignee)

Comment 23

3 years ago
Created attachment 8533825 [details] [diff] [review]
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.
Attachment #8533146 - Attachment is obsolete: true
Attachment #8533825 - Flags: review?(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?
Attachment #8533821 - Flags: review?(till) → review+
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.
Attachment #8533825 - Flags: review?(till) → review+
(Assignee)

Comment 26

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/54938d8a3baa
https://hg.mozilla.org/mozilla-central/rev/4638c344364d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.