1350864 - Freezing an array does not set non-writable length correctly

Status: RESOLVED FIXED

Description

[Jan de Mooij [:jandem]]

Created attachment 8851564 [details] [diff] [review]
Patch

We have an invariant that arrays with non-writable length have capacity <= initializedLength, so the JITs don't have to check for non-writable length when adding new elements.

However, when we freeze an array, we set the NONWRITABLE_ARRAY_LENGTH ObjectElements flag but we don't ensure capacity <= initializedLength. This is a regression from bug 1283334 and observable with inlined Array#push in Ion.

The attached patch moves this code from ArraySetLength into ArrayObject::setNonWritableLength, so we can also call it from SetIntegrityLevel.

Comment 1

[Jeff Walden [:Waldo]]

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

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

> We have an invariant that arrays with non-writable length have
> capacity <= initializedLength, so the JITs don't have to check
> for non-writable length when adding new elements.

You meant capacity <= length, right?  I'm pretty sure initializedLength <= capacity is the *actual* invariant between these two fields.

Despite that flub, the patch does seem to actually be implementing the real invariant.

::: js/src/jit-test/tests/ion/array-push-frozen-array.js
@@ +11,5 @@
> +        try {
> +            arr.push(2);
> +            assertEq(i <= 1500, true);
> +        } catch(e) {
> +            assertEq(e.toString().includes("non-writable length"), true);

I'd prefer if you just checked for instanceof TypeError, to not depend on error messages that might change.

::: js/src/vm/ArrayObject.h
@@ +42,5 @@
> +        if (getElementsHeader()->capacity > len) {
> +            shrinkElements(cx, len);
> +            getElementsHeader()->capacity = len;
> +        }
> +        getElementsHeader()->setNonwritableArrayLength();

A |ObjectElements* header = getElementsHeader();| local would be better than calling the function multiple times, IMO.

::: js/src/vm/NativeObject.h
@@ +231,5 @@
>          return flags & NONWRITABLE_ARRAY_LENGTH;
>      }
>      void setNonwritableArrayLength() {
> +        // See ArrayObject::setNonWritableLength.
> +        MOZ_ASSERT(capacity <= initializedLength);

Mm.  I wish this could be folded directly into ArrayObject::setNonWritableArrayLength, while still preserving its encapsulation of flags-modification, given that's the only caller now.  :-\

Based on that being the only caller, and in light of the existing invariants on initializedLength/capacity/length, this should flatly assert |capacity == initializedLength|.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03b31080750
Fix SetIntegrityLevel to set non-writable length correctly when freezing arrays. r=Waldo

Comment 3

[Wes Kocher (:KWierso)]

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