893434 - asm.js: getters executed twice when link error occurs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

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.

Comment 1

[Luke Wagner [:luke]]

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.

Comment 2

[Luke Wagner [:luke]]

Thanks for reporting btw!

Comment 3

[André Bargull [:anba]]

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

Comment 4

[Luke Wagner [:luke]]

Attachment: fix and tests

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 776634 [details] [diff] [review]
fix and tests

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

Nice!

Comment 6

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17404ee140f3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for making test_asmjs.html perma-timeout on Android.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3437dad963

https://tbpl.mozilla.org/php/getParsedLog.php?id=25440154&tree=Mozilla-Inbound

Comment 8

[Luke Wagner [:luke]]

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

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84804eb3f8e2