830294 - Can't launch an app after canceling its update

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
* install a big enough app (around 8MB is fine) whose you can trigger updates
* trigger an update of this app
* start the update this app
* cancel the update
* try to launch the app

Expected:
* the app is launching

Actual:
* nothing happens

There is no meaningful logcat except we seems to get two errors from the platform:

I/Gecko   ( 1155): -*-*- Webapps.jsm : Cleanup: NETWORK_ERROR
E/GeckoConsole( 1155): Content JS INFO at app://system.gaiamobile.org/js/updatable.js:101 in anonymous: downloadError event, error code is NETWORK_ERROR
E/GeckoConsole( 1155): Content JS INFO at app://system.gaiamobile.org/js/updatable.js:101 in anonymous: downloadError event, error code is DOWNLOAD_CANCELED

This could be another bug though.

Comment 1

[Jason Smith [:jsmith]]

Might be a regression. I thought this was working before...

Comment 2

[Julien Wajsberg [:julienw]]

I believe this is a DOM::Apps bug.

The installState is reset to 'pending' when we get a downloaderror. This is obviously wrong because we basically have 2 cases of downloaderror:

- the app is not installed yet, installState === 'pending' => no need to change it, it's still 'pending'
- the app is installed, then installState === 'updating' => we should get it back to 'installed' imho.

The installState is correctly set to 'updating' during the update.

I'm continuing to investigate.

Comment 3

[Julien Wajsberg [:julienw]]

I think I found the culprit. The error comes directly from the fact that we have two errors: the first one remove the saved value in AppInstallManager, so the second one can't find one, and use "pending" as a default.

We have a line to prevent sending 2 errors in the install case, but it doesn't work in the update case because it checks for downloadAvailable.

A patch is coming soon.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: patch v1

please tell me if you want that I remove the debug statements.

also I may have not understood what the checks that I removed were for.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 702363 [details] [diff] [review]
patch v1

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

::: dom/apps/src/Webapps.jsm
@@ +21,5 @@
>  Cu.import("resource://gre/modules/SystemMessagePermissionsChecker.jsm");
>  Cu.import("resource://gre/modules/AppDownloadManager.jsm");
>  
>  function debug(aMsg) {
> +  dump("-*-*- Webapps.jsm : " + aMsg + "\n");

re-comment.

@@ +936,5 @@
>  
>      // First of all, we check if the download is supposed to update an
>      // already installed application.
>      let isUpdate = (app.installState == "installed");
> +    debug('startDownload - isUpdate = ' + isUpdate);

remove

@@ +1812,5 @@
>  
>        // We avoid notifying the error to the DOM side if the app download
>        // was cancelled via cancelDownload, which already sends its own
>        // notification.
> +      if (!app.downloading) {

Can you check if we can keep the !app.downloadAvailable here. I'm fine with removinf downloadSize since it comes from the manifest and may be incorrect.

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> 
> @@ +1812,5 @@
> >  
> >        // We avoid notifying the error to the DOM side if the app download
> >        // was cancelled via cancelDownload, which already sends its own
> >        // notification.
> > +      if (!app.downloading) {
> 
> Can you check if we can keep the !app.downloadAvailable here. I'm fine with
> removinf downloadSize since it comes from the manifest and may be incorrect.

That's actually the main problem: we're canceling an _update_ so we _do_ have a downloadAvailable property. If we keep this check here, then the bug comes back.

I removed downloadSize more instinctively: I think we'd have it also in case of updates, but I'm not sure of that. Still I think |app.downloading| is sufficient here. Am I missing something ?

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: patch v2

carry on fabrice's feedback+

who should I ask approval to ?

Comment 8

[[:fabrice] Fabrice Desré]

feedback is not r+. I want to make sure we do the right sure for all the download cancel cases.

Comment 9

[Julien Wajsberg [:julienw]]

yep I understand that :) just wanted to ask for approval in advance.

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Hi,

AFAIK, maybe I'm wrong, being a tef+ you don't have to ask for approval, just receive a r+ (unless this is different for the backend bugs ;))

Comment 11

[Julien Wajsberg [:julienw]]

Francisco> ok, that's not what I thought but ok ;)

Comment 12

[[:fabrice] Fabrice Desré]

Also, approval applies to patches once they are ready to land.

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: alternate patch

Since what we want is explicitly now if we're called by cancelDownload, this patch does just that instead of relying of some fragile heuristic.

Julien, does this fix the issue?

Comment 14

[Julien Wajsberg [:julienw]]

I'll try tomorrow (and I think it may work) but without trying I already think that this is not a right fix...

Let me explain.

The initial problem was that we were really getting 2 errors. So what we really want is _not_ sending 2 errors. Using |app.canceling| for that does not seem right.

Testing |app.downloading| makes it really clear that we already handled this for this download. Or that we won't send an error if we already sent a success.

Maybe testing the return value of AppInstallManager.get() is sufficient too...

Also I wonder if we could not put a cleanup outside of downloadPackage and use it in |cancelDownload|.

Anyway I'll try your patch tomorrow and see if it fixes this specific problem.

Comment 15

[Julien Wajsberg [:julienw]]

Except if you think that we might legitimately call |cleanup| with |app.downloading === false| but I couldn't find a place where we do that now.

The fragile heuristic was what was here at first, with the checks to downloadSize and downloadAvailable, that you're also removing in your patch.

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #14)

> The initial problem was that we were really getting 2 errors. So what we
> really want is _not_ sending 2 errors. Using |app.canceling| for that does
> not seem right.

why? One error event was coming from cancelDownload() directly, and the other one from onStopRequest triggered by channel.cancel. What's wrong by making sure that we don't send the second one when we know that the channel is canceled by us explicitly, instead of going in onStopRequest because we finished the download?

> Testing |app.downloading| makes it really clear that we already handled this
> for this download. Or that we won't send an error if we already sent a
> success.
> 
> Maybe testing the return value of AppInstallManager.get() is sufficient
> too...

This is in gaia, right? I don't see how this relates to the current patches.

> Also I wonder if we could not put a cleanup outside of downloadPackage and
> use it in |cancelDownload|.

You can't prevent onStopRequest from being called when the underlying channel is canceled. So I don't think this would work.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #16)
> (In reply to Julien Wajsberg [:julienw] from comment #14)
> 
> > The initial problem was that we were really getting 2 errors. So what we
> > really want is _not_ sending 2 errors. Using |app.canceling| for that does
> > not seem right.
> 
> why? One error event was coming from cancelDownload() directly, and the
> other one from onStopRequest triggered by channel.cancel. What's wrong by
> making sure that we don't send the second one when we know that the channel
> is canceled by us explicitly, instead of going in onStopRequest because we
> finished the download?

We don't even know if the error is sent synchronously or asynchronously when we call |cancel| on the channel. And even if it will stay like this in the future.

> > Testing |app.downloading| makes it really clear that we already handled this
> > for this download. Or that we won't send an error if we already sent a
> > success.


You didn't comment this.

> > 
> > Maybe testing the return value of AppInstallManager.get() is sufficient
> > too...
> 
> This is in gaia, right? I don't see how this relates to the current patches.


Sorry, I was talking about |AppDownloadManager|...

I'd say that if |AppDownloadManager.get(manifestURL)| is null, then something wrong happened.

> 
> > Also I wonder if we could not put a cleanup outside of downloadPackage and
> > use it in |cancelDownload|.
> 
> You can't prevent onStopRequest from being called when the underlying
> channel is canceled. So I don't think this would work.

That was not my goal. I'd like to reduce the code where we modify |app.downloading| and having only one function sending errors would make it easier to not send them twice.


But you still didn't say why my first patch does not work.

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 702512 [details] [diff] [review]
alternate patch

Anyway your patch does not fix the bug. So I guess the error is sent asynchronously when we cancel the download.

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: alternate patch v2

I still think that it's better to rely on an explicit field indicating that we are stopping the download for a specific reason (canceling the download) than using the app state. We have too many corner cases and opportunities to regress that.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 702923 [details] [diff] [review]
alternate patch v2

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

Quick review as I can't test the patch right now.

I agree that this feels like a safest way.

::: dom/apps/src/Webapps.jsm
@@ +903,2 @@
>      if (download.cacheUpdate) {
>        // Cancel hosted app download.

Shouldn't we also set app.isCanceling for hosted apps?

Comment 21

[[:fabrice] Fabrice Desré]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #20)

> Shouldn't we also set app.isCanceling for hosted apps?

No. We only need this flag for packaged apps to know in onStopRequest what causes the request to be stopped.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

You are right. Thanks.
r=me. Still couldn't  test the patch though.

Comment 23

[Julien Wajsberg [:julienw]]

I will be able to test tomorrow. I understand the no regression argument even if I disagree for this specific case. But as you wish I can understand.

Comment 25

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc7bbbc2eea

Comment 26

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/35f645b83289

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdc7bbbc2eea

Comment 28

[Julien Wajsberg [:julienw]]

Just tried with latest b2g18, the bug is not fixed, sorry.

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 702923 [details] [diff] [review]
alternate patch v2

And I still don't understand why this patch is safer. I fear a lot that isCanceling could stay set for some reason and makes thing worse.

Comment 30

[Julien Wajsberg [:julienw]]

I tried again with a full reseted phone, and I can't reproduce.

So I put back "resolved fixed".

Comment 31

[Jason Smith [:jsmith]]

Verified using Julien's STR.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.