Closed Bug 1310744 Opened 8 years ago Closed 8 years ago

Array.prototype.reverse should call Set, and throw when [[Set]] fails.

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Version:
51 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 + fixed
firefox52 + fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(9 files)

Bug 1310744: Add NativeObject::denseElementsAreFrozen.
58 bytes, text/x-review-board-request
nbp
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1310744: A frozen element has no writable length.
58 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1310744: Allow MoveBoxedOrUnboxedDenseElements to bail out if the elements are frozen.
58 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1310744: Bail out from SetOrExtendBoxedOrUnboxedDenseElements if the elements are frozen.
58 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1310744: Make ArrayReverseDenseKernel frozenness-aware.
58 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1310744: Don't enter in the fast path for DeleteArrayElement for frozen arrays.
58 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1310744: Add missing assertions in NativeObject.h
58 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1310744: Regression test.
58 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1310744: Revert "Bug 1300193: Make existing assertion that no longer holds more flexible. ", but keep the test.
58 bytes, text/x-review-board-request
jandem
: review+
nbp
: review+
Details 
Test case:

var a = Object.freeze([5, 4]);
a.reverse();

Expected: TypeError per https://tc39.github.io/ecma262/#sec-array.prototype.reverse
Actual result: The array is reversed.
This is actually kind of bad, because it allows to overwrite non-configurable, non-writable properties. And it's also regression, because it's not reproducible in Firefox 49. I wonder if it's caused by bug 1283334...?
Confirmed that 51 is the first affected version. Will try to track down a more specific range.
status-firefox50: ? → unaffected
status-firefox51: ? → affected
(In reply to André Bargull from comment #1)
> I wonder if it's caused by bug 1283334...?

Good call, confirmed with mozregression :)
Blocks: 1283334
Flags: needinfo?(leo)
Keywords: regressionwindow-wanted
Flags: in-testsuite?
Yeah, in general I feel we should assert !getElementsHeader()->isFrozen() in setDenseElement and pretty much every call in NativeObject where we assert !denseElementsAreCopyOnWrite().

I can try to find some time this week to track this down, since I think this is not exclusive of Array.prototype.reverse, but I can't promise anything just yet.
Yes we should have added these asserts. Emilio, if you don't have time please let us know, someone else will have to take this then.
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Priority: -- → P1
Recent javascript regression, tracking for 51
tracking-firefox51: ? → +
tracking-firefox52: ? → +
I'm trying to fix this.
Assignee: nobody → ecoal95
Sorry for the storm of review requests, Nicolas, I asked you for review since you reviewed bug 1283334.
Comment on attachment 8802657 [details]
Bug 1310744: Revert "Bug 1300193: Make existing assertion that no longer holds more flexible. ", but keep the test.

https://reviewboard.mozilla.org/r/86996/#review86186

Thanks!
Attachment #8802657 - Flags: review?(jdemooij) → review+
Comment on attachment 8802649 [details]
Bug 1310744: Add NativeObject::denseElementsAreFrozen.

https://reviewboard.mozilla.org/r/86980/#review86188
Attachment #8802649 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802650 [details]
Bug 1310744: A frozen element has no writable length.

https://reviewboard.mozilla.org/r/86982/#review86190
Attachment #8802650 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802652 [details]
Bug 1310744: Bail out from SetOrExtendBoxedOrUnboxedDenseElements if the elements are frozen.

https://reviewboard.mozilla.org/r/86986/#review86192
Attachment #8802652 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802651 [details]
Bug 1310744: Allow MoveBoxedOrUnboxedDenseElements to bail out if the elements are frozen.

https://reviewboard.mozilla.org/r/86984/#review86194
Attachment #8802651 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802653 [details]
Bug 1310744: Make ArrayReverseDenseKernel frozenness-aware.

https://reviewboard.mozilla.org/r/86988/#review86196
Attachment #8802653 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802654 [details]
Bug 1310744: Don't enter in the fast path for DeleteArrayElement for frozen arrays.

https://reviewboard.mozilla.org/r/86990/#review86198
Attachment #8802654 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802655 [details]
Bug 1310744: Add missing assertions in NativeObject.h

https://reviewboard.mozilla.org/r/86992/#review86202
Attachment #8802655 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802656 [details]
Bug 1310744: Regression test.

https://reviewboard.mozilla.org/r/86994/#review86204

r=me with the following nit fixed.

::: js/src/tests/ecma_5/Array/frozen-dense-array.js:27
(Diff revision 1)
> +// Shouldn't throw, since this is not strict mode, but shouldn't change the
> +// value of the property.
> +a.length = 5;

nit: Add the following test:

assertThrowsInstanceOf(() => { "use strict"; a.length = 5; }, TypeError);
Attachment #8802656 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8802657 [details]
Bug 1310744: Revert "Bug 1300193: Make existing assertion that no longer holds more flexible. ", but keep the test.

https://reviewboard.mozilla.org/r/86996/#review86208

Thanks!

These changes are really nice.
Splitting in so many tiny patches to fix it incrementally is nice too, but make it harder to review.
Attachment #8802657 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #27)
> These changes are really nice.
> Splitting in so many tiny patches to fix it incrementally is nice too, but
> make it harder to review.

Sorry, the intent was the exact opposite! :D

I'll try to find a better balance between throwing a single monolithic patch and this next time :)

I added test for strict and non-strict mode deletion of properties too, added a few more assertions (and fixed one related with lengthIsWritable, which assumed lengthIsWritable == lengthShape->writable() and was hitting on ecma_5/Array/redefine-length-frozen-dictionarymode-array.js). I don't think any of these changes are major enough to request a re-review, but feel free to take a look at them if needed.

Waiting for try now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c8ec0ffb1144f27e1fe5575cd3e4150d05ba85 (current) and https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8d6bbb0e24 (without the latest assertions I added).
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/888c35d3e472
Add NativeObject::denseElementsAreFrozen. r=nbp
https://hg.mozilla.org/integration/autoland/rev/9cf4fb2f956a
A frozen element has no writable length. r=nbp
https://hg.mozilla.org/integration/autoland/rev/01d6130f6cb5
Allow MoveBoxedOrUnboxedDenseElements to bail out if the elements are frozen. r=nbp
https://hg.mozilla.org/integration/autoland/rev/8ee1bce84ad8
Bail out from SetOrExtendBoxedOrUnboxedDenseElements if the elements are frozen. r=nbp
https://hg.mozilla.org/integration/autoland/rev/eceaa41aebae
Make ArrayReverseDenseKernel frozenness-aware. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d0d4f9792c6b
Don't enter in the fast path for DeleteArrayElement for frozen arrays. r=nbp
https://hg.mozilla.org/integration/autoland/rev/4839dd5d7d96
Add missing assertions in NativeObject.h r=nbp
https://hg.mozilla.org/integration/autoland/rev/3d81f34050a4
Regression test. r=nbp
https://hg.mozilla.org/integration/autoland/rev/c1fa65bccbd1
Revert "Bug 1300193: Make existing assertion that no longer holds more flexible. r=jandem,nbp", but keep the test.
Please request aurora uplift when this is on m-c.
https://hg.mozilla.org/mozilla-central/rev/c1fa65bccbd1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8802649 [details]
Bug 1310744: Add NativeObject::denseElementsAreFrozen.

This request is for all the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: bug 1283334
[User impact if declined]: Javascript regression that allows overriding non-configurable properties.
[Describe test coverage new/current, TreeHerder]: Automated regression test added in this PR.
[Risks and why]: Low: small changeset that prevents some fast-paths from being taken incorrectly.
[String/UUID change made/needed]: None
Attachment #8802649 - Flags: approval-mozilla-aurora?
Flags: needinfo?(leo)
Comment on attachment 8802649 [details]
Bug 1310744: Add NativeObject::denseElementsAreFrozen.

Fix a regression. Take it in 51 aurora.
Attachment #8802649 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1312948
Depends on: 1312485
Version: unspecified → 51 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: