958972 - Asm.js code fails to link in SDK addon

Status: REOPENED

(Core :: XPConnect, defect)

Description

[Peter J. Sloetjes]

Attachment: AsmXULWin.xpi

User Agent: Mozilla/5.0 (masking-agent; rv:17.0) Gecko/17.0 Firefox/17.0 (Nightly/Aurora)
Build ID: 20140110030650

Steps to reproduce:

Asm.js code in the context of a XUL window is parsed correctly but fails to compile or link.


Actual results:

Upon installing an extension which tries to run asm.js code in the context of a XUL window, instead of a message noting successful compilation, the message "asm.js type error: Disabled by javascript.options.asmjs in about:config" is displayed.


Expected results:

The solution is probably to enable AOT asm.js compilation on the browser-internal JSContext which is used to execute the asm.js code (according to Luke Wagner).

A bootstrapped extension is provided as a test case. It shows failure only during installation!

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Upon installing an extension which tries to run asm.js code in the context of a XUL
> window

Window or JS component?  Because XUL windows, just like web windows, just use the javascript.options.asmjs preference, and should have the same behavior as web windows.

Comment 2

[Peter J. Sloetjes]

It might well be that linking to extension code is the real issue, in which case "Asm.js code fails to link to a JS component" would be a better bug title. However, I am not even sure that it is merely linking that fails, given that no message is shown to note successful compilation (with a XUL window as stdlib). Perhaps implementing separate link errors would clarify things.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So looking at the attached extension, I very much doubt that it runs in a XUL window.  

And in fact if I add a breakpoint in "startup" and examine what's going on, I have a cx->options_ like so:

$3 = {
  extraWarnings_ = false, 
  werror_ = false, 
  varObjFix_ = false, 
  privateIsNSISupports_ = false, 
  dontReportUncaught_ = true, 
  noDefaultCompartmentObject_ = false, 
  noScriptRval_ = false, 
  strictMode_ = false, 
  baseline_ = false, 
  typeInference_ = false, 
  ion_ = false, 
  asmJS_ = false
}

which is ... not helpful, per the JS engine defaults.  Can we just fix those defaults already?  :(

The entrypoint into JS was via the eventloop calling an XPCWrappedJS runnable, looks like; the JSContext we're running on is mozJSComponentLoader::sSelf->mContext.

Then when the load event fires we come in via WebIDL callback codegen calling into the JS component, so we run on nsContentUtils::GetSafeJSContext().  In both cases everything is disabled.

So the JS component loader thing is basically bug 776798.  Not sure whether we have a bug for enabling these things on the safe context.  But again, why does the JS engine disable all this stuff by default?  :(

Comment 4

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> So the JS component loader thing is basically bug 776798.  Not sure whether
> we have a bug for enabling these things on the safe context.  But again, why
> does the JS engine disable all this stuff by default?  :(

Probably because tracemonkey did it first to be conservative and then everyone grepped for tracing's JSOPTION and added their own.  I'd be fine flipping the defaults.  What about jorendorff and jandem?

Wow, context options have been majorly refactored since last I looked.  Is it as easy as just flipping the default value in the ContextOptions constructor?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #4)
> I'd be fine
> flipping the defaults.  What about jorendorff and jandem?

For bug triage it would still be nice to have browser prefs that turn off Ion/asm.js/Baseline/TI everywhere, no exceptions. I guess the browser prefs could also set some process-wide static bool...

Eddy, what's the status of bug 939562?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #5)
> For bug triage it would still be nice to have browser prefs that turn off
> Ion/asm.js/Baseline/TI everywhere, no exceptions. I guess the browser prefs
> could also set some process-wide static bool...

That would be "Safe Mode", which is currently implemented in nsJSContext.cpp, and may need some refactoring once Eddy's stuff is landed (I'm curious about the status as well).

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> So the JS component loader thing is basically bug 776798.  Not sure whether
> we have a bug for enabling these things on the safe context.  But again, why
> does the JS engine disable all this stuff by default?  :(

Eddy was inches from fixing all of this last we talked about it in November FWIW.

Comment 8

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #3)
> > So the JS component loader thing is basically bug 776798.  Not sure whether
> > we have a bug for enabling these things on the safe context.  But again, why
> > does the JS engine disable all this stuff by default?  :(
> 
> Eddy was inches from fixing all of this last we talked about it in November
> FWIW.

I was, but I got stuck on the very last step: moving the JIT flags from the context to the runtime. The last time I tried doing this I observed random test failures due to timeouts. What is probably going on is that several tests for which the JIT flags were previously enabled are now run with the JIT flags turned off, so moving these flags to the runtime subtly changed our behavior.

I've tried to ascertain *how* exactly our behavior is changed by this patch, but so far I've been unable to reproduce any of these timeouts locally, and its never the same tests that time out on try. I really have no idea how to figure out why my patch causes timeouts if I cannot compare results between tests with and without my test applied.

After trying for several weeks, I got frustrated, and decided to get back to my main project (making workers debuggable) for a while. I'd like to get back to this bug and finish it up, but I really need a bit more guidance in figuring out what is going on, otherwise I'm likely to get stuck again.

To both bobby and bz: let's talk about this on irc tomorrow morning? I have a team meeting between 10 and 11, so I can do either before or after that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sure.  I'm free anytime tomorrow.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that runtime-wide flags will mean getting rid of the content/chrome difference in JIT flags, btw.

Comment 11

[Eddy Bruel [:ejpbruel]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Note that runtime-wide flags will mean getting rid of the content/chrome
> difference in JIT flags, btw.

That should be okay. If I recall correctly, enabling the JIT flags for chrome is only problematic for XPCShell at this point, so we can just enable them everywhere except there.

Jandem, I know we talked about this last year, but I'm not sure I remember our conversation correctly. Can you please confirm the above?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I recommend you flip those prefs _now_ then, to separate the effects of that from the patch that moves flags to the runtime....

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> That should be okay. If I recall correctly, enabling the JIT flags for
> chrome is only problematic for XPCShell at this point, so we can just enable
> them everywhere except there.
> 
> Jandem, I know we talked about this last year, but I'm not sure I remember
> our conversation correctly. Can you please confirm the above?

Yes that's correct. As long as we have browser flags for Baseline, Ion and asm.js that work everywhere (content, chrome, workers, add-ons) I'm happy.

I'm afraid if we turn the JSContext JIT flags on by default, we'll forget to flip them somewhere if we are in Safe Mode or if the prefs are disabled (think workers, sandboxes and what not). But maybe that's not a huge deal.

Comment 14

[Luke Wagner [:luke]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> I'm afraid if we turn the JSContext JIT flags on by default, we'll forget to
> flip them somewhere if we are in Safe Mode or if the prefs are disabled
> (think workers, sandboxes and what not). But maybe that's not a huge deal.

Ah right, that's a pretty good reason.  Clearly, if we just flipped the defaults now, we'd be missing a few places already.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #13)
> I'm afraid if we turn the JSContext JIT flags on by default, we'll forget to
> flip them somewhere if we are in Safe Mode or if the prefs are disabled
> (think workers, sandboxes and what not). But maybe that's not a huge deal.

That's probably worse than the inverse, FWIW. Once this stuff is all a single pref on the runtime, and not tied up with JSContexts, this stuff should all be simple to do anyhow.

Eddy, if I recall, the next thing you were going to do was to do a try push and see what happens when you enable TI everywhere except XPCShell. What did the results look like?

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> I recommend you flip those prefs _now_ then, to separate the effects of that
> from the patch that moves flags to the runtime....

We could turn on TI and Ion for chrome right now (prefs are there), but it will break xpcshell (bug 931861). Apparently xpcshell also depends on the browser prefs, or at least some tests do. bholley/bz, can we work around that somehow?

Or maybe I should just go ahead and fix bug 931861 myself. I do like the extra JIT coverage we'd get from xpcshell tests...

Comment 17

[Bobby Holley (:bholley)]

Eddy's patches give the ability to just disable TI on the runtime, so we could do that very simply with an explicit call in XPCShellImpl.cpp.

Comment 18

[Peter J. Sloetjes]

Most app developers consider going native, but with asm.js it seems advisable to move from extension code to web code :) Has any progress been made recently?

Comment 19

[Jan de Mooij [:jandem]]

(In reply to pjs.nl from comment #18)
> Most app developers consider going native, but with asm.js it seems
> advisable to move from extension code to web code :) Has any progress been
> made recently?

Yes I'm working on this atm (bug 939562).

Comment 20

[Peter J. Sloetjes]

Great! I'm only slightly aware of the involved complexity by following some of these bugs, but I think it's amazing that issues as complex as this can be brought to a good end by different developers. I'm looking forward to using asm.js in extensions :)

Comment 21

[Luke Wagner [:luke]]

bz: does extension code ever get XDR'd into the fastload cache?  The reason I ask is that addon code appears to be compiled with !compileAndGo.  Currently compileAndGo is required for asm.js-specific optimizations since we don't currently support XDR on asm.js modules (although we could without a ton of work).

Comment 22

[Luke Wagner [:luke]]

I should add that, even if asm.js-specific optimizations are disabled, as long as JIT compilation is turn on (which Jan is working on), your asm.js will still run very fast.

Comment 23

[Peter J. Sloetjes]

Well, I am mainly interested in speeding up pieces of hot extension code which are already within 2.5 times the speed of optimized native code - so I guess I am asking for full asm.js optimization, when needed accompanied by a move from doubles to floats. On systems with a lousy GPU, that seems to be the only way to save time and energy.

Comment 24

[Luke Wagner [:luke]]

Ok, well, we'll be fixing the XDR (and CloneScript) issue pretty soon anyhow, so that should remove the compileAndGo restriction.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> does extension code ever get XDR'd into the fastload cache?

As far as I know, yes...

Comment 26

[Peter J. Sloetjes]

Attachment: AsmChromeWorker.xpi

Perhaps this small bootstrapped extension might prove useful for testing the sub-goal of caching asm.js extension scripts which are called from a ChromeWorker. As I noticed recently, those scripts compile fine and have been doing so for quite some time, provided that they are linked only once.

Comment 27

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.