Bug 38636 – Object.defineProperty doesn't respect attributes when applied to the Global Object

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: JavaScriptCore

[Kent Hansen]

For a "plain" object, this works as expected:

./jsc
> Object.defineProperty(o, "foo", { writable: false, configurable: false, value: 123 });
[object Object]
> o.foo
123
> o.foo = 456; o.foo
123
> delete o.foo
false

When defining a property on the Global Object, however, the result is different:

./jsc
> Object.defineProperty(this, "foo", { writable: false, configurable: false, value: 123 });
[object global]
> foo
123
> foo = 456; foo
456
> delete foo
true
> foo
Exception: ReferenceError: Can't find variable: foo

The same happens when using the C API, e.g.:

JSStringRef jsFooString = JSStringCreateWithUTF8CString("foo");
JSValueRef js123Value = JSValueMakeNumber(context, 123);
JSObjectSetProperty(context, globalObject, jsFooString, js123Value, kJSPropertyAttributeReadOnly, 0);
JSStringRef myScript = JSStringCreateWithUTF8CString("foo = 456; foo");
JSValueRef ret = JSEvaluateScript(context, myScript, 0, 0, 0, 0); // ret will be 456

[William J. Edney]

I can confirm this bug, and its a nasty one (IE9 beta1 has the same issue and I've reported it to them as well).

One of our chief uses for this is the ability to 'lock' a global variable acting as a namespace for a library of code, such that it can't be inadvertently blown away by a user of that library forgetting to 'var' a variable of the same name:

Object.defineProperty(self, 'myLib', {writable: false});

Then 'myLib' can be used to 'hang' code off of, but can't be blown away.

Any ideas on fixage here?

Cheers,

- Bill

[Xan Lopez]

FWIW, something like this:


diff --git a/JavaScriptCore/runtime/JSGlobalObject.cpp b/JavaScriptCore/runtime/JSGlobalObject.cpp
index a8fb7bf..11a06ce 100644
--- a/JavaScriptCore/runtime/JSGlobalObject.cpp
+++ b/JavaScriptCore/runtime/JSGlobalObject.cpp
@@ -163,15 +163,7 @@ void JSGlobalObject::putWithAttributes(ExecState* exec, const Identifier& proper

     if (symbolTablePutWithAttributes(propertyName, value, attributes))
         return;
-
-    JSValue valueBefore = getDirect(propertyName);
-    PutPropertySlot slot;
-    JSVariableObject::put(exec, propertyName, value, slot);
-    if (!valueBefore) {
-        JSValue valueAfter = getDirect(propertyName);
-        if (valueAfter)
-            JSObject::putWithAttributes(exec, propertyName, valueAfter, attributes);
-    }
+    JSObject::putWithAttributes(exec, propertyName, value, attributes);
 }

seems to fix the proposed testcase, and I don't get any regression in the jsc or layout tests. I must say that I don't really understand what the removed code is trying to achieve though, but the end result is that the value is introduced in the global object disregarding the attributes requested (first in the direct call to ::put, and after that in JSObject::putDirectInternal, when we find that the value was already inserted).

If this seems sensible I can do a layout test and put it up for review.

[Xan Lopez]

Created attachment 72562 [details]
definepropertyglobalobject.diff

Attaching the suggested patch with a layout test.

[Kent Hansen]

Hi Xan, thanks for your interest in this issue. :-)

It looks like the valueBefore/valueAfter logic dates back to the patch for https://bugs.webkit.org/show_bug.cgi?id=17067 (http://trac.webkit.org/changeset/30534), although code has changed/moved around since then. Maybe Darin could comment on what this actually does.

It seems like this part

    if (valueAfter)
        JSObject::putWithAttributes(exec, propertyName, valueAfter, attributes);

relies on putWithAttributes() updating the attributes of the existing property (since the previous put() created it with no attributes).

This is the bug reported in https://bugs.webkit.org/show_bug.cgi?id=40613. And like I commented there, a potential fix would be to make JSObject::putWithAttributes() update the attributes in case the property already exists (just like it does for the value).

Simply removing the call to put() seems a bit risky since JSObject::put() has a lot more logic than JSObject::putWithAttributes().
For example, your testcase should have something like

Object.defineProperty(this, "__proto__", {value:123})

As per JSObject::put(), "Setting __proto__ to a non-object, non-null value is silently ignored to match Mozilla". (I just checked with V8 and they also match this behavior for defineProperty(O, "__proto__")).

You should also check what happens when the property exists as a getter/setter in the prototype chain, since JSObject::put() has logic for that too.

this.__proto__ = {};
this.__proto__.__defineGetter__("foo", function() { return this._x; });
this.__proto__.__defineSetter__("foo", function(v) { this._x = v; });
Object.defineProperty(this, "foo", { value: 123, configurable: true, writable: true });
this.hasOwnProperty("foo"); // FALSE!

I.e. currently (without your patch) the property will not be defined on the Global Object, but will rather invoke the foo setter in the prototype chain. This is different than the behavior for normal objects, where Object.defineProperty() _will_ create a new property on the object itself, even if there is a setter in the prototype chain. This is a different bug, but I don't see how it can be fixed so long as JSGlobalObject::putWithAttributes() calls JSObject::put()...

[Kent Hansen]

I created https://bugs.webkit.org/show_bug.cgi?id=48911 for the Global-Object-with-setter-in-prototype-chain issue.

[Oliver Hunt]

Comment on attachment 72562 [details]
definepropertyglobalobject.diff

View in context: https://bugs.webkit.org/attachment.cgi?id=72562&action=review

> JavaScriptCore/runtime/JSGlobalObject.cpp:-174
> -    JSValue valueBefore = getDirect(propertyName);
> -    PutPropertySlot slot;
> -    JSVariableObject::put(exec, propertyName, value, slot);
> -    if (!valueBefore) {
> -        JSValue valueAfter = getDirect(propertyName);
> -        if (valueAfter)
> -            JSObject::putWithAttributes(exec, propertyName, valueAfter, attributes);
> -    }

This old code was somewhat convoluted and I'm not sure why -- before we remove it I think we should really understand why the current code behaves as it does.

[Xan Lopez]

(In reply to comment #6)
> (From update of attachment 72562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72562&action=review
> 
> > JavaScriptCore/runtime/JSGlobalObject.cpp:-174
> > -    JSValue valueBefore = getDirect(propertyName);
> > -    PutPropertySlot slot;
> > -    JSVariableObject::put(exec, propertyName, value, slot);
> > -    if (!valueBefore) {
> > -        JSValue valueAfter = getDirect(propertyName);
> > -        if (valueAfter)
> > -            JSObject::putWithAttributes(exec, propertyName, valueAfter, attributes);
> > -    }
> 
> This old code was somewhat convoluted and I'm not sure why -- before we remove it I think we should really understand why the current code behaves as it does.

Totally agree (and thanks Kent for the extended reply!). The patch was more meant as a "Hey, this seems to fix it and pass all tests, what's the deal here". I've been using the patch for many days now and I haven't seen any obvious bug, so if the code achieves something it must be very subtle and I think at the very least it should a comment and some test checking that we do what we intend to do there.

[William J. Edney]

I'm attaching a testcase here. It certainly isn't all-encompassing, but when run on FF 4.Xb7 and Chrome 9.0.587.0 dev, it gives the expected values.

OTOH, when run against Webkit build 72487, the part of the test where the 'self' (i.e. global) object is being tested fails.

Cheers,

- Bill

[William J. Edney]

Created attachment 74511 [details]
A test showing that the global object isn't respecting the property descriptor supplied when defining a property on it.

[Dirk Schulze]

Comment on attachment 72562 [details]
definepropertyglobalobject.diff

Oliver changed the behavior on Object.defineProperty lately. Can you check it again please? Otherwise feel free to set r? again.

[Gavin Barraclough]

This works for me in ToT, was likely fixed in r106783.  Will land a regression test for this.  Please reopen if you're still repro'ing any issues in ToT.

Cheers, G.