997848 - Add support to compile asm.js code at install/update time for desktop apps

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Marco Castelluccio [:marco]]

Once bug 965970 is fixed, we should extend the mechanism to support desktop apps too.

We'll need to modify the asm.js cache implementation to store data in a different profile.

Comment 1

[Marco Castelluccio [:marco]]

Luke, any pointers / suggestions?

On desktop (and Android), we have multiple profiles (each app has its own profile).

Comment 2

[Luke Wagner [:luke]]

asm.js cache entries are stored in the 'storage/' subdir of gecko's profile dir; the same 'storage/' dir that contains all the IndexedDB storage (and later localstorage, etc).  I assume when a gecko process is started for an installed app, that gecko process only opens one profile dir and that profile dir is the app's profile dir.  Is that right?  If so, then I don't so far see the problem; the asm.js cache entries would go in the app's profile dir.

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Luke Wagner [:luke] from comment #2)
> asm.js cache entries are stored in the 'storage/' subdir of gecko's profile
> dir; the same 'storage/' dir that contains all the IndexedDB storage (and
> later localstorage, etc).  I assume when a gecko process is started for an
> installed app, that gecko process only opens one profile dir and that
> profile dir is the app's profile dir.  Is that right?  If so, then I don't
> so far see the problem; the asm.js cache entries would go in the app's
> profile dir.

Apps are installed by Firefox, so if we compile asm.js modules during the installation, we end up with cache entries stored in the Firefox profile.
We need instead to store the cache entries in a different profile than the profile being used while the installation happens.

Comment 4

[[:fabrice] Fabrice Desré]

Marco, how do you manage permissions? Isn't it the same kind of issue?

Comment 5

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Marco, how do you manage permissions? Isn't it the same kind of issue?

Setting permissions doesn't really take so much time, so we just set them during the first startup.

For AppCache, the implementation was modified to use a custom profile directory (bug 753990).

Comment 6

[Marco Castelluccio [:marco]]

An idea that I would like to explore is launching the app in a "preload mode", that executes the initialization steps and than terminates. I don't know yet if it's actually feasible.

Comment 7

[Luke Wagner [:luke]]

Yes, a 'preload mode' would make a lot of sense.  From a platform perspective, this wouldn't require any additional functionality, which I like :)

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Early WIP

Myk, I'd like your feedback on this idea.

The idea is to introduce a preloading mechanism in the webapp runtime.

- In preload mode, the webapp runtime doesn't show any UI, it only takes care of some initial things that need to happen during the first startup (things that are far easier to do if we're in the webapp runtime environment than in Firefox itself, like asm.js preloading)
- The installation/update will take care of executing the app in preload mode and wait for the preloading to finish before calling the installation/update finished.
- I'll start with asm.js preloading, but the plan is to move other initialization steps out of the Startup module and into the Preload module (for example setting permissions, creating the branding files, etc.).

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8440202 [details] [diff] [review]
Early WIP

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

This seems like a fine way to implement asm.js precompilation!  We should just make sure it doesn't introduce contention for DOMApplicationRegistry files between the Firefox and app processes.

Comment 10

[Marco Castelluccio [:marco]]

I've a WIP patch (supporting only preloading at install time, I haven't decided yet
when is the best time to preload for updates), but I've run into a problem.

There's a failing assertion:

--*-- ScriptPreloader: Preloading app://test_desktop_packaged_launch/
--*-- ScriptPreloader: Script to compile: app://test_desktop_packaged_launch/asmjsmodule.js
[8058] ###!!! ASSERTION: Calling GetOrCreate() after shutdown!: 'Error', file dom/quota/QuotaManager.cpp, line 897
InitOnMainThread (dom/asmjscache/AsmJSCache.cpp:671)
nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:766)
NS_InvokeByIndex (xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:166)
XPCCallContext::operator JSContext*() const (js/xpconnect/src/xpcprivate.h:801)
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xpconnect/src/XPCWrappedNative.cpp:1703)
XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1272)
UNKNOWN 0x7f08f816ed80
[8058] WARNING: NS_ENSURE_TRUE(qm) failed: file dom/asmjscache/AsmJSCache.cpp, line 671
[8058] WARNING: unable to post SHUTDOWN message: file netwerk/protocol/http/nsHttpConnectionMgr.cpp, line 170
[8058] ###!!! ASSERTION: Calling GetOrCreate() after shutdown!: 'Error', file dom/quota/QuotaManager.cpp, line 897
InitOnMainThread (dom/asmjscache/AsmJSCache.cpp:671)
nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:766)
NS_InvokeByIndex (xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:166)
XPCCallContext::operator JSContext*() const (js/xpconnect/src/xpcprivate.h:801)
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (js/xpconnect/src/XPCWrappedNative.cpp:1703)
XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1272)
UNKNOWN 0x7f08f816ed80
[8058] WARNING: NS_ENSURE_TRUE(qm) failed: file dom/asmjscache/AsmJSCache.cpp, line 671
--*-- ScriptPreloader: Done compiling app://test_desktop_packaged_launch/asmjsmodule.js

This assertion doesn't fail if in preload mode I open a window before using
ScriptPreloader.

Comment 11

[Marco Castelluccio [:marco]]

Attachment: WIPv2

This is the WIP patch.

Comment 12

[Marco Castelluccio [:marco]]

Comment on attachment 8451270 [details] [diff] [review]
WIPv2

I've found a way to overcome the problem.

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 14

[Marco Castelluccio [:marco]]

Comment on attachment 8460697 [details] [diff] [review]
Patch

Tests are failing on try, so transforming the review request in a feedback request.

Comment 15

[Marco Castelluccio [:marco]]

Comment on attachment 8460697 [details] [diff] [review]
Patch

Actually, I only forgot to add the new module to the package-manifest.in file. With that done, try is green.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8460697 [details] [diff] [review]
Patch

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

There's a trivial conflict, but otherwise this applies just fine.  The tests fail, however, perhaps because ScriptPreloader.jsm is missing?

::: testing/profiles/webapps_mochitest.json
@@ +106,5 @@
>    {
>      "name": "test_desktop_packaged_launch",
>      "csp": "",
>      "origin": "app://test_desktop_packaged_launch",
> +    "manifestURL": "http://test_desktop_packaged_launch/sample.manifest",

This host isn't defined anywhere, so it isn't clear why this change is being made.  Can you elaborate?

::: toolkit/webapps/MacNativeApp.js
@@ +104,5 @@
> +      }
> +
> +      if (!isLaunchable) {
> +        throw new Error("APP_NOT_CORRECTLY_INSTALLED");
> +      }

Hmm, if the app takes longer than 30 seconds to become launchable, so we don't precompile its code, then it should still run when the user launches it.  Thus I'm leery of including such an arbitrary timeout after which we assume the installation failed and remove the app.  We might give up on preloading it, but I wouldn't give up on completing the installation process.

::: toolkit/webapps/tests/data/asmjs_app/asmjs.sjs
@@ +74,5 @@
> +  for (var i = 0; i < 5000; i++) {
> +    code += "function g" + i + "() { return " + i + "}\n";
> +  }
> +  code += "return g1 }\n\
> +var g1 = f();";

Nit: I'd format the code so it's more readable in this source file, even if that means it's less readable in the generated file.

::: toolkit/webapps/tests/test_asmjs_packaged_app.xul
@@ +91,5 @@
> +  let storageDir = OS.Path.join(testAppInfo.profileDir.path, "storage",
> +                                "temporary");
> +
> +  let size = yield dirSize(storageDir);
> +  ok(size > 300000, "There are some files in the storage directory");

I wonder if we could make this number be the size of the largest empty directory.

@@ +97,5 @@
> +  let exeFile = getFile(testAppInfo.exePath);
> +
> +  let appClosed = false;
> +
> +  testAppInfo.appProcess.init(exeFile)

Nit: trailing semicolon.

::: webapprt/CommandLineHandler.js
@@ +28,5 @@
> +
> +      Preload().then(() => {}, () => {}).then(() => {
> +        Services.startup.exitLastWindowClosingSurvivalArea();
> +        Services.startup.quit(Ci.nsIAppStartup.eAttemptQuit);
> +      });

Nit: I'd rather we make the "handle" method an async task and then yield to the Preload function instead of creating a promise chain.  Alternatively, move this code into a separate "preload" async task method.

::: webapprt/Preload.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +this.EXPORTED_SYMBOLS = ["Preload", "ScriptPreload"];

Why export ScriptPreload from this module?  It doesn't do much more than ScriptPreloader.preload itself does.  Is there a reason that the ScriptPreload functionality can't be built into ScriptPreloader itself?

ScriptPreloader isn't included in this patch, so I can't check for myself, but I'd much rather have each module export only a single symbol that is named after the module file, as that makes it much easier to keep track of the symbols being imported into any given context.

::: webapprt/moz.build
@@ +21,5 @@
>      'PaymentUIGlue.js',
>  ]
>  
>  EXTRA_JS_MODULES += [
> +    'Preload.jsm',

Presumably ScriptPreloader.jsm needs to be added here as well.

Also, nit: I would use the same form of the word "preload" for both modules, i.e. either call them Preload.jsm and ScriptPreload.jsm or Preloader.jsm and ScriptPreloader.jsm.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Per bug 1238576, we're going to stop exposing navigator.mozApps on desktop/Android (except where MOZ_B2G is set, such as B2G Desktop), so we won't fix this bug.