Closed Bug 1471788 Opened 2 years ago Closed 2 years ago

Inconsistent output compared with other JS engines

Categories

(Core :: JavaScript Engine, defect)

60 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sunlili, Assigned: anba)

References

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.87 Safari/537.36

Steps to reproduce:

Execute following code:

v1=new (Float64Array)();
v2={
    valueOf : function (){ 
        v3.y="bar";
        return 42;}
    };  
v3=v1;
v3[0]=v2;
print(JSON.stringify(v1));


Actual results:

{}


Expected results:

{"y":"bar"}
The test case is correct, but I'm still going to file a spec issue for this one, because I don't like how [[DefineOwnProperty]]  [1] performs bounds checks before calling IntegerIndexedElementSet, whereas [[Set]] [2] directly calls IntegerIndexedElementSet and then relies on IntegerIndexedElementSet [3] performing the bounds checks. Maybe 9.4.5.3 [[DefineOwnProperty]], steps 3.b.i-v should just be removed?

[1] https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-defineownproperty-p-desc
[2] https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-set-p-v-receiver
[3] https://tc39.github.io/ecma262/#sec-integerindexedelementset
Attached patch bug1471788.patch (obsolete) — Splinter Review
Applies on top of bug 1471841.


Adds the missing ToNumber call to DefineNonexistentProperty.

Also updates/adds step comments and corrects the spec reference in DefineNonexistentProperty: Despite its name, DefineNonexistentProperty when called with a typed array, implements 9.4.5.5 [[Set]], not 9.4.5.3 [[DefineOwnProperty]]!

And a drive-by change in IsTypedArrayIndex to use a more sensible cast.
Assignee: nobody → andrebargull
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8988483 - Flags: review?(evilpies)
Comment on attachment 8988483 [details] [diff] [review]
bug1471788.patch

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

Nice, thanks for adding the steps.
Attachment #8988483 - Flags: review?(evilpies) → review+
Attached patch bug1471788.patch (obsolete) — Splinter Review
Rebased to apply cleanly on inbound, carrying r+.
Attachment #8988483 - Attachment is obsolete: true
Attachment #8988682 - Flags: review+
Hi, I am unable to apply your patch after importing it.

This is the error I get when doing so:

hg qpush -a 
applying bug1471788.patch
patching file js/src/vm/NativeObject.cpp
Hunk #2 FAILED at 2657
1 out of 2 hunks FAILED -- saving rejects to file js/src/vm/NativeObject.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1471788.patch.

Can you please check it?
Flags: needinfo?(andrebargull)
Attached patch bug1471788.patchSplinter Review
Rebased patch to apply cleanly on inbound.
Attachment #8988682 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #8988878 - Flags: review+
(In reply to Dorel Luca [:dluca] from comment #6)
> Hi, I am unable to apply your patch after importing it.

The patch needed to be rebased again, sorry for the trouble!
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3030106b14f7
Apply ToNumber conversion even when the typed array write is out-of-bounds. r=evilpie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3030106b14f7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Regressions: 1574415
You need to log in before you can comment on or make changes to this bug.