1478503 - Differential Testing: Different output message involving Object.preventExtensions

Status: RESOLVED FIXED

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

Description

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

try {
    x = [8, 0];
    Object.preventExtensions(x);
    x.sort(function() {
        x.shift();
    });
} catch (e) {
    print(e);
};
print(x);


$ ./js-dbg-64-dm-linux-4c4abe35d808 --fuzzing-safe --no-threads --ion-eager testcase.js 
8,0

$ ./js-dbg-64-dm-linux-4c4abe35d808 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 
TypeError: can't define property 1: Array is not extensible
8

Tested this on m-c rev 4c4abe35d808.

My configure flags are:

'AR=ar' 'sh' './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 4c4abe35d808

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

[André Bargull [:anba]]

Attachment: bug1478503.patch

Is the following analysis and its conclusion correct or can we do better somehow?


We only use MFallibleStoreElement when we store an element into a potentially non-extensible object [1]. But when an object is non-extensible, we shouldn't try to use this code [2] to store the element, just because there's enough elements-capacity. Instead we need to call into the VM and check that the object is actually extensible before adding the new element. We assumed non-extensible arrays always have a capacity equal to their initialized length, but as the test case shows, this is not necessarily the case when the array length is modified after calling PreventExtensions. So unless we always adjust the capacity to match the initialized length when an object is non-extensible, it seems inevitable to have this VM-call for MFallibleStoreElement.

I've also checked other callers which use ObjectElements::offsetOfCapacity() to see if there are similar issues. But that doesn't seem to be the case.

IonCacheIRCompiler::emitStoreDenseElement():
- Copied the comment from BaselineCacheIRCompiler [3], because it should also apply for IonCacheIRCompiler. (Is that correct?)

IonBuilder::inlineArrayPush():
- I don't think this change is strictly necessary, because we shouldn't end up inlining Array.prototype.push when the array is non-extensible, because calling Array.prototype.push on a non-extensible array always throws a TypeError, which means for IonBuilder the return-type is not a MIRType::Int32 [4]. But explicitly rejecting arrays with OBJECT_FLAG_NON_EXTENSIBLE_ELEMENTS seems more robust.


[1] https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/js/src/jit/IonBuilder.cpp#9268,9273-9274
[2] https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/js/src/jit/CodeGenerator.cpp#9504-9555
[3] https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/js/src/jit/BaselineCacheIRCompiler.cpp#1416-1418
[4] https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/js/src/jit/MCallOptimize.cpp#819-820

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8995138 [details] [diff] [review]
bug1478503.patch

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

Thanks for looking at this! Patch LGTM. There is a perf issue when the object *might* be non-extensible but it's actually extensible, then we can no longer bump the length inline and now have to call into the VM. We could avoid this by checking the BaseShape's NON_EXTENSIBLE flag, but that's probably not worth it.

::: js/src/jit-test/tests/basic/non-extensible-elements9.js
@@ +19,5 @@
> +for (var i = 0; i < 15; ++i)
> +    testNonExtensibleStoreFallibleT();
> +
> +// Repeat testNonExtensibleStoreFallibleT for the MIRType::Value specialization.
> +function testNonExtensibleStoreFallibleV() {

Should this function have an |i| argument? It works now because |i| is a global variable, but the caller also passes |i| as argument.

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +1915,5 @@
>      masm.loadPtr(Address(obj, NativeObject::offsetOfElements()), scratch1);
>  
> +    // Assert no copy-on-write elements are present. Note that this stub is
> +    // not attached for non-extensible objects, so the shape guard ensures
> +    // there are no sealed or frozen elements.

Yes this is correct, https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/js/src/jit/CacheIR.cpp#3634-3635

Comment 3

[Jan de Mooij [:jandem]]

Another option is for ArrayObject::setLength{Int32} to call shrinkCapacityToInitializedLength() if !isExtensible() right? Or move this into the callers and have setLength{Int32} assert this. Do you think that's worth trying?

Comment 4

[André Bargull [:anba]]

Attachment: bug1478503-alternative.patch

For the alternative patch we always need to adjust the capacity when modifying the initialized length of a non-extensible object.

Most callers which modify `ObjectElements::initializedLength` are statically known to operate on extensible objects, so we don't need to worry about adjusting the capacity for them. To enforce this I've added additional assertions in all(?) functions which potentially modify the initialized length.

NativeObject::setDenseInitializedLength() was then changed, so it can only be called on extensible objects and NativeObject::setDenseInitializedLengthMaybeNonExtensible() was added for the case when modifying the initialized length of a potentially non-extensible object. 

And finally also changed IonBuilder::inlineArrayPopShift() to disallow inlining Array.prototype.pop() on potentially non-extensible objects, because when the object is non-extensible, we'd need to adjust the capacity when removing the last element of the array.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8996419 [details] [diff] [review]
bug1478503-alternative.patch

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

Thanks a lot! I'm not sure which one I like better: this one avoids the potential perf issue and JIT changes but it does add slight complexity to the NativeObject methods (however I like the isExtensible() assertions you added, it will catch problems there and fuzzers are good at finding such issues). I think I prefer this version but if you want to land the other patch that's also fine with me.

::: js/src/vm/NativeObject.h
@@ +1268,5 @@
> +
> +    void setDenseInitializedLengthMaybeNonExtensible(JSContext* cx, uint32_t length) {
> +        setDenseInitializedLengthInternal(length);
> +        if (!isExtensible())
> +            shrinkCapacityToInitializedLength(cx);

It would be nice to assert this invariant in some strategic location, to make sure we don't break it elsewhere (in JIT code or something). Something like:

MOZ_ASSERT_IF(!isExtensible(), getDenseInitializedLength() == getDenseCapacity());

Maybe in getDenseInitializedLength? We just have to be careful to avoid infinite recursion...

Comment 6

[Nicolas B. Pierron [:nbp]]

André, forgot to ask for checkin?

Comment 7

[André Bargull [:anba]]

Attachment: bug1478503.patch

Okay, then let's go with the second version and wait for the fuzzers to find new and exciting ways to break the internal invariants. :-)

Also added the assertion proposed in comment #5 to NativeObject::getDenseInitializedLength(). Carrying r+.

Comment 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd030479d010982a9b60d48d39e8ba9731f4da97

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c543368b25a6
Shrink capacity when modifying length on a non-extensible array. r=jandem

Comment 10

[Dorel Luca [:dluca]]

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