Closed Bug 893434 Opened 7 years ago Closed 7 years ago

asm.js: getters executed twice when link error occurs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: anba, Assigned: luke)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

test case:
---
js> stdlib = {get Math(){print("get Math"); return this._Math}, _Math: {get sin(){print("get sin"); return Math.cos}}};
({get Math (){print("get Math"); return this._Math}, _Math:{get sin (){print("get sin"); return Math.cos}}})
js> (function(stdlib){"use asm"; var sin = stdlib.Math.sin; return {}; })(stdlib)
warning: successfully compiled asm.js code (total compilation time 0ms)
get Math
get sin
typein:3:1 warning: asm.js link error: bad Math.* builtin
get Math
get sin
({})
---


Actual results:

Getters for "Math" and "sin" are executed twice.


Expected results:

Getters for "Math" and "sin" should only be executed once.
OS: Windows 7 → All
Hardware: x86_64 → All
On first consideration, I think the simplest solution to this corner case is to restrict the dynamic linking rules to require that the properties accessed in the global imports section be data properties.  This avoids a rather messy problem where we've executed N getters, we fail, we reparse the script from source, and now need to jump into the middle of this script to where we failed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Thanks for reporting btw!
And most likely also restrict to non-Proxy objects, because the double execution issue can be triggered in that case, too.

js> var stdlib = Proxy({},{get: (_,pk)=>{print("pk="+pk); return this}});         
js> (function(stdlib){"use asm"; var sin = stdlib.Math.sin; return {}; })(stdlib)
warning: successfully compiled asm.js code (total compilation time 0ms)
pk=Math
typein:8:1 warning: asm.js link error: bad Math.* builtin
pk=Math
Attached patch fix and testsSplinter Review
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #776634 - Flags: review?(bbouvier)
Comment on attachment 776634 [details] [diff] [review]
fix and tests

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

Nice!
Attachment #776634 - Flags: review?(bbouvier) → review+
So, I should have used the canonical GetPropertyDescriptor to detect data-property-ness.  The previous predicate rejected transparent wrappers (so, outer windows).  The mysterious timeout is due to the link-time validation checks failing in the mochitest which causes the code to run in non-asm.js mode (this didn't show up in the Emscripten demos I tested since Emscripten doesn't pass in the global object).  There were assertions in the mochitest to detect these link-time validation errors, but of course these asserts were botched too :/  All fixed now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/84804eb3f8e2
https://hg.mozilla.org/mozilla-central/rev/84804eb3f8e2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.