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)

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed
firefox-esr45 --- affected

People

(Reporter: gkw, Assigned: h4writer)

Details

(Keywords: testcase)

Attachments

(1 file)

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)
This issue was first found while fuzzing the mozilla-esr45 branch, and found that the reduced testcase also affects mozilla-central.
Hannes can you take this? I've a lot of bugs and crashes on my plate atm.
Flags: needinfo?(jdemooij)
sure
Attached patch PatchSplinter Review
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.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8742899 - Flags: review?(jdemooij)
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, &notEmpty);
> +
> +        // 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+
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.
(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).
Is bug 858381 the real regressor then? If so, it probably affects all branches. Please nominate as you see fit.
(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.
(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.
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.