1471841 - Move WouldDefinePastNonwritableLength into NativeObject.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

WouldDefinePastNonwritableLength is only used within NativeObject.cpp, so moving it there should helper the compiler potentially inlining it.

Comment 1

[André Bargull [:anba]]

Attachment: bug1471841.patch

Moves WouldDefinePastNonwritableLength into NativeObject.

I've also removed the type check from WouldDefinePastNonwritableLength, because all callers except for SetDenseOrTypedArrayElement already call it with ArrayObjects.

Also SetDenseOrTypedArrayElement doesn't really seem to need the WouldDefinePastNonwritableLength check, because an existing dense array element can't have an index greater-or-equals than the array's length, can it? 

And I've changed the fail-soft error for typed arrays in SetDenseOrTypedArrayElement from JSMSG_BAD_INDEX to JSMSG_TYPED_ARRAY_DETACHED and added a comment (and an assertion) explaining when the out-of-bounds write access can actually happen.

And finally the patch also removes an unused parameter from SetExistingProperty.

Phew, that was a bit more than simply moving WouldDefinePastNonwritableLength into NativeObject.cpp. :-)

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8988454 [details] [diff] [review]
bug1471841.patch

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

Very nice.

::: js/src/vm/NativeObject.cpp
@@ +2657,5 @@
>              TypedArrayObject::setElement(obj->as<TypedArrayObject>(), index, d);
>              return result.succeed();
>          }
>  
> +        // An existing typed array element is now out-of-bounds implies the

s/implies/implying/ ?

@@ +2666,4 @@
>      }
>  
> +    MOZ_ASSERT_IF(obj->is<ArrayObject>(),
> +                  !WouldDefinePastNonwritableLength(&obj->as<ArrayObject>(), index));

Yes this is fine, we could even MOZ_ASSERT(obj->containsDenseElement(index)) I think? If we're just setting an existing element the non-writable length stuff is irrelevant anyway.

Comment 3

[André Bargull [:anba]]

Attachment: bug1471841.patch

Updated patch to use |NativeObject::containsDenseElement(...)| per review comments and slightly reworded one comment. Carrying r+.

Comment 4

[André Bargull [:anba]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa64e3aff88
Move WouldDefinePastNonwritableLength into NativeObject.cpp. r=jandem

Comment 6

[Eliza Balazs [:ebalazs_]]

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