997717 - Errors during package download aren't handled correctly

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Marco Castelluccio [:marco]]

downloadPackage's promise is always resolved, so both the function that should be called when a package is correctly downloaded and the function that should be called in case of errors are called when there is an error.

It does something like:
> Task.spawn(function() {
>   ...
> })
> .then(aOnSuccess,
>       revertDownload)

And then its users:
> downloadPackage(...).then(function downloadSuccess() {
>   ...
> });

The downloadSuccess function is always called.

I think this regression was introduced in bug 910815.

P.S.: downloadPackage has also an aOnSuccess argument that is unused. We may remove it while we fix this bug.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

Refactored the download package so that it doesn't handle and consume the rejection handler from the promise.

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

Left some bad formatting in last patch - this fixes that.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: 0001-Move-rejection-handler-out-of-downloadPackage.patch

Apologies for the spam - uploaded the wrong patch last time

Comment 4

[Marco Castelluccio [:marco]]

Comment on attachment 8424777 [details] [diff] [review]
0001-Move-rejection-handler-out-of-downloadPackage.patch

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

::: dom/apps/src/Webapps.jsm
@@ +1405,5 @@
>          manifestURL: aManifestURL,
>          origin: app.origin,
>          installOrigin: app.installOrigin,
>          downloadSize: app.downloadSize
> +      }, isUpdate).catch(

I think this isn't going to solve the problem, because the code after dowloadPackage (that is the code that should be executed only when the download succeeds), is going to be executed anyway.

Here's a small example of what's happening:

let func1 = Task.async(function*() {
  console.log("func1");
  throw "ASD";
});

let func2 = Task.async(function*() {
  yield func1().catch(() => { console.log("Handle exception"); });

  console.log("func2 continues");
});

func2();

This is going to print:
func1
Handle exception
func2 continues

Instead you should do something like:

let func1 = Task.async(function*() {
  console.log("func1");
  throw "ASD";
});

let func2 = Task.async(function*() {
  try {
    yield func1();

    console.log("func2 continues");
  } catch (ex) {
    console.log("Handle exception");
  }
});

func2();

This is going to print:
func1
Handle exception

@@ +1406,5 @@
>          origin: app.origin,
>          installOrigin: app.installOrigin,
>          downloadSize: app.downloadSize
> +      }, isUpdate).catch(
> +        this._revertDownloadPackage.bind(this, id, oldApp, newApp, false)

oldApp and newApp are undefined here.

@@ +2703,4 @@
>      }.bind(this)).then(null, Cu.reportError);
>    },
>  
> +  downloadPackage: Task.async(function*(aId, aOldApp, aManifest, aNewApp, aIsUpdate) {

Nit: I think you could remove the aId parameter and directly get the ID from aOldApp.

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

I filed bug 1013279 (found this error while looking for other thing). That bug has also a proposed patch, which makes less changes. Feel free to grab anything usable from that :)

Try run (for that patch) is at https://tbpl.mozilla.org/?tree=Try&rev=26ba1cc142e9

Comment 7

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

Comment on attachment 8424777 [details] [diff] [review]
0001-Move-rejection-handler-out-of-downloadPackage.patch

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

Hmm, I originally thought this bug wouldn't occur, because promise chain traversal stops at the first rejection handler.

But Marco is right: when a promise is rejected, and its rejection handler doesn't throw (or return a promise that gets rejected), then promise chain traversal continues, which means that yielding such a promise in a task returns a value instead of throwing an exception.

So the call to downloadPackage that calls the "catch" method on the returned promise needs to instead do what Marco suggested, i.e. catch the exception thrown by downloadPackage using a try/catch block and then return after calling _revertDownloadPackage in the catch block to ensure the rest of startDownload doesn't execute.

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

The way I fixed that on the duped bug I filed was just ensure that when I wanted the promise to be rejected I did throw instead of just returning. That ensured that only the reject handler (if present) would be called on all the chain (and that it something up the chain wanted to do something with the rejection, it could). Also, the change was simpler :).

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

Have adjusted code in a bit more of a thorough way that suggested by Antonio because as it stands the code is confusing and not in line with the rest of the codebase.  By moving the rejection handler out of the downloadPackage function, aside from fixing the issue in particular, we improve readability and maintain consistency with other asyn functions in the codebase.

Comment 10

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

Comment on attachment 8434079 [details] [diff] [review]
Move rejection handler out of downloadPackage

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

::: dom/apps/src/Webapps.jsm
@@ +1410,5 @@
>          manifestURL: aManifestURL,
>          origin: app.origin,
>          installOrigin: app.installOrigin,
>          downloadSize: app.downloadSize
> +      };

Nit: use two-space indent for this object definition, i.e.:

    let newApp = {
      manifestURL: aManifestURL,
      origin: app.origin,
      installOrigin: app.installOrigin,
      downloadSize: app.downloadSize
    };

@@ +1415,4 @@
>  
> +    try {
> +      let [aId, aManifest] = yield this.downloadPackage(id, app, manifest,
> +                                                        newApp, isUpdate);

Restrict this try/catch block to the code that may fail, i.e.:

    let aId, aManifest;
    try {
      [aId, aManifest] = yield this.downloadPackage(id, app, manifest, newApp, 
                                                    isUpdate);
    } catch (ex) {
    …

@@ +1444,5 @@
> +        // We restarted a failed download, apply it automatically.
> +        this.applyDownload(aManifestURL);
> +      }
> +    } catch (ex) {
> +        debug("Error caught " + ex);

Nit: here and below, log a descriptive error message, like |"Error downloading package: " + ex|.

@@ +1445,5 @@
> +        this.applyDownload(aManifestURL);
> +      }
> +    } catch (ex) {
> +        debug("Error caught " + ex);
> +        this._revertDownloadPackage.bind(this, id, app, newApp, isUpdate, ex);

This just returns the bound function without calling it.  It doesn't need to bind the function; it should simply call it.

Also, the reason we were passing the error to _revertDownloadPackage is that we were chaining it to a promise as the promise's reject handler.  Now that we aren't doing that, we don't need to pass the error, as we can simply log the error here (which is exactly what you're doing!).

@@ +2265,4 @@
>    queuedDownload: {},
>    queuedPackageDownload: {},
>  
> +  onInstallSuccessAck: Task.async(function*(aManifestURL, aDontNeedNetwork) {

If you're going to make this a asynchronous function, then you'll need to audit its callers to ensure they continue to behave the same.  At first glance, it looks like at least confirmInstall needs to yield its call to this function in order for its behavior to stay the same.  I'm not sure if any others need to change, however.

@@ +2291,4 @@
>      }
>  
>      let packageDownload = this.queuedPackageDownload[aManifestURL];
> +

Nit: revert this extraneous change.

@@ +2299,4 @@
>  
>        delete this.queuedPackageDownload[aManifestURL];
>  
> +      // We define these outside the task to use them in its reject handler.

Remove this comment, which no longer makes sense.

@@ +2785,2 @@
>  
> +    return [aOldApp.id, newManifest];

Now that the callers pass in the old app, it's no longer necessary to return its ID, so this should simply return the new manifest.

@@ +3442,4 @@
>    },
>  
>    // Removes the directory we created, and sends an error to the DOM side.
> +  _revertDownloadPackage: Task.async(function*(aId, aOldApp, aNewApp, aIsUpdate, aError) {

Nit: now that the callers of downloadPackage are the ones who call _revertDownloadPackage, this function should be public too, so rename it to revertDownloadPackage.

Also, this doesn't need to be a task, as it doesn't do anything asynchronously.

Comment 11

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

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

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> @@ +2785,2 @@
> >  
> > +    return [aOldApp.id, newManifest];
> 
> Now that the callers pass in the old app, it's no longer necessary to return
> its ID, so this should simply return the new manifest.

The app ID may change in |_checkOrigin|: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm?rev=48eee276b1ee#3367

Comment 13

[Marco Castelluccio [:marco]]

Martyn, I was wondering why you propagate the exception from onInstallSuccessAck up to confirmInstall.

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

Nice catch Marco; I'd inadvertently changed the behaviour in onInstallSuccessAck to rethrow the error, we now deal with the error and return.

Also, I've tweaked the code to deal with the case when the app id changes.

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

Comment 15

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

Comment on attachment 8442491 [details] [diff] [review]
Move rejection handler out of downloadPackage

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

Other than the one issue, this seems reasonable.  But I'm concerned about the case where the ID changes, and I'm just generally concerned that we're touching critical code paths here, so I'd like a second review from Marco.

::: dom/apps/src/Webapps.jsm
@@ +1444,1 @@
>                                 "manifest.webapp");

Hrm, I don't quite understand how this works, since *id* is the old ID here, which may have changed.  But downloadPackage already downloads the ZIP to a subdirectory based on the old ID, and that appears to work correctly too.  But perhaps that actually doesn't work when the ID changes, and it's an existing bug that we're exacerbating with this change.

@@ +2286,5 @@
>        let onlineWrapper = {
>          observe: function(aSubject, aTopic, aData) {
>            Services.obs.removeObserver(onlineWrapper,
>                                        "network:offline-status-changed");
> +          yield DOMApplicationRegistry.onInstallSuccessAck(aManifestURL);

Yielding here breaks this codepath, because it means the "observe" function is going to return a generator rather than execute when it's called.  In order to make it execute, you would have to do:

  observe: Task.async(function*(aSubject, aTopic, aData) {
    …
  })

But it doesn't make a difference in this case whether or not the caller (nsIObserverService) waits for DOMApplicationRegistry.onInstallSuccessAck, so you could instead simply not yield to it.

Comment 16

[Marco Castelluccio [:marco]]

Comment on attachment 8442491 [details] [diff] [review]
Move rejection handler out of downloadPackage

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> ::: dom/apps/src/Webapps.jsm
> @@ +1444,1 @@
> >                                 "manifest.webapp");
> 
> Hrm, I don't quite understand how this works, since *id* is the old ID here,
> which may have changed.  But downloadPackage already downloads the ZIP to a
> subdirectory based on the old ID, and that appears to work correctly too. 
> But perhaps that actually doesn't work when the ID changes, and it's an
> existing bug that we're exacerbating with this change.

This code is quite confusing :D
Without the patch, we're using |aID|, that is the new ID returned by
|downloadPackage| (|downloadPackage| returns |oldApp.id|, that is the new ID).

In _checkOrigin, when we modify the app ID, we also move stuff from the
subdirectory named after the old ID to a subdirectory named after the new ID
(see http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm?rev=366b5c0c02d3#3369
and the following lines, in particular "Rename the directories where the files
are installed.").

So I think the patch (in particular the changes to |startDownload|) are still
breaking this case.

The ID change code was added in bug 852720. It may be useful to ask for a manual
test case or even better write an automated test case to ensure we don't regress
this (since it's easy to regress).

::: dom/apps/src/Webapps.jsm
@@ +2568,4 @@
>        // Skip directly to onInstallSuccessAck, since there isn't
>        // a WebappsRegistry to receive Webapps:Install:Return:OK and respond
>        // Webapps:Install:Return:Ack when an app is being auto-installed.
> +      yield this.onInstallSuccessAck(app.manifestURL);

If you yield on the promise returned by onInstallSuccessAck, the confirmInstall promise will be resolved after the call to installSuccessCallback, before the patch it was resolved before the call.

I don't know if the code using confirmInstall is making any assumption about this behavior, it's just something to double-check.

Comment 17

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

Reverted changes to the values we were returning from downloadPackage; it could do with being update to remove some spaghetti from the code, but this should be addressed in another bug.

Also removed the yield on the promise returned by onInstallSuccessAck as mentioned by Marco; although it didn't seem to make any difference I'd rather not change code if not needed.

Comment 18

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

Comment on attachment 8453810 [details] [diff] [review]
Move rejection handler out of downloadPackage

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

startDownload still has a reference to aId, which causes tests to fail:

07-10 15:01 > ./mach build && ./mach mochitest dom/apps/tests
…
 0:10.76 A coding exception was thrown and uncaught in a Task.
 0:10.76 
 0:10.76 Full message: ReferenceError: aId is not defined
 0:10.76 Full stack: this.DOMApplicationRegistry.startDownload<@resource://gre/modules/Webapps.jsm:1318:5

::: dom/apps/src/Webapps.jsm
@@ +1454,2 @@
>  
>      app = this.webapps[aId];

aId -> newId

@@ +3485,1 @@
>      debug("Cleanup: " + aError + "\n" + aError.stack);

Both revertDownloadPackage callers now log the error, so this debug() statement is no longer necessary (alternately, you could remove those two statements in favor of this one; although in that case I would change this statement to "Error downloading package: " + aError).

Comment 19

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

Fixed aId bug and removed extraneous logging.

Comment 20

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

Comment on attachment 8454461 [details] [diff] [review]
Move rejection handler out of downloadPackage

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

::: dom/apps/src/Webapps.jsm
@@ +2810,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.

Nit: I would leave the comment explaining why we throw the error.

Comment 21

[Martyn Haigh (:mhaigh)]

Attachment: Move rejection handler out of downloadPackage

Updated patch to fix bitrot, added note back in as nitted by Myk

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/821754af04e3

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/821754af04e3