961762 - A manifest.webapp file is downloaded and the web app is loading forever when trying to launch a web app

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Flaviu Cos]

Environment: 
Device: Google Nexus 7 (Android 4.4.2);
Build: Nightly 29.0a1 (2014-01-20);

Steps to reproduce:
1. Install any web app from the Firefox Marketplace;
2. Launch the web app.

Expected result:
The web app is successfully launched.

Actual result:
A manifest.webapp file is downloaded and the web app is loading forever.

Note:
Tested with the following apps: MessageMe, Wikipedia and SoundCloud.

Comment 2

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

This is another regression from bug 959420.  It affects apps on Android both with and without synthetic APKs enabled.

Comment 3

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

Attachment: 961762v1.diff

I'm not sure how this used to work, since the webapp registry initialized asynchronously even before bug 959420.  But since that bug landed, the Android runtime has been racing it, so it calls mozApps.getAll before the registry populates its list of apps.

Here's a fix that gives DOMApplicationRegistry an "inited" promise which gets resolved upon the completion of initialization, so a consumer like the Android runtime can import Webapps.jsm and then ensure the registry is initialized before it accesses any of its functionality.

This isn't ideal, since it puts the onus on the consumer to explicitly wait for initialization, which means there may be other broken consumers out there.  And it's troubling that consumers can race registry initialization (both because it exposes a footgun and because it suggests that bug 959420 degraded performance).

But this patch does unbust Android in a robust fashion, so I think it's worth taking until we come up with a better solution.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8363510 [details] [diff] [review]
961762v1.diff

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

Alex just fixed the same race with a slightly different solution in bug 961655. Can you reuse that?

Comment 5

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

(In reply to Fabrice Desré [:fabrice] from comment #4)

> Alex just fixed the same race with a slightly different solution in bug
> 961655. Can you reuse that?

Yes, although it looks more complex and brittle, given that there are several callers of notifyAppsRegistryReady besides the initialization code path, and it isn't obvious that the method should be the last thing to happen on that code path, whereas this patch is intended to make it obvious within the *init* function itself that you should not add anything to the end of the function unless you also delay resolution of the promise until afterward.

Comment 6

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

Also, that patch is currently rotted.  But if ochameau can update and land it today, then I'll respin this patch on top of it.  Otherwise, I'd prefer to take this one sooner than later and combine forces later, since the regression busts all apps on Android.

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)
> Created attachment 8363510 [details] [diff] [review]
> 961762v1.diff
> 
> I'm not sure how this used to work, since the webapp registry initialized
> asynchronously even before bug 959420.  But since that bug landed, the
> Android runtime has been racing it, so it calls mozApps.getAll before the
> registry populates its list of apps.
> 
> Here's a fix that gives DOMApplicationRegistry an "inited" promise which
> gets resolved upon the completion of initialization, so a consumer like the
> Android runtime can import Webapps.jsm and then ensure the registry is
> initialized before it accesses any of its functionality.
> 
> This isn't ideal, since it puts the onus on the consumer to explicitly wait
> for initialization, which means there may be other broken consumers out
> there.  And it's troubling that consumers can race registry initialization
> (both because it exposes a footgun and because it suggests that bug 959420
> degraded performance).
> 
> But this patch does unbust Android in a robust fashion, so I think it's
> worth taking until we come up with a better solution.

It's possible that it used to work out of luck (like in the b2g case, where using OS.File busts everything [bug 961866]). It's good that we uncovered these race conditions!

I think there are two options:
1) Bug 959420 actually caused a small performance regression (maybe because we're using Task.jsm in some functions called at startup?).
2) Bug 959420 modified the order of execution of the code, making the WebAppRT.js function execute earlier than before.

Overall, I think the removal of the main thread operation (exists) and the code simplification is worth the small performance regression, because in real world situations this could mean a performance improvement.

While waiting for bug 961655, you could use this stopgap solution: http://mxr.mozilla.org/mozilla-central/source/webapprt/Startup.jsm#84

Comment 8

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

Attachment: patch v2: reuses bug 961655's API

I just pushed bug 961655 to inbound, so here's an updated patch that uses the API it exposes.  The patch is smaller than it looks, since most of the changes are to indentation.  All it does is wrap getManifestFor's implementation in a promise chain, so it's a two-line change not counting the indentation changes:

   getManifestFor: function (aUrl, aCallback) {
+    DOMApplicationRegistry.registryReady.then(() => {
…
+    });
   },

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/93ff98fbbaa4

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93ff98fbbaa4