Freezing an array does not set non-writable length correctly

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
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.
Attachment #8851564 - Flags: review?(jwalden+bmo)

Comment 1

11 months ago
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|.
Attachment #8851564 - Flags: review?(jwalden+bmo) → review+

Comment 2

9 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03b31080750
Fix SetIntegrityLevel to set non-writable length correctly when freezing arrays. r=Waldo

Comment 3

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e03b31080750
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.