1120512 - TestIntegrityLevel (isSealed/isFrozen) can't short-circuit, per spec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The spec requires TestIntegrityLevel to plod on through every property, even after it determines that it won't return true. We currently short-circuit out.

It's observable when isSealed/isFrozen is used on a proxy:


var log = '';
var target = Object.preventExtensions({a: 1, b: 2, c: 3});
var p = new Proxy(target, {
    getOwnPropertyDescriptor(t, id) {
        log += id;
        return Object.getOwnPropertyDescriptor(t, id);
    }
});
assertEq(Object.isSealed(p), false);
assertEq(log, 'abc');   // FAILS with log === 'a'

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Test all property descriptors in Object.isSealed/isFrozen.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4a93d228b7e

Comment 2

[Tom S [:evilpie]]

I think it wouldn't hurt if we actually implemented this functions in terms of the spec. i.e  using GetOwnProperty instead of GetPropertyAttributes and proper step numbers.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Test all property descriptors in js::TestIntegrityLevel.

Thanks evilpie :)
I rewrote it match to the spec steps.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f82984dd632e

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8550777 [details] [diff] [review]
Test all property descriptors in js::TestIntegrityLevel.

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

OK.

Comment 6

[Tom S [:evilpie]]

Comment on attachment 8550777 [details] [diff] [review]
Test all property descriptors in js::TestIntegrityLevel.

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

::: js/src/jsobj.cpp
@@ +1177,5 @@
> +    // Steps 12-14.
> +    if (level == IntegrityLevel::Frozen && writable)
> +        *result = false;
> +    else if (configurable)
> +        *result = false;

I would change this to
 else
  *result = !configurable.

I my mind that reads like "result is okay, when it's not configurable."

Comment 7

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Tom Schuster [:evilpie] from comment #6)
> I would change this to
>  else
>   *result = !configurable.
> 
> I my mind that reads like "result is okay, when it's not configurable."

Okay, changed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17c4307d1678

Comment 8

[André Bargull [:anba]]

Apparently this is going to change again and short-circuiting is then correct: https://bugs.ecmascript.org/show_bug.cgi?id=3586

Comment 9

[Tooru Fujisawa [:arai]]

Is it better to backout this change now? or wait for draft rev32?
In any case, it would be better to update comment (and code) after rev32 is published.

Comment 10

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> Is it better to backout this change now? or wait for draft rev32?
> In any case, it would be better to update comment (and code) after rev32 is
> published.

It's better to back out now. We don't want to be compatible with whatever is the current draft if we already know that the next one will make us incompatible.

Comment 11

[Tooru Fujisawa [:arai]]

Thank you André and till :)

Backed out the change.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf998040d6b

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Update comment for TestIntegrityLevel and add a testcase.

No changes in code, but updated comment and added testcase for short-circuit.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c04d70bde8e

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8560835 [details] [diff] [review]
Update comment for TestIntegrityLevel and add a testcase.

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

Too bad that in the current draft revision TestIntegrity level is slightly wrong, so the step numbers are going to change.

Comment 14

[Tooru Fujisawa [:arai]]

thanks, it should be this:
https://bugs.ecmascript.org/show_bug.cgi?id=3727

https://hg.mozilla.org/integration/mozilla-inbound/rev/f79ee4ad3b68

Comment 15

[Tom S [:evilpie]]

https://bugs.ecmascript.org/show_bug.cgi?id=3758 as well.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f79ee4ad3b68

Comment 17

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.