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

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: jorendorff, Unassigned)

Tracking

(Blocks: 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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'
Created attachment 8548191 [details] [diff] [review]
Test all property descriptors in Object.isSealed/isFrozen.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4a93d228b7e
Attachment #8548191 - Flags: review?(jorendorff)
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.
Duplicate of this bug: 1062340
Created attachment 8550777 [details] [diff] [review]
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
Attachment #8548191 - Attachment is obsolete: true
Attachment #8548191 - Flags: review?(jorendorff)
Attachment #8550777 - Flags: review?(jorendorff)
(Reporter)

Comment 5

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

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

OK.
Attachment #8550777 - Flags: review?(jorendorff) → review+
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."
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
Apparently this is going to change again and short-circuiting is then correct: https://bugs.ecmascript.org/show_bug.cgi?id=3586
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.
(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.
Thank you André and till :)

Backed out the change.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf998040d6b
Created attachment 8560835 [details] [diff] [review]
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
Attachment #8560835 - Flags: review?(evilpies)
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.
Attachment #8560835 - Flags: review?(evilpies) → review+
thanks, it should be this:
https://bugs.ecmascript.org/show_bug.cgi?id=3727

https://hg.mozilla.org/integration/mozilla-inbound/rev/f79ee4ad3b68
https://bugs.ecmascript.org/show_bug.cgi?id=3758 as well.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/f79ee4ad3b68
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.