1013433 - uninstalling, resideloading, and relaunching app causes it to stall at white screen

Status: RESOLVED FIXED

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

Description

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

Uninstalling a running app and then resideloading and relaunching it causes it to stall at the white screen of sorrow.

Steps to reproduce are something like:

1. sideload an app (I used a test packaged app, but unsure whether it needs to be packaged);
2. launch the app;
3. uninstall the app;
4. sideload the app;
5. launch the app.

After step 2, the app ran as expected.  But after step 5, it didn't.  It just displayed the white screen.

Strangely, step 4 didn't prompt me to pick a runtime, even though I hadn't set a default.  It's almost as if the app's Fennec process was still running, even though the app had been uninstalled, and step 5 reused the existing process instead of starting a new one.  But killing both Nightly and the app and then relaunching the app doesn't solve the problem.

There's nothing obvious in the logs, except that the launch process gets farther than it does for bug 991394, so the problem isn't that bug.

Comment 1

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

Attachment: patch v1: complete the packaged app update process

This patch fixes the problem that WebappManager doesn't complete the packaged app update process, as it calls DOMApplicationRegistry.updatePackagedApp but doesn't subsequently call DOMApplicationRegistry.startDownload and DOMApplicationRegistry.applyDownload, all three of which need to be called to complete the process.


mfinkle: can you review the WebappManager.jsm changes?

Mostly they just add those two additional calls, but they also refactor some common code between the install and update paths into a _postInstall function.


Marco: can you review the Webapps.jsm changes?  They are:

1. make startDownload throw exceptions on error conditions so WebappManager._autoUpdatePackagedApp will abort if there's an error starting the download;

2. make applyDownload be a Task so WebappManager._autoUpdatePackagedApp can yield until it returns;

3. make downloadPackage and _revertDownloadPackage throw on an error (which is the minimal fix for bug 997717, so mhaigh and/or I, depending on who lands first, will have to reconcile his more comprehensive fix for that bug with these changes).


Note that startDownload and applyDownload are otherwise called by callers who don't care that they're asynchronous operations (i.e. they don't get chained to other promises, nor are their promises yielded).  So these changes shouldn't affect those callers.  But tryserver will tell:

  https://tbpl.mozilla.org/?tree=Try&rev=af2a13225f02


This should also fix bug 1013428.

Comment 2

[Marco Castelluccio [:marco]]

Comment on attachment 8434569 [details] [diff] [review]
patch v1: complete the packaged app update process

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #1)
> Marco: can you review the Webapps.jsm changes?  They are:
> 
> 1. make startDownload throw exceptions on error conditions so
> WebappManager._autoUpdatePackagedApp will abort if there's an error starting
> the download;
> 
> 2. make applyDownload be a Task so WebappManager._autoUpdatePackagedApp can
> yield until it returns;

This part looks OK to me.

> 3. make downloadPackage and _revertDownloadPackage throw on an error (which
> is the minimal fix for bug 997717, so mhaigh and/or I, depending on who
> lands first, will have to reconcile his more comprehensive fix for that bug
> with these changes).

I don't particularly like this simpler approach where we re-throw the
error in the function that is supposed to catch the error. But I guess it's ok
to do it as a temporary workaround before bug 997717 is properly fixed.

::: dom/apps/src/Webapps.jsm
@@ +2763,5 @@
>        if (oldPackage) {
>          debug("package's etag or hash unchanged; sending 'applied' event");
>          // The package's Etag or hash has not changed.
> +        // We send an "applied" event right away so code awaiting that event
> +        // can proceed to access the app.  We also throw an error to alert

You shouldn't need this, because _sendAppliedEvent sets readyToApplyDownload to false,
making a subsequent call to |applyDownload| basically a no-op.

::: mobile/android/modules/WebappManager.jsm
@@ +307,5 @@
> +    yield DOMApplicationRegistry.updatePackagedApp(aData, aOldApp.id, aOldApp, aData.updateManifest);
> +
> +    try {
> +      yield DOMApplicationRegistry.startDownload(aData.manifestURL);
> +    } catch (ex if ex.message == "PACKAGE_UNCHANGED") {

So you wouldn't need to catch this error and you could just call
applyDownload, knowing it won't do anything (because if the package
is unchanged, the app isn't "readyToApplyDownload").

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8434569 [details] [diff] [review]
patch v1: complete the packaged app update process

LGTM

Comment 4

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

(In reply to Marco Castelluccio [:marco] from comment #2)

> > +        // We send an "applied" event right away so code awaiting that event
> > +        // can proceed to access the app.  We also throw an error to alert
> 
> You shouldn't need this, because _sendAppliedEvent sets readyToApplyDownload
> to false,
> making a subsequent call to |applyDownload| basically a no-op.
…
> So you wouldn't need to catch this error and you could just call
> applyDownload, knowing it won't do anything (because if the package
> is unchanged, the app isn't "readyToApplyDownload").

Indeed!  The problem is just that startDownload destructures the return value of its downloadPackage call:

    let [aId, aManifest] = yield this.downloadPackage(manifest, {
        manifestURL: aManifestURL,
        origin: app.origin,
        installOrigin: app.installOrigin,
        downloadSize: app.downloadSize
      }, isUpdate);

So when downloadPackage returns early, without defining a return value, when the package is unchanged, then this destructuring triggers an exception (don't remember exactly what now; something about destructuring an undefined value) that prevents _autoUpdatePackagedApp from continuing to applyDownload.


There are other ways to fix the problem.  And bug 997717 will change the return value of downloadPackage, so it only returns the new manifest, which means we won't destructure it and trigger an exception if it's undefined.

But we'll still need to handle the early return, since the rest of startDownload shouldn't get executed if the package is unchanged.  Currently, it doesn't get executed accidentally (because of the destructuring bug).  This patch just makes it not get executed intentionally (because downloadPackage throws PACKAGE_UNCHANGED).

And we'll also need to handle the early return in _autoUpdatePackagedApp, which should itself return early to _autoUpdate (instead of propagating the exception) to complete the update process.

Comment 5

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

https://hg.mozilla.org/integration/fx-team/rev/4f945ab9cc3c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f945ab9cc3c