1068589 - Don't call JSObject::isExtensible() before JSObject::preventExtensions()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test 1:

void Object.preventExtensions(new Proxy({}, new Proxy({}, {get(t, pk){ print(`trap: ${pk}`); return t[pk]; }})));

Expected: Prints
---
trap: preventExtensions
---

Actual:
Prints in debug mode
---
trap: isExtensible
trap: isExtensible
trap: preventExtensions
---

Prints in non-debug mode
---
trap: isExtensible
trap: preventExtensions
---


Test 2: 

void Object.seal(new Proxy({}, new Proxy({}, {get(t, pk){ print(`trap: ${pk}`); return t[pk]; }})));

Expected: Prints
---
trap: preventExtensions
trap: ownKeys
---

Actual:
Prints in debug mode
---
trap: isExtensible
trap: isExtensible
trap: preventExtensions
trap: ownKeys
---

Prints in non-debug mode
---
trap: isExtensible
trap: preventExtensions
trap: ownKeys
---


Test 3:

void Object.freeze(new Proxy({}, new Proxy({}, {get(t, pk){ print(`trap: ${pk}`); return t[pk]; }})))

Expected: Prints
---
trap: preventExtensions
trap: ownKeys
---

Actual:
Prints in debug mode
---
trap: isExtensible
trap: isExtensible
trap: preventExtensions
trap: ownKeys
---

Prints in non-debug mode
---
trap: isExtensible
trap: preventExtensions
trap: ownKeys
---


Bonus Round:

(function() { var o = {}; var p = new Proxy(o, {get preventExtensions(){ Object.preventExtensions(o) }}); Object.preventExtensions(p) })()

Expected: No assertion error
Actual:
---
Assertion failure: extensible (Callers must ensure |obj| is extensible before calling preventExtensions), at /home/andre/git/mozilla-central-master/js/src/vm/Shape.cpp:1350
---

Comment 1

[André Bargull [:anba]]

Attachment: bug1068589 patch

As usual proxies are able to detect when things are not exactly implemented as spec'ed, in this case extraneous calls to JSObject::isExtensible() can be detected. 

I think it's safe to drop the additional calls to JSObject::isExtensible() in vm/Debugger.cpp and jsapi.cpp, but I can't tell for sure. It's seems unlikely a jsapi user expects JS_PreventExtensions does not call JSObject::preventExtensions for non-extensible objects...

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8491035 [details] [diff] [review]
bug1068589 patch

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

So, the patch as it is is okay.  But two of the tests in here are wrong, because of a pre-existing issue.  I'm fine with us deferring that to a followup bug, but if we do that, I don't want known-wrong tests landing at the same time.

So, rename the one test, kill the extra braces, and remove the two wrong tests, and I can r+ that.  We should pocket that win and deal with the other issue in a separate bug.

::: js/src/jit-test/tests/basic/bug1068589.js
@@ +1,1 @@
> +// Don't assert

This is a bad test file name and location.  This should be in tests/proxy, and it should have a name like target-becomes-nonextensible-during-preventExtensions.js.  No reader is going to have any idea what this file name means if it includes an opaque bug number.  Plus, the current name is un-autocompletable and unmemorable (even for quick copy-from-terminal-to-editor uses), whereas a phrase-like name has a fighting chance of being remembered well enough for autocomplete to save the day.

::: js/src/tests/ecma_6/Object/freeze-proxy.js
@@ +13,5 @@
> +}
> +
> +var {proxy, log} = logProxy();
> +Object.freeze(proxy);
> +assertDeepEq(log, ["preventExtensions", "ownKeys"]);

Same reversed order as in the other test.

@@ +17,5 @@
> +assertDeepEq(log, ["preventExtensions", "ownKeys"]);
> +
> +var {proxy, log} = logProxy();
> +Object.freeze(Object.freeze(proxy));
> +assertDeepEq(log, ["preventExtensions", "ownKeys", "preventExtensions", "ownKeys"]);

Same.

::: js/src/tests/ecma_6/Object/seal-proxy.js
@@ +13,5 @@
> +}
> +
> +var {proxy, log} = logProxy();
> +Object.seal(proxy);
> +assertDeepEq(log, ["preventExtensions", "ownKeys"]);

This order is wrong, isn't it?  Object.seal calls SetIntegrityLevel, whose first step is to call [[OwnPropertyKeys]].  Only at the end does it call [[PreventExtensions]].

@@ +17,5 @@
> +assertDeepEq(log, ["preventExtensions", "ownKeys"]);
> +
> +var {proxy, log} = logProxy();
> +Object.seal(Object.seal(proxy));
> +assertDeepEq(log, ["preventExtensions", "ownKeys", "preventExtensions", "ownKeys"]);

Again with the backwards.

::: js/src/vm/Shape.cpp
@@ +1347,5 @@
>      }
>  
> +    if (!obj->nonProxyIsExtensible()) {
> +        return true;
> +    }

No braces.

Comment 3

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> So, the patch as it is is okay.  But two of the tests in here are wrong,
> because of a pre-existing issue.  I'm fine with us deferring that to a
> followup bug, but if we do that, I don't want known-wrong tests landing at
> the same time.

The current behaviour (first preventExtensions, then ownKeys) contradicts what's in the spec, but it seems the spec is wrong about this [1]. The evaluation of that spec bug is still pending, though. 

[1] https://bugs.ecmascript.org/show_bug.cgi?id=3215

Comment 4

[Jeff Walden [:Waldo]]

Ooh, good call.  I rescind the request to remove those tests.  Please make the few other changes noted, add comments into those two tests saying that the spec requires something clearly wrong, and I'll r+ that promptly.

Comment 5

[André Bargull [:anba]]

Attachment: bug1068589 patch v2

Updated patch per review comments!

Comment 6

[Jeff Walden [:Waldo]]

If I'd been thinking I'd have landed this in my push just now, but sadly I wasn't.  :-(  Will batch it up with my next set of pushes, unless this has particular reason to land sooner.

Comment 7

[Jeff Walden [:Waldo]]

Not accumulating patches quickly enough to push this promptly, landed it standalone to get it in sooner:

https://hg.mozilla.org/integration/mozilla-inbound/rev/051ab1a84d38

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/051ab1a84d38