Closed Bug 965970 Opened 10 years ago Closed 10 years ago

Add support to compile asm.js code at install/update time

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(tracking-b2g:backlog, firefox29 wontfix, firefox30 wontfix, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
mozilla31
tracking-b2g backlog
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [games][qa-])

Attachments

(1 file, 6 obsolete files)

For packaged apps shipping asm.js code, we want to compile this code at install time and when gecko or app updates happen. That provides better user experience since the first launch won't be slowed down.

Apps will opt-in to this mechanism by adding a property to their manifest listing the js resources that need to be compiled.
\o/
Whiteboard: [games]
Will this feature work on any FF platform that supports installing packaged apps or only FFOS?  (For that matter, are there any FF platforms other than FFOS on which you can install a packaged app?)
(In reply to Luke Wagner [:luke] from comment #2)
> For that matter, are there any FF platforms other than FFOS on which you can install a packaged app?

You can install packaged apps on both desktop (Windows, Mac, Linux) and Android (although the desktop runtime doesn't yet support signed packaged apps, and most packaged apps are signed).
(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)
Cool!  I'm guessing this is the new "App Manager" entry in the developer tools menu.
The way we install apps is quite different on desktop - each app has its own profile, so that will need some more work here I guess. I never remember if we share the same profile on android or not, so calling Myk to the rescue on this one!
Flags: needinfo?(myk)
(In reply to Fabrice Desré [:fabrice] from comment #5)
> The way we install apps is quite different on desktop - each app has its own
> profile, so that will need some more work here I guess. I never remember if
> we share the same profile on android or not, so calling Myk to the rescue on
> this one!

Each app gets its own profile on Android, so presumably we'll need more work there too.
Flags: needinfo?(myk)
A few notes:

For the 'obj' parameter to CompileOffThread, I would create a throwaway sandbox and pass the global object of that sandbox.  (That way the GC will easily clear all the resulting compiled scripts when you're done.)  You should also enter the compartment of the sandbox global:
  AutoCompartment ac(cx, global);
(using whatever random 'cx' you have lying around) before calling CompileOffThread.

After default-constructing a CompileOptions(cx, JSVERSION_DEFAULT), the options to set are:
 - setSourcePolicy(CompileOptions::NO_SOURCE)
 - asmJSOption = true
 - forceAsync = true

Lastly, bug 963588 adds a new 'installedFile' flag to CompileOptions (passed to CompileOffThread).  Setting this flag will place any asm.js cache entries added during compilation in persistent storage so they won't get evicted.
Attached patch wip (obsolete) — Splinter Review
Luke, here's my current wip. It's crashing in the js parser:

--*-- AsmjsPreloader: Preloading app://{6c1696d4-a825-4476-9e98-7b0729726c87}
--*-- AsmjsPreloader: asmJS file: app://{6c1696d4-a825-4476-9e98-7b0729726c87}/game.js
XXX script=0x7fffb8d00008 length=13619942

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffdd8f7700 (LWP 13423)]
0x00007ffff49e162a in js::frontend::TokenStream::TokenBuf::getRawChar (this=0x7fffdd8f6780) at /home/fabrice/dev/birch/js/src/frontend/TokenStream.h:771
771	            return *ptr++;      // this will nullptr-crash if poisoned
(gdb) bt
#0  0x00007ffff49e162a in js::frontend::TokenStream::TokenBuf::getRawChar (this=0x7fffdd8f6780) at /home/fabrice/dev/birch/js/src/frontend/TokenStream.h:771
#1  0x00007ffff49a0c34 in js::frontend::TokenStream::getTokenInternal (this=0x7fffdd8f64b0, modifier=js::frontend::TokenStream::Operand)
    at /home/fabrice/dev/birch/js/src/frontend/TokenStream.cpp:1068
#2  0x00007ffff49864a5 in js::frontend::TokenStream::peekToken (this=0x7fffdd8f64b0, modifier=js::frontend::TokenStream::Operand)
    at /home/fabrice/dev/birch/js/src/frontend/TokenStream.h:523
#3  0x00007ffff4957bde in js::frontend::CompileScript (cx=0x7fffcf739380, alloc=0x7fffd0996e98, scopeChain=..., evalCaller=..., options=..., 
    chars=0x7fffb8d00008 <Address 0x7fffb8d00008 out of bounds>, length=13619942, source_=0x0, staticLevel=0, extraSct=0x0)
    at /home/fabrice/dev/birch/js/src/frontend/BytecodeCompiler.cpp:318
#4  0x00007ffff4d1d47b in js::WorkerThread::handleParseWorkload (this=0x7fffdd6479b0) at /home/fabrice/dev/birch/js/src/jsworkers.cpp:855
#5  0x00007ffff4d1dd16 in js::WorkerThread::threadLoop (this=0x7fffdd6479b0) at /home/fabrice/dev/birch/js/src/jsworkers.cpp:1032
#6  0x00007ffff4d1cb1e in js::WorkerThread::ThreadMain (arg=0x7fffdd6479b0) at /home/fabrice/dev/birch/js/src/jsworkers.cpp:712
#7  0x00007ffff6aad0be in _pt_root (arg=0x7ffff6c60ee0) at /home/fabrice/dev/birch/nsprpub/pr/src/pthreads/ptthread.c:212
#8  0x00007ffff7bc6e0e in start_thread (arg=0x7fffdd8f7700) at pthread_create.c:311
#9  0x00007ffff6ee00fd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:113
(gdb)


Do you see anything obviously wrong in this patch?
Attachment #8385871 - Flags: feedback?(luke)
Ah, yes, I think so: the patch decodes the chars into a 'script' string variable on the stack and then passes this to CompileOffThread() which does not make a copy before the function returns and 'script' is destroyed.  You probably want to make 'script' a member variable of AsmJSCompileLoader.
Comment on attachment 8385871 [details] [diff] [review]
wip

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

Glad to see this shaping up, btw!

::: js/xpconnect/src/XPCComponents.cpp
@@ +3772,5 @@
> +    aObserver->Observe(uri, nullptr, nullptr);
> +}
> +
> +NS_IMETHODIMP
> +nsXPCComponents_Utils::CompileAsmJS(nsIURI *aURI, nsIObserver *aObserver,

Random bikeshedding request: since this mechanism may be reused for other times of install-time caching, could you leave 'AsmJS' out of all the identifiers and just use a generic term like "PrecompileScript" or something?
Attachment #8385871 - Flags: feedback?(luke)
Comment on attachment 8385871 [details] [diff] [review]
wip

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

Nevermind, I found the issue.
Attachment #8385871 - Attachment is obsolete: true
Attached patch wip preload-asm-js.patch (obsolete) — Splinter Review
With this patch I've been able to install a game with asm.js code, and everything works as expected. At first launch instead of waiting 20s (on a desktop build) we get:
JavaScript warning: app://{3adc6f9a-9f03-408f-9879-311976cd45a5}/game.js, line 0: Successfully compiled asm.js code (loaded from cache in 418ms)

Still missing from this patch:
- a couple of paths in Webapps.jsm (download retries and updates).
- what to do if a file previously compiled should not be compiled anymore after an update.
- tests.
Attachment #8386590 - Flags: feedback?(luke)
Comment on attachment 8386590 [details] [diff] [review]
wip preload-asm-js.patch

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

\o/  Beautiful.  Thanks a lot Fabrice.

Is it possible to emit a warning if the 'preload' option is set but the 'indexedDB-unlimited' option
  http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#229
is not set?  The reason is that, in bug 963588 (which should land soon), even if options.installedFile is set, we only store the cache entry in persistent storage if the app has the indexedDB-unlimited storage permission (this option name will later be renamed to something that isn't indexedDB-specific) to avoid some tricky quota corner cases we'd otherwise have to deal with.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3655,5 @@
> +/**
> +  * Let us compile scripts from a URI off the main thread.
> +  */
> +
> +class ScriptLoader : public nsIStreamLoaderObserver

Maybe name it "ScriptPreloader" or "BackgroudScriptCompiler" to distinguish from nsScriptLoader?

@@ +3726,5 @@
> +    JSObject* scope =
> +        JS_NewGlobalObject(cx, &sGlobalClass,
> +                           nsJSPrincipals::get(mPrincipal),
> +                           JS::FireOnNewGlobalHook);
> +    JS::Rooted<JSObject*> global(cx, scope);

To avoid leaving a possibly-stale pointer in 'scope', I'd do:

JS::Rooted<JSObject*> global(cx,
    JS_NewGlobalObject(...));

@@ +3742,5 @@
> +    nsAutoCString spec;
> +    uri->GetSpec(spec);
> +    options.setFile(spec.get());
> +
> +    if (!JS::CanCompileOffThread(cx, options, mScript.Length())) {

With forceAsync, the only way this can fail is if !cx->runtime()->canUseParallelParsing which is default true, even on single-core devices.  Thus, if this returns false, there is some configuration error involving the different prefs involved.  It's still worth checking, but, if this fails, is there any way to issue a "really loud" warning that would show up in whatever logs a FFOS dev would see?  (Same for any failure in this function, actually.)

@@ +3751,5 @@
> +    nsCOMPtr<nsIRunnable> runnable =
> +        NS_NewRunnableMethod(this, &ScriptLoader::SendNotification);
> +
> +    // This reference will be consumed by OffThreadCallback.
> +    NS_ADDREF(runnable);

The !JS::CompileOffThread failure path doesn't release this.  Perhaps use an nsRefPtr that is .Forget()'d after JS::ComileOffThread succeeds?
Attachment #8386590 - Flags: feedback?(luke) → feedback+
Bug 963588 landed so now you can add options.installedFile = true to your compilation options.  Right now there is, unfortunately, no way that I know of to test whether a cache file is stored in persistent or temporary storage.  Probably we can just add a nsIQuotaManager.clearAllTemporaryStorages() (like clearAllStorages()) and then assert in the test that we get a cache hit after that.  For now, though, you can just look in $(PROFILE)/storage/persistent/$(APP)/asmjs to see if you have an entry :)
I think we should call the preload function only on b2g, because on Firefox and Android the data stored in the main profile is useless for apps.
Per Martin Best, nom'ing this for 1.4. Without this we can't ship important tier 1 game titles that require emscripten as their startup times will be too long
blocking-b2g: --- → 1.4?
We can't block on this - we aren't accepting new features into 1.4 at this time. We can consider this for a future release though.
blocking-b2g: 1.4? → backlog
Hey guys, if you don't block on this, you are saying that you don't want to ship WMW on 1.4.  We have been pushing for this for a very long time and it blocks cache on install.
Martin

Did we support MWM in 1.4? Or is it something we're developing newly?
Flags: needinfo?(mbest)
Preeti, this has been pushed out from other releases and it's going to become a bit partner issue if we don't get this in so it's likely a good candidate for an exception. Fabrice is working on the patch, says it's fairly low risk and that we should be able to get it in. I defer to him on status of the actual patch.
(In reply to Sheila Mooney from comment #20)
> Preeti, this has been pushed out from other releases and it's going to
> become a bit partner issue if we don't get this in so it's likely a good
> candidate for an exception. Fabrice is working on the patch, says it's
> fairly low risk and that we should be able to get it in. I defer to him on
> status of the actual patch.

FWIW - I think that's a case where we should consider this patch for approval, but I don't think we would hold the release for this.

Note - the last set of dev activity was over a month ago, so if this is needed to be fixed for 1.4, then we're going to need to get traction on this quickly.
Attached patch preload-asm-js.patch v2 (obsolete) — Splinter Review
That works fine on desktop builds, still need to test on a 512MB device.

Luke, please review the anything not in dom/apps, and Marco the dom/apps parts.
Attachment #8406533 - Flags: review?(mar.castelluccio)
Attachment #8406533 - Flags: review?(luke)
Comment on attachment 8406533 [details] [diff] [review]
preload-asm-js.patch v2

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

Thanks!  This is a promising start, I think I found some bugs so we'll have another iteration or two.

More importantly, I'm not an XPConnect peer, so I can't give the final r+ for XPCComponents.cpp.  I think it's likely bholley will think that nsIXPCComponents_Utils/XPCComponents.cpp is the wrong place to put this.  js/xpconnect/loader, on the other hand, has a lot of similar code, so probably that would be a good place, but we should ask bholley first.

::: content/base/src/nsScriptLoader.cpp
@@ +857,5 @@
>    if (!JS::CompileOffThread(cx, options,
>                              aRequest->mScriptText.get(), aRequest->mScriptText.Length(),
>                              OffThreadScriptLoaderCallback,
>                              static_cast<void*>(runnable))) {
> +    NS_RELEASE(runnable);

Good catch.

::: js/xpconnect/idl/xpccomponents.idl
@@ +574,5 @@
> +    /*
> +     * Compiles a JS script off the main thread and calls back the
> +     * observer once it's done.
> +     */
> +    void compileScript(in nsIURI uri,

This sort of name tends to imply that the resulting script is returned in some way.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3620,5 @@
> +/**
> +  * Let us compile scripts from a URI off the main thread.
> +  */
> +
> +class BackgroundScriptCompiler : public nsIStreamLoaderObserver

Perhaps we could name this ScriptPrecompiler?

@@ +3667,5 @@
> +                                           const uint8_t* aString)
> +{
> +    nsCOMPtr<nsIRequest> request;
> +    aLoader->GetRequest(getter_AddRefs(request));
> +    mChannel = do_QueryInterface(request);

Not my area of expertise, but could you avoid this step by passing the nsIChannel to BackgroundScriptCompiler's constructor?

@@ +3689,5 @@
> +
> +    AutoJSContext cx;
> +    JS::Rooted<JSObject*> global(cx,
> +        JS_NewGlobalObject(cx, &sGlobalClass,
> +                           nsJSPrincipals::get(mPrincipal),

Can you add a little comment explaining that our goal is to cache persistently and avoid quota checks and that caching mechanisms decide the persistence type based on the principal and that this is why we create a new global with the app's principal: so we can enter its compartment and therefore compile with its principal.

@@ +3699,5 @@
> +    options.asmJSOption = true;
> +    options.forceAsync = true;
> +    options.compileAndGo = true;
> +    options.installedFile = true;
> +    options.setOriginPrincipals(nsJSPrincipals::get(mPrincipal));

You can remove the setOriginPrincipals; originPrincipals() will correctly default to the compartment's current principals.

@@ +3708,5 @@
> +    uri->GetSpec(spec);
> +    options.setFile(spec.get());
> +
> +    if (!JS::CanCompileOffThread(cx, options, mScript.Length())) {
> +        printf_stderr("Can't compile script off thread! %s\n", uri.get());

I assume the right warning mechanism for this is NS_WARNING?

@@ +3738,5 @@
> +BackgroundScriptCompiler::OffThreadCallback(void *aToken, void *aData)
> +{
> +    nsCOMPtr<nsIRunnable> runnable = static_cast<nsIRunnable*>(aData);
> +
> +    NS_DispatchToMainThread(runnable);

This skips the important step of calling JS::FinishOffThreadScript() on the main thread.  Even though you will be ignoring the return value, FinishOffThreadScript effectively releases the resources consumed by the compiled script which would otherwise be leaked.

@@ +3754,5 @@
> +    mObserver->Observe(uri, nullptr, nullptr);
> +}
> +
> +NS_IMETHODIMP
> +nsXPCComponents_Utils::CompileScript(nsIURI *aURI,

* with the type

@@ +3764,5 @@
> +                                aURI, nullptr, nullptr, nullptr,
> +                                nsIRequest::LOAD_NORMAL, nullptr);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsIStreamLoader> loader;

Can you move 'loader' down to right before NS_NewStreamLoader?

@@ +3768,5 @@
> +    nsCOMPtr<nsIStreamLoader> loader;
> +    nsCOMPtr<BackgroundScriptCompiler> bgObserver =
> +        new BackgroundScriptCompiler(aObserver, aPrincipal);
> +
> +    NS_ADDREF(bgObserver);

I can't find the paired NS_RELEASE.  Is this something magic to do with NS_NewStreamLoader?  Whatever the case, a comment explaining would be good.
Attachment #8406533 - Flags: review?(luke)
needinfo? bholley for opinion on comment 23 (question at the top).
Flags: needinfo?(bobbyholley)
Yeah, Cu is pretty crowded. Let's stick it on mozIJSSubscriptLoader, even though it's not entirely accurate name-wise.
Flags: needinfo?(bobbyholley)
(In reply to Luke Wagner [:luke] from comment #23)
> > +    void compileScript(in nsIURI uri,
> 
> This sort of name tends to imply that the resulting script is returned in
> some way.

Oops, looks like I cut off before suggesting the name 'precompileScript'.
Attached patch preload-asm-js.patch v3 (obsolete) — Splinter Review
I addressed review comments and moved everything to mozIJSSubScriptLoader.

Works fine on a Leo with 512MB and wmw!
Attachment #8386590 - Attachment is obsolete: true
Attachment #8406533 - Attachment is obsolete: true
Attachment #8406533 - Flags: review?(mar.castelluccio)
Attachment #8407113 - Flags: review?(mar.castelluccio)
Attachment #8407113 - Flags: review?(luke)
Attachment #8407113 - Flags: review?(bobbyholley)
Comment on attachment 8407113 [details] [diff] [review]
preload-asm-js.patch v3

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

Looks good in general. Just some small things.

::: content/base/src/nsScriptLoader.cpp
@@ +857,5 @@
>    if (!JS::CompileOffThread(cx, options,
>                              aRequest->mScriptText.get(), aRequest->mScriptText.Length(),
>                              OffThreadScriptLoaderCallback,
>                              static_cast<void*>(runnable))) {
> +    NS_RELEASE(runnable);

While you're here, can you fix this pattern to match the one suggested in mozJSSubScriptLoader?

::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
@@ +45,5 @@
>      jsval loadSubScriptWithOptions(in AString url, in jsval options);
> +
> +    /*
> +     * Compiles a JS script off the main thread and calls back the
> +     * observer once it's done.

We need more comments here explaining the impact of doing this (i.e. that the script gets cached by the JS engine on success).

@@ +50,5 @@
> +     * @param uri       the uri of the script to load.
> +     * @param principal the principal from which we get the app id if any.
> +     * @param observer  this observer will be called once the script has
> +     *                  been precompiled.
> +     *                  the topic will be 'script-precompiled' and the subject

Nit - incomplete sentence, and probably no need for the line break.

@@ +51,5 @@
> +     * @param principal the principal from which we get the app id if any.
> +     * @param observer  this observer will be called once the script has
> +     *                  been precompiled.
> +     *                  the topic will be 'script-precompiled' and the subject
> +     *                  the uri of the script as a nsIURI.

What happens if we fail to compile the script (either due to an error in the script or an internal error)? We need to handle that, and describe what happens here.

@@ +60,4 @@
>  };
> +
> +%{C++
> +#define SCRIPT_PRECOMPILED_TOPIC "script-precompiled"

This #define doesn't seem all that useful to me tbh.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +407,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS1(ScriptPrecompiler, nsIStreamLoaderObserver);
> +
> +const JSClass ScriptPrecompiler::sGlobalClass = {

Hm, why do we need this? Can't we just use a Sandbox (via xpc::CreateSandboxObject)?

@@ +414,5 @@
> +    JS_PropertyStub, JS_DeletePropertyStub, JS_PropertyStub,
> +    JS_StrictPropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub
> +};
> +
> +class ScriptPrecompilerRunnable : public nsRunnable

This could use a better name. NotifyPrecompilationCompleteRunnable?

@@ +499,5 @@
> +        SendObserverNotification();
> +        return NS_OK;
> +    }
> +
> +    nsCOMPtr<nsIRunnable> runnable =

This is dicey. If your runnable (or nsRunnable) ended up inheriting nsISupports twice, the cast in OffThreadCallback would be incorrect. Please use nsRefPtr<ConcreteType>, here and in OffThreadCallback.

@@ +503,5 @@
> +    nsCOMPtr<nsIRunnable> runnable =
> +        new ScriptPrecompilerRunnable(this);
> +
> +    // This reference will be consumed by OffThreadCallback.
> +    NS_ADDREF(runnable);

Instead of ADDREFing here and RELEASEing below, how about just doing runnable.forget() in the success branch?

@@ +522,5 @@
> +/* static */
> +void
> +ScriptPrecompiler::OffThreadCallback(void* aToken, void* aData)
> +{
> +    nsCOMPtr<ScriptPrecompilerRunnable> runnable =

I'm pretty sure that nsCOMPtr should only be used for interface types. Please use nsRefPtr here.

@@ +546,5 @@
> +    JSRuntime *rt;
> +    svc->GetRuntime(&rt);
> +    if (!rt) {
> +        return;
> +    }

Just do XPCJSRuntime::Get()->Runtime(); And really, this whole thing can just be inlined into the one caller, right?

@@ +572,5 @@
> +                                aURI, nullptr, nullptr, nullptr,
> +                                nsIRequest::LOAD_NORMAL, nullptr);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<ScriptPrecompiler> loadObserver =

nsRefPtr
Attachment #8407113 - Flags: review?(bobbyholley) → review-
Comment on attachment 8407113 [details] [diff] [review]
preload-asm-js.patch v3

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

Thanks for addressing the last round; this is looking great!  Just a few comments on top of Bobby's.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +414,5 @@
> +    JS_PropertyStub, JS_DeletePropertyStub, JS_PropertyStub,
> +    JS_StrictPropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub
> +};
> +
> +class ScriptPrecompilerRunnable : public nsRunnable

Alternatively, it seems like you could make ScriptPrecompiler derive nsRunnable which would reduce the number of moving parts.  Then ScriptPrecompilerRunnable::Run could just hold the body of Cleanup() (with a SendObserverNotification() at the end).

@@ +482,5 @@
> +    JSAutoCompartment ac(cx, global);
> +
> +    JS::CompileOptions options(cx, JSVERSION_DEFAULT);
> +    options.setSourcePolicy(CompileOptions::NO_SOURCE);
> +    options.asmJSOption = true;

You can remove the asmJSOption.  The ReadOnlyCompileOptions constructor may look like the default is 'false', but the CompileOptions constructor initializes it to cx->runtime()->options().asmJS which is controlled by the global pref (which we'd like to respect).
Attachment #8407113 - Flags: review?(luke)
Attached patch preload-asm-js.patch v4 (obsolete) — Splinter Review
Addressed comments except Luke's "merging the runnable".

Works really good on device, where we load the cached code for wmw in 2.5s
Attachment #8407113 - Attachment is obsolete: true
Attachment #8407113 - Flags: review?(mar.castelluccio)
Attachment #8407220 - Flags: review?(mar.castelluccio)
Attachment #8407220 - Flags: review?(luke)
Attachment #8407220 - Flags: review?(bobbyholley)
Comment on attachment 8407220 [details] [diff] [review]
preload-asm-js.patch v4

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

::: content/base/src/nsScriptLoader.cpp
@@ -817,5 @@
>    NotifyOffThreadScriptLoadCompletedRunnable* aRunnable =
>      static_cast<NotifyOffThreadScriptLoadCompletedRunnable*>(aCallbackData);
>    aRunnable->SetToken(aToken);
>    NS_DispatchToMainThread(aRunnable);
> -  NS_RELEASE(aRunnable);

From IRC: this change should be reverted.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +431,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +    MOZ_ASSERT(mPrecompiler);
> +
> +    MOZ_ASSERT(mChannel && mObserver);
> +    MOZ_ASSERT(NS_IsMainThread());

This assert is repeated.

@@ +435,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    if (mToken) {
> +        JSRuntime *rt = XPCJSRuntime::Get()->Runtime();
> +        NS_ENSURE_TRUE(rt, NS_ERROR_FAILURE);

I assume rt can never be null.  If it could, then this error return path would skip the call to SendObserverNotification.

@@ +476,5 @@
> +    RootedValue v(cx);
> +    SandboxOptions sandboxOptions;
> +    sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> +    rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);

This failure skips SendObserverNotification().

@@ +478,5 @@
> +    sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> +    rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
> +
> +    JSObject* global = js::UncheckedUnwrap(&(v.toObject()));

Generally we avoid naked JSObject* on the stack, so you'd need a RootedObject.  Alternatively, since this is only used by JSAutoCompartment, you could just
  JSAutoCompartment ac(cx, js::UncheckedUnwrap(&v.toObject()));
(The null assert doesn't seem very helpful since we'll immediately null-crash and UncheckedUnwrap is infallible anyhow.)
Attachment #8407220 - Flags: review?(luke) → review+
Comment on attachment 8407220 [details] [diff] [review]
preload-asm-js.patch v4

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

This is close, but I want to look at the resulting RAII class.

::: content/base/src/nsScriptLoader.cpp
@@ -817,5 @@
>    NotifyOffThreadScriptLoadCompletedRunnable* aRunnable =
>      static_cast<NotifyOffThreadScriptLoadCompletedRunnable*>(aCallbackData);
>    aRunnable->SetToken(aToken);
>    NS_DispatchToMainThread(aRunnable);
> -  NS_RELEASE(aRunnable);

This will leak. You either need to keep this line, or cast aCallbackData to an already_AddRefed and assign that into an nsRefPtr.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +389,5 @@
> +
> +    virtual ~ScriptPrecompiler()
> +    {}
> +
> +    /* The Off main thread callback */

nit: /* The off-main-thread callback */

But actually, this comment doesn't really add anything. Maybe just remove it?

@@ +392,5 @@
> +
> +    /* The Off main thread callback */
> +    static void OffThreadCallback(void *aToken, void *aData);
> +
> +    /* Sends the "done" notification back. Main thread only */

nit - add a period.

@@ +430,5 @@
> +{
> +    MOZ_ASSERT(NS_IsMainThread());
> +    MOZ_ASSERT(mPrecompiler);
> +
> +    MOZ_ASSERT(mChannel && mObserver);

This class has neither an mChannel nor an mObserver. How does this even compile?

@@ +435,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    if (mToken) {
> +        JSRuntime *rt = XPCJSRuntime::Get()->Runtime();
> +        NS_ENSURE_TRUE(rt, NS_ERROR_FAILURE);

Reply to luke - actually, it might be, in the case that this runnable runs at shutdown. Though in that case we probably don't care about leaking.

@@ +476,5 @@
> +    RootedValue v(cx);
> +    SandboxOptions sandboxOptions;
> +    sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> +    rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);

yeah, I think this is too error prone. We should add an RAII AutoSendObserverNotification class (with a .disarm()) that we can use both here and in NotifyPrecompilationCompleteRunnable.

@@ +478,5 @@
> +    sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> +    rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
> +
> +    JSObject* global = js::UncheckedUnwrap(&(v.toObject()));

Also, drop the extra parens and make that &v.toObject().

@@ +526,5 @@
> +        static_cast<NotifyPrecompilationCompleteRunnable*>(aData);
> +    runnable->SetToken(aToken);
> +
> +    NS_DispatchToMainThread(runnable);
> +    NS_RELEASE(runnable);

The refcounting here is confusing. We should stick a dont_AddRef around the static_cast and drop the NS_RELEASE.
Attachment #8407220 - Flags: review?(bobbyholley) → review-
Attached patch preload-asm-js.patch v5 (obsolete) — Splinter Review
Attachment #8407649 - Flags: review?(luke)
Attachment #8407649 - Flags: review?(ferjmoreno)
Attachment #8407649 - Flags: review?(bobbyholley)
Attachment #8407649 - Flags: review?(luke) → review+
Comment on attachment 8407649 [details] [diff] [review]
preload-asm-js.patch v5

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

r=bholley on the C++ bits with those two fixes. Thanks for doing this!

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +438,5 @@
> +        }
> +    }
> +
> +    void Disarm() {
> +        mDisarmed = true;

This could be done more simply by having disarm just null out mPrecompiler. No need for the extra piece of state.

@@ +490,5 @@
> +    // We then enter its compartment to compile with its principal.
> +    AutoSafeJSContext cx;
> +    RootedValue v(cx);
> +    SandboxOptions sandboxOptions;
> +    sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");

Note - given that this is an implementation detail, you should set sandboxOptions.invisibleToDebugger = true, so that the debugger won't be notified about these scopes.
Attachment #8407649 - Flags: review?(bobbyholley) → review+
Comment on attachment 8407649 [details] [diff] [review]
preload-asm-js.patch v5

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

::: dom/apps/src/ScriptPreloader.jsm
@@ +6,5 @@
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;

Nit: You could use destructuring assignments for clarity here (you could also avoid defining some of the constants unused in this file).

@@ +20,5 @@
> +}
> +
> +this.ScriptPreloader = {
> +
> +  preload: function(aApp, aManifest) {

I think we should reject the promise in case of problems (for example if the developer didn't write the precompile field correctly), to let the developer know if they've made any error (instead now we're failing silently here).
Even if we want to complete the installation either way, we should still at least report the error.

@@ +42,5 @@
> +
> +          toLoad--;
> +          if (toLoad == 0) {
> +            deferred.resolve();
> +          }

Is precompileScript always going to call the observer for each compilation, even if there are errors?
If it isn't, then the promise could end up being always unresolved.

@@ +51,5 @@
> +
> +      aManifest.precompile.forEach((aPath) => {
> +        let uri = Services.io.newURI(aPath, null, origin);
> +        debug("Script to compile: " + uri.spec);
> +        Services.scriptloader.precompileScript(uri, principal, observer);

Nit: IIRC, even if you keep using nsIObserver, I think the observer could just be a normal function.

Services.scriptLoader.precompileScript(uri, principal, (aSubject) => {
  ...
});

::: dom/apps/src/Webapps.jsm
@@ +1493,5 @@
>          delete app.retryingDownload;
>  
> +        // Update the asm.js scripts we need to compile.
> +        ScriptPreloader.preload(app, aData)
> +          .then(this.saveApps).then(() => {

this._saveApps

::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
@@ +59,5 @@
> +     *                  script as a nsIURI.
> +     */
> +    void precompileScript(in nsIURI uri,
> +                          in nsIPrincipal principal,
> +                          in nsIObserver observer);

Nit: I'm not sure, but I think you could just use a callback function instead of an observer here.
Attachment #8407649 - Flags: feedback+
(In reply to Marco Castelluccio [:marco] from comment #35)
> Comment on attachment 8407649 [details] [diff] [review]
> preload-asm-js.patch v5
> 
> Review of attachment 8407649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/ScriptPreloader.jsm
> @@ +6,5 @@
> > +
> > +const Cu = Components.utils;
> > +const Cc = Components.classes;
> > +const Ci = Components.interfaces;
> > +const Cr = Components.results;
> 
> Nit: You could use destructuring assignments for clarity here (you could
> also avoid defining some of the constants unused in this file).

Yep, I'll clean up the ununsed one. I kind of disagree about destructuring assignment being clearer ;)
 
> @@ +20,5 @@
> > +}
> > +
> > +this.ScriptPreloader = {
> > +
> > +  preload: function(aApp, aManifest) {
> 
> I think we should reject the promise in case of problems (for example if the
> developer didn't write the precompile field correctly), to let the developer
> know if they've made any error (instead now we're failing silently here).
> Even if we want to complete the installation either way, we should still at
> least report the error.

I'm fine with reporting an error, but that should not prevent app install, so I will not reject the promise.

> @@ +42,5 @@
> > +
> > +          toLoad--;
> > +          if (toLoad == 0) {
> > +            deferred.resolve();
> > +          }
> 
> Is precompileScript always going to call the observer for each compilation,
> even if there are errors?
> If it isn't, then the promise could end up being always unresolved.

Yes, the observer is always called from the c++ side.

> @@ +51,5 @@
> > +
> > +      aManifest.precompile.forEach((aPath) => {
> > +        let uri = Services.io.newURI(aPath, null, origin);
> > +        debug("Script to compile: " + uri.spec);
> > +        Services.scriptloader.precompileScript(uri, principal, observer);
> 
> Nit: IIRC, even if you keep using nsIObserver, I think the observer could
> just be a normal function.
> 
> Services.scriptLoader.precompileScript(uri, principal, (aSubject) => {
>   ...
> });

Yes, that will work. I have no strong opinion there.


> ::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
> @@ +59,5 @@
> > +     *                  script as a nsIURI.
> > +     */
> > +    void precompileScript(in nsIURI uri,
> > +                          in nsIPrincipal principal,
> > +                          in nsIObserver observer);
> 
> Nit: I'm not sure, but I think you could just use a callback function
> instead of an observer here.

You can't define function parameters in xpidl unfortunately, you have to wrap them in an interface, which is just what nsIObserver does. The observe() signature is a bit overkill (we don't use data) but I think it's still better than creating a new interface for this use case.
Some changes only to the js side.

try build at https://tbpl.mozilla.org/?tree=Try&rev=eeb1bcb7472b
Attachment #8407220 - Attachment is obsolete: true
Attachment #8407649 - Attachment is obsolete: true
Attachment #8407220 - Flags: review?(mar.castelluccio)
Attachment #8407649 - Flags: review?(ferjmoreno)
Attachment #8407794 - Flags: review?(mar.castelluccio)
updated after discussing on irc with bholley:

https://tbpl.mozilla.org/?tree=Try&rev=59261ff96ea9
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6

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

(In reply to Fabrice Desré [:fabrice] from comment #36) 
> > @@ +20,5 @@
> > > +}
> > > +
> > > +this.ScriptPreloader = {
> > > +
> > > +  preload: function(aApp, aManifest) {
> > 
> > I think we should reject the promise in case of problems (for example if the
> > developer didn't write the precompile field correctly), to let the developer
> > know if they've made any error (instead now we're failing silently here).
> > Even if we want to complete the installation either way, we should still at
> > least report the error.
> 
> I'm fine with reporting an error, but that should not prevent app install,
> so I will not reject the promise.

Fine by me.

> > @@ +42,5 @@
> > > +
> > > +          toLoad--;
> > > +          if (toLoad == 0) {
> > > +            deferred.resolve();
> > > +          }
> > 
> > Is precompileScript always going to call the observer for each compilation,
> > even if there are errors?
> > If it isn't, then the promise could end up being always unresolved.
> 
> Yes, the observer is always called from the c++ side.

Good, if it is called even if the compilation fails then we're good.

> > ::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
> > @@ +59,5 @@
> > > +     *                  script as a nsIURI.
> > > +     */
> > > +    void precompileScript(in nsIURI uri,
> > > +                          in nsIPrincipal principal,
> > > +                          in nsIObserver observer);
> > 
> > Nit: I'm not sure, but I think you could just use a callback function
> > instead of an observer here.
> 
> You can't define function parameters in xpidl unfortunately, you have to
> wrap them in an interface, which is just what nsIObserver does. The
> observe() signature is a bit overkill (we don't use data) but I think it's
> still better than creating a new interface for this use case.

Well, we're actually not using any of the parameters (just the subject, but
only to print a debug message, that we could print anyway).
Anyway, it's just a matter of taste, I prefer a callback function in cases like
this because it feels more "logical" (you're observing just one notification,
and it's just the end of an asynchronous operation) and "coherent" (because by
defining an interface you precisely define what the arguments of the function
are).
It's OK either way.

::: dom/apps/src/ScriptPreloader.jsm
@@ +50,5 @@
> +    } else {
> +      // The precompile field is not an array, let the developer know.
> +      // We don't want to have to enable debug for that to show.
> +      if (aManifest.precompile) {
> +        dump("ASM.JS compilation failed: the 'precompile' manifest " +

Cu.reportError would be better, I think web developers usually don't see what Firefox prints in the console.

::: dom/apps/src/Webapps.jsm
@@ +1493,5 @@
>          delete app.retryingDownload;
>  
> +        // Update the asm.js scripts we need to compile.
> +        ScriptPreloader.preload(app, aData)
> +          .then(this._saveApps).then(() => {

The test is failing because the function is not bound here, you could do: () => this._saveApps()
Attachment #8407794 - Flags: review?(mar.castelluccio) → review+
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6

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

::: dom/apps/src/ScriptPreloader.jsm
@@ +22,5 @@
> +  preload: function(aApp, aManifest) {
> +    debug("Preloading " + aApp.origin);
> +    let deferred = Promise.defer();
> +
> +#ifdef MOZ_B2G

For testing purposes, it could be useful to make this a pref or
something configurable (otherwise we'll be able to test only on b2g).

For example, to avoid yet another pref, you could define a |_isEnabled|
property (|true| by default if MOZ_B2G is defined, |false| otherwise)
that we can override during tests.
Filed bug 997886 for tests.
Hopefully last try build before landing:
https://tbpl.mozilla.org/?tree=Try&rev=f208fa849ca1
Last thing, |precompileScript| could throw (for example if the js file doesn't exist), you should wrap the call in a try block and resolve the promise in case of exceptions.
\o/
This patch has landed in in-bound and requesting a nom for 1.4

Per results from QA will decide if safe for 1.4 or move to backlog.

Tony,

Please help verify the same.
blocking-b2g: backlog → 1.4?
Flags: needinfo?(tchung)
Keywords: verifyme
(In reply to Preeti Raghunath(:Preeti) from comment #46)
> This patch has landed in in-bound and requesting a nom for 1.4
> 
> Per results from QA will decide if safe for 1.4 or move to backlog.
> 
> Tony,
> 
> Please help verify the same.

This is not a blocker - the only option we can consider this for is an approval request.

Please have an approval form filled out & we'll consider this for approval.
blocking-b2g: 1.4? → backlog
QA Contact: jsmith
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Luke,

Please request aurora-approval on the same. Form attached on top.
blocking-b2g: backlog → 1.4?
Flags: needinfo?(luke)
QA Contact: jsmith
blocking-b2g: 1.4? → backlog
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6

[Approval Request Comment]
User impact if declined: asm.js games will sometimes (esp. on first run) take a really long time to load (like 1 minute)
Testing completed (on m-c, etc.): b2g-inbound
Risk to taking this patch (and alternatives if risky): very low: this whole path is predicated on a new manifest option that isn't set by any existing apps
String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader
Attachment #8407794 - Flags: approval-mozilla-aurora?
Flags: needinfo?(luke)
Flags got overwritten. Moving to backlog so approval can be requested.
blocking-b2g: backlog → -
blocking-b2g: - → backlog
(In reply to Luke Wagner [:luke] from comment #49)
> Comment on attachment 8407794 [details] [diff] [review]
> preload-asm-js.patch v6
> 
> [Approval Request Comment]
> User impact if declined: asm.js games will sometimes (esp. on first run)
> take a really long time to load (like 1 minute)
> Testing completed (on m-c, etc.): b2g-inbound
> Risk to taking this patch (and alternatives if risky): very low: this whole
> path is predicated on a new manifest option that isn't set by any existing
> apps
> String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader

To amplify Luke's notes -- a high profile partner we've been cultivating won't submit their game to marketplace without this patch (nor should they), see Martin's comment 18 above.
(In reply to Preeti Raghunath(:Preeti) from comment #46)
> This patch has landed in in-bound and requesting a nom for 1.4
> 
> Per results from QA will decide if safe for 1.4 or move to backlog.
> 
> Tony,
> 
> Please help verify the same.

(In reply to Luke Wagner [:luke] from comment #49)
> Comment on attachment 8407794 [details] [diff] [review]
> preload-asm-js.patch v6
> 
> [Approval Request Comment]
> User impact if declined: asm.js games will sometimes (esp. on first run)
> take a really long time to load (like 1 minute)
> Testing completed (on m-c, etc.): b2g-inbound
> Risk to taking this patch (and alternatives if risky): very low: this whole
> path is predicated on a new manifest option that isn't set by any existing
> apps
> String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader

Looking for a little more Dev guidance here to help with inbound testing.   Some questions i have:
- regression risks to the rest of the platform when this lands?   (specifically, affected APIs around app installation, removal, etc.., other packaged apps that are already on the device)
- is WMW the only affected app that we know of for this work on 1.4?  or are there other apps that need to be considered for 1.4?
- if QA needs to test WMW from marketplace, is there a sandboxed environment that we can pull this from?   (we can certainly test sideloading, but the correct way is to go through the full marketplace userflow)
- Performance risks:  do you expect this to touch any number of cold/warm launch hits on the app and other core apps?
- if something goes awry, can we cleanly back this patch out in isolation?
Flags: needinfo?(tchung)
Luke,

Can you please help Tony here? Looks like risk analysis is being asked here.
Flags: needinfo?(luke)
I'm not Luke, but I htink I can answer!

(In reply to Tony Chung [:tchung] from comment #52)

> (In reply to Luke Wagner [:luke] from comment #49)
> > Comment on attachment 8407794 [details] [diff] [review]
> > preload-asm-js.patch v6
> > 
> > [Approval Request Comment]
> > User impact if declined: asm.js games will sometimes (esp. on first run)
> > take a really long time to load (like 1 minute)
> > Testing completed (on m-c, etc.): b2g-inbound
> > Risk to taking this patch (and alternatives if risky): very low: this whole
> > path is predicated on a new manifest option that isn't set by any existing
> > apps
> > String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader
> 
> Looking for a little more Dev guidance here to help with inbound testing.  
> Some questions i have:
> - regression risks to the rest of the platform when this lands?  
> (specifically, affected APIs around app installation, removal, etc.., other
> packaged apps that are already on the device)

Very low, and covered by existing install/update tests. We also have new tests coming up in bug 997886.

> - is WMW the only affected app that we know of for this work on 1.4?  or are
> there other apps that need to be considered for 1.4?

It's partner dependant, but could be used by other apps for sure.

> - if QA needs to test WMW from marketplace, is there a sandboxed environment
> that we can pull this from?   (we can certainly test sideloading, but the
> correct way is to go through the full marketplace userflow)

You don't need to test with wmw specifically. Any app the include asm.js code will be good enough. I'll provide one to QA that we can share.

> - Performance risks:  do you expect this to touch any number of cold/warm
> launch hits on the app and other core apps?

No.

> - if something goes awry, can we cleanly back this patch out in isolation?

Yes.
(I thought most of these questions should be going to you anyway :)
Flags: needinfo?(luke)
sounds good.  Fabrice, can you attach that asm.js test app to this bug?  then anyone watching this can help out too.

when this patch a-plusses, qa will organize some folks to test the scenarios out on 1.4.  Jason is already working on a testplan so we can hammer through different usecases.
Flags: needinfo?(fabrice)
(In reply to Tony Chung [:tchung] from comment #56)
> sounds good.  Fabrice, can you attach that asm.js test app to this bug? 
> then anyone watching this can help out too.

I think Fabrice mentioned Alon had some sample asm.js test apps I could use.

> 
> when this patch a-plusses, qa will organize some folks to test the scenarios
> out on 1.4.  Jason is already working on a testplan so we can hammer through
> different usecases.

Actually we should test this before uplifting. That means we should test this on trunk first.
https://hg.mozilla.org/mozilla-central/rev/51fd619edd5a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
QA Contact: jsmith
Flags: needinfo?(mbest)
(In reply to Jason Smith [:jsmith] from comment #59)
> Test plan is written up here:
> 
> https://docs.google.com/a/mozilla.com/spreadsheets/d/1qFct4EDx5mCt3G0n-
> KHxpeZztxwsNmD7Z5dmAFUs1mY/edit#gid=1588926924
> 
> I'm currently waiting on getting a reference app.

Nick (cc'ed) has volunteered to help get you a reference app
Flags: needinfo?(nick)
Keywords: dev-doc-needed
Blocks: 1000577
works when installing using `navigator.mozApps.installPackage`.  Boot time cut down to 20s from 50s!  Doesn't work when side loading from App Manager.
Flags: needinfo?(nick)
Flags: needinfo?(fabrice)
Triage update : waiting on QA testing to be completed before uplifting on 1.4
Update on testing:

* Nick and I worked to get WMW working with the precompile manifest flag. We verified that it does work with this patch.
* I also developed two custom packaged apps making use of the precompile flag with some of Enscripten benchmarks & verified that precompilation worked correctly on install.
* I worked on trying to get a customization working with an app using the precompile manifest flag, but got blocked by bug 1001209 in the process

Next Steps

* Run a couple more tests with install flows for error cases & variant file paths
* Run a couple tests doing app updates for precompliation of JS files

I'll plan on finishing this up tomorrow.
Sorry for taking too long to finish this. App update tests look fine - I'm seeing the right files getting compiled on an update. I still haven't been able to test preinstalled apps with asm.js due to being blocked on bug 1001209, but I don't think that needs to block an uplift. I'll just test it post uplift.
Status: RESOLVED → VERIFIED
Keywords: verifyme
blocking-b2g: backlog → 1.4?
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6

Taking asm.js in 1.4
Attachment #8407794 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
blocking-b2g: 1.4? → backlog
FYI - Just verified that preinstalling apps with a precompile flag also works as expected.
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6

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

For 1.4 marking b2g30 as approved.
Attachment #8407794 - Flags: approval-mozilla-b2g30+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #68)
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1a35ec54b068
> 
> Should we uplift bug 997886 as well?

That would be a nice touch, but I won't fight for that.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #69)
> Unfortunately, my attempts to rebase this didn't go so well. Backed out for
> bustage.
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4e9928bfccff
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38882336&tree=Mozilla-B2g30-
> v1.4

ha... I need to clone a 1.4 tree to help :(
Flags: needinfo?(fabrice)
Fabrice backed this out for m-oth failures on all platforms.
Nevermind me, I mixed up the changesets. This is still landed.
Whiteboard: [games] → [games][qa-]
blocking-b2g: backlog → ---
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.