1466363 - Differential Testing: Different output message involving Object.seal

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = {};
Array.prototype.unshift.apply(x, [(function() {}), {}, 0, undefined]);
Object.seal(x);
for (var p in this) {
    try {
        Object.defineProperty(x, 3, {
            configurable: (function() {}),
            enumerable: true,
            writable: true,
            value: 0,
        });
    } catch (e1) {}
}
print(uneval(x[3]));

$ ./js-dbg-64-dm-linux-8c9263730393 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js 
(void 0)

$ ./js-dbg-64-dm-linux-8c9263730393 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 
0

Tested this on m-c rev 8c9263730393.

My configure flags are:

'AR=ar' 'sh' '/home/ubuntu/trees/mozilla-central/js/src/configure' '--enable-debug' '--enable-more-deterministic' '--with-ccache' '--enable-gczeal' '--enable-debug-symbols' '--disable-tests'

python -u -m funfuzz.js.compile_shell -b "--enable-debug --enable-more-deterministic" -r 8c9263730393

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b46f3ba0c766
user:        Jan de Mooij
date:        Sat May 12 11:46:51 2018 +0200
summary:     Bug 1460381 - Support sealed and non-extensible dense elements on native objects. r=anba

Jan, is bug 1460381 a likely regressor?

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Good find. defineProperty uses INITELEM when defining a configurable/writable/enumerable property, but the JIT can't optimize that to a set when the object is sealed, because we have to throw a TypeError then. This patch just checks for non-extensible objects when optimizing INITELEM.

Annoying: I added tests for this (defineProperty on non-extensible/sealed/frozen objects) but they needed a loop to trigger this.

Comment 2

[André Bargull [:anba]]

Comment on attachment 8982868 [details] [diff] [review]
Patch

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

::: js/src/jit/CacheIR.cpp
@@ +3540,5 @@
>  
> +    // Don't optimize INITELEM (DefineProperty) on non-extensible objects: when
> +    // the elements are sealed, we have to throw an exception.
> +    if (IsPropertyInitOp(JSOp(*pc_)) && !nobj->isExtensible())
> +        return false;

Hmm, I don't understand why it's necessary to check for extensibility compared to just checking if the elements are sealed. 

For example when I replace the condition with:
---
if (IsPropertyInitOp(JSOp(*pc_)) && nobj->denseElementsAreSealed())
---

the fuzz-test and basic/non-extensible-elements6.js both still pass.

::: js/src/jit/IonBuilder.cpp
@@ +9158,5 @@
> +
> +        // Don't optimize INITELEM (DefineProperty) on potentially non-extensible
> +        // objects: when the array is sealed, we have to throw an exception.
> +        if (IsPropertyInitOp(JSOp(*pc)))
> +            return Ok();

Can you add a test case for this one, because it doesn't seem to be covered by basic/non-extensible-elements6.js (i.e. commenting out the if-statement didn't lead to test failures in basic/non-extensible-elements6.js.)

For example with this test case I was able to get consistent failures without the change in jit/IonBuilder.cpp.
---
function testSealNonExtensible() {
    var obj = Object.preventExtensions([0]);
    var desc = {value: 0, writable: true, enumerable: true, configurable: true};

    var errorAt = -1;
    var N = 10000;

    for (var i = 0; i <= N; ++i) {
        if (i === N) {
            Object.seal(obj);
        }
        try {
            Object.defineProperty(obj, 0, desc);
        } catch {
            errorAt = i;
        }
    }
    assertEq(errorAt, N);
}
for (var i = 0; i < 15; i++)
  testSealNonExtensible();
---

Comment 3

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #2)
> Hmm, I don't understand why it's necessary to check for extensibility
> compared to just checking if the elements are sealed. 

We have to check for extensibility because PreventExtensions triggers a Shape change. If we just check for sealed elements, the stub would fail if we pass a non-extensible object and then later seal that object (without triggering a Shape change). I will add a comment (I should have added one, sorry :/).

The test you wrote fails when I change [0] to {0:0} and check for sealed-elements instead of non-extensible. It doesn't repro with arrays because sealing an array triggers a Shape change for some reason, I'll try to figure out why - must be something related to the length property.

(I think using INITELEM on non-extensible objects like this is pretty rare, but if needed we could optimize this later by having StoreDenseElement guard on the SEALED flag for INITELEM ops.)

> Can you add a test case for this one, because it doesn't seem to be covered
> by basic/non-extensible-elements6.js (i.e. commenting out the if-statement
> didn't lead to test failures in basic/non-extensible-elements6.js.)

Oops yes, thanks for the test!

Comment 4

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to André Bargull [:anba] from comment #2)
> > Hmm, I don't understand why it's necessary to check for extensibility
> > compared to just checking if the elements are sealed. 
> 
> We have to check for extensibility because PreventExtensions triggers a
> Shape change. If we just check for sealed elements, the stub would fail if
> we pass a non-extensible object and then later seal that object (without
> triggering a Shape change). I will add a comment (I should have added one,
> sorry :/).
> 
> The test you wrote fails when I change [0] to {0:0} and check for
> sealed-elements instead of non-extensible. It doesn't repro with arrays
> because sealing an array triggers a Shape change for some reason, I'll try
> to figure out why - must be something related to the length property.

Derp, know I feel dumb. Like I was so close, but still couldn't make the connection. :-D

(I was only testing with arrays and just like said, sealing an array leads to a shape change, so I was never able to create a test case where only testing for sealed elements made a different compared to testing for extensibility.)

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> It doesn't repro with arrays
> because sealing an array triggers a Shape change for some reason, I'll try
> to figure out why - must be something related to the length property.

The length property is innocent...

What's happening is that when we call Object.preventExtensions(array), we set the NOT_EXTENSIBLE bit on the BaseShape and generate a new Shape for the last property ("length" in this case). Then when we call Object.seal(array), SetIntegrityLevel looks up the initial/empty shape and passes |nobj->lastProperty()->getObjectFlags()| for the BaseShape flags and this will create a *new* shape lineage, hence the shape change. There's not an easy fix and it's probably not worth worrying about..

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> There's not an easy fix and it's probably not worth worrying about..

I guess one option is for SetIntegrityLevel to try to *reuse* shapes in |shapes| until it finds a Shape it can't reuse because it needs to set the non-configurable/non-writable attributes on it. But anyway, that's just a pre-existing issue.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1765b4cd8fa
Don't optimize INITELEM on non-extensible objects as we might have to throw a TypeError. r=anba

Comment 8

[Jan de Mooij [:jandem]]

Btw I added your testcase, with separate functions for the object and array cases. I changed N from 10000 to 100 but confirmed I still get failures with --no-threads if I comment out the Ion code or change CacheIR.cpp to check for sealed elements instead of non-extensible objects.

Comment 9

[Andreea Pavel [:apavel]]

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