Closed
Bug 1265159
Opened 8 years ago
Closed 8 years ago
Differential Testing: Different output message involving Object.freeze
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: gkw, Assigned: h4writer)
Details
(Keywords: testcase)
Attachments
(1 file)
1.66 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
try { x = [0]; for (var i = 0; i < 32; ++i) { if (i % 39 == 30) { Object.freeze(x); } else { x.pop(); } } } catch (e) { print(e); } $ ./js-dbg-64-dm-clang-darwin-1da1937a9e03 --fuzzing-safe --no-threads --baseline-eager testcase.js TypeError: "length" is read-only $ $ ./js-dbg-64-dm-clang-darwin-1da1937a9e03 --fuzzing-safe --no-threads --ion-eager testcase.js $ Tested this on m-c rev 1da1937a9e03. My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 1da1937a9e03 This seems to go back beyond m-c rev dc4b163f7db7 (Fx36, early Nov 2014), so setting needinfo? from Jan/Hannes as a start.
Flags: needinfo?(jdemooij)
Flags: needinfo?(hv1989)
Reporter | ||
Comment 1•8 years ago
|
||
This issue was first found while fuzzing the mozilla-esr45 branch, and found that the reduced testcase also affects mozilla-central.
Comment 2•8 years ago
|
||
Hannes can you take this? I've a lot of bugs and crashes on my plate atm.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 3•8 years ago
|
||
sure
Assignee | ||
Comment 4•8 years ago
|
||
When doing array.pop() on an empty array, we need to set array.length to 0. It is already 0 so we don't do anything. This is not entirely correct, since if the array.length is made not writable we should throw.
Comment 5•8 years ago
|
||
Comment on attachment 8742899 [details] [diff] [review] Patch Review of attachment 8742899 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/jit/CodeGenerator.cpp @@ +8087,5 @@ > if (mir->maybeUndefined()) { > Label notEmpty; > masm.branchTest32(Assembler::NonZero, lengthTemp, lengthTemp, ¬Empty); > + > + // According to the spec we need to set the length 0 (which is already 0). I think we should wrap this in an |if (mir->unboxedType() == JSVAL_TYPE_MAGIC)| If that's false, we're loading from an unboxed array and elementsTemp is not an ObjectElements header.
Attachment #8742899 -
Flags: review?(jdemooij) → review+
Comment 6•8 years ago
|
||
Is this a regression? And if so, when? 'cause I tried reeeeeeeeeaallly hard to not introduce these problems when fixing bug 858381 and would like to know if I done goofed.
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6) > Is this a regression? And if so, when? 'cause I tried reeeeeeeeeaallly > hard to not introduce these problems when fixing bug 858381 and would like > to know if I done goofed. It was a minor correction issue. Expected result - pop on an empty frozen array should throw the length cannot get adjusted. Actual result - pop on an empty array was unconditional returning undefined. (in the MArrayPop/Shift MIR).
Reporter | ||
Comment 8•8 years ago
|
||
Is bug 858381 the real regressor then? If so, it probably affects all branches. Please nominate as you see fit.
Comment 10•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #7) > It was a minor correction issue. Yes, I agree. The question is whether it's one that dates to the introduction of non-writable array lengths or whether it was introduced after that time.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > (In reply to Hannes Verschore [:h4writer] from comment #7) > > It was a minor correction issue. > > Yes, I agree. The question is whether it's one that dates to the > introduction of non-writable array lengths or whether it was introduced > after that time. Just checked the patch that landed in bug 858381. This issue was introduced at the introduction of non-writable array.lengths.
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ebf23fb059f5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•