Closed Bug 1120512 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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'
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
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)
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
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+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.