The user needs to cancel twice to dismiss the download notification

VERIFIED FIXED

Status

Firefox OS
Gaia::System
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: julienw, Assigned: etienne)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.0 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
STR:
* have both a system and a app update
* start the updates
* cancel the update from the notification

Expected:
* we whould have the "update available" notification

Actual:
* we still have the "currently downloading" notification

If you cancel once more, then we have the expected notification.

Both downloads are maybe canceled at the first cancellation though, but I didn't verified that. I didn't verified very thoroughly either if this happens when only a system update or an app update are happening, but I'd say it doesn't.

This is a quite serious bug imho.
blocking-b2g: tef? → tef+
regression from bug 804571?  Could you place which build are you on please, julienw?
blocking-b2g: tef+ → tef?
oops. undid the tef... replacing it back
Assignee: nobody → etienne
blocking-b2g: tef? → tef+
(Reporter)

Comment 3

6 years ago
I've seen this on a build from today.

I've verified that at the first cancel, the system update is canceled but not the app update. At the second cancel, the app update is at last canceled.

Updated

6 years ago
Blocks: 728081
(Assignee)

Comment 4

6 years ago
Created attachment 701827 [details] [diff] [review]
Patch proposal
Attachment #701827 - Flags: review?(felash)
(Reporter)

Updated

6 years ago
Flags: in-testsuite+
(Reporter)

Comment 5

6 years ago
Comment on attachment 701827 [details] [diff] [review]
Patch proposal

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

I'd love to see a |git format-patch -U8| output next time ;)

Sorry that doesn't work.

::: apps/system/js/update_manager.js
@@ +120,5 @@
>      CustomDialog.hide();
>  
> +    // We're emptying the array while iterating
> +    for (var i = this.downloadsQueue.length - 1 ;i >= 0 ;i--) {
> +      var updatable = this.downloadsQueue[i];

thinking of this a little bit more, I'm not sure this is the right fix. Actually I'm quite sure this is wrong :-)

in |removeFromDownloadsQueue|, you're completely changing |this.downloadsQueue| (because of the call to |splice|). This means that |this.downloadsQueue[i]| has a good chance of skipping one item.

See for yourself:

var array = [0, 1, 2, 3, 4, 5, 6, 7, 8];
for (var i = 0, l = array.length; i < l; i++) { console.log(i, array[i]); array.splice(i, 1); }
console.log(array);
=> [1, 3, 5, 7]

This is the same result than :

var array = [0, 1, 2, 3, 4, 5, 6, 7, 8];
array.forEach(function(val, i) {  console.log(i, val); array.splice(i, 1);});
console.log(array);
=> [1, 3, 5, 7]


The _real_ fix here is to create a copy of the array before iterating:

this.downloadsQueue.slice().forEach(function(updatableApp) {
  updatableApp.cancelDownload();
});

However, I'm still ok with moving the call to removeFromDownloadsQueue here, this feels better.

::: apps/system/test/unit/mock_updatable.js
@@ +27,5 @@
>    this.mCancelCalled = true;
>  };
>  
> +function MockSystemUpdatable() {
> +  this.size = null;

shouldn't you set |size| each time you create a new MockSystemUpdatable in update_manager.js then ?

ok, maybe not, so I let you choose but I wanted this not to slip out of your attention.

::: apps/system/test/unit/update_manager_test.js
@@ +897,2 @@
>          UpdateManager.updatableApps = updatableApps;
> +        [systemUpdatable, uAppWithDownloadAvailable].forEach(function(updatable) {

try adding 3 more apps here and you'll see what I'm saying above.

@@ +897,5 @@
>          UpdateManager.updatableApps = updatableApps;
> +        [systemUpdatable, uAppWithDownloadAvailable].forEach(function(updatable) {
> +          UpdateManager.addToUpdatesQueue(updatable);
> +          UpdateManager.addToDownloadsQueue(updatable);
> +        });

\o/

@@ +1125,3 @@
>          });
>  
>          test('should not add more than on system update', function() {

nit: one
Attachment #701827 - Flags: review?(felash) → review-
(Reporter)

Comment 6

6 years ago
Created attachment 702237 [details] [diff] [review]
test modification to show why this doesn't work

apply this after etienne's patch
(Assignee)

Comment 7

6 years ago
(In reply to Julien Wajsberg [:julienw] from comment #5)
> I'd love to see a |git format-patch -U8| output next time ;)

Hey as soon as we can have it in a git config we'll be set :)

> thinking of this a little bit more, I'm not sure this is the right fix.
> Actually I'm quite sure this is wrong :-)

Thanks for the demo patch :)

> 
> shouldn't you set |size| each time you create a new MockSystemUpdatable in
> update_manager.js then ?

I'd rather not since it's part of the UpdateManager responsibility.

Patch incoming!
(Assignee)

Comment 8

6 years ago
Created attachment 702305 [details] [diff] [review]
Patch v2
Attachment #701827 - Attachment is obsolete: true
Attachment #702237 - Attachment is obsolete: true
Attachment #702305 - Flags: review?(felash)
(Reporter)

Comment 9

6 years ago
Comment on attachment 702305 [details] [diff] [review]
Patch v2

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

¿ no test failing with your previous patch ?
(Reporter)

Comment 10

6 years ago
Comment on attachment 702305 [details] [diff] [review]
Patch v2

other than the missing test, r+ for me :)
Attachment #702305 - Flags: review?(felash) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 702380 [details] [diff] [review]
Demo patch to apply on top of the Patch v2

While cleaning up the test I realized that the first version of the patch is actually working properly...

The patch you proposed to prove its wrongness was actually using an AppUpdatable instead of a MockAppUpdatable (we're in the UpdateManager tests here).

Not sure where to go from here but I'm sure we'd both like to really understand what's happening here :)
(Reporter)

Comment 12

6 years ago
(In reply to Etienne Segonzac (:etienne) from comment #11)
> Created attachment 702380 [details] [diff] [review]
> Demo patch to apply on top of the Patch v2
> 
> While cleaning up the test I realized that the first version of the patch is
> actually working properly...

That actually surprises me a lot.

> 
> The patch you proposed to prove its wrongness was actually using an
> AppUpdatable instead of a MockAppUpdatable (we're in the UpdateManager tests
> here).

nope, AppUpdatable is mocked in update_manager.js, so it is actually MockAppUpdatable. I thought of that and decided to use the same code than in the main setup where we also use AppUpdatable :)
(Assignee)

Comment 13

6 years ago
(In reply to Julien Wajsberg [:julienw] from comment #5)
> var array = [0, 1, 2, 3, 4, 5, 6, 7, 8];
> for (var i = 0, l = array.length; i < l; i++) { console.log(i, array[i]);
> array.splice(i, 1); }
> console.log(array);
> => [1, 3, 5, 7]

You're not starting from the end here (like in the v1 patch).
(Assignee)

Comment 14

6 years ago
Created attachment 702396 [details] [diff] [review]
Patch v1bis ;)

Here we go, this a revision of the first patch without copy but implementing a suggestion from Julien making it clearer. And obviously we need clarity here :)
Attachment #702305 - Attachment is obsolete: true
Attachment #702380 - Attachment is obsolete: true
Attachment #702396 - Flags: review?(felash)
(Reporter)

Comment 15

6 years ago
Comment on attachment 702396 [details] [diff] [review]
Patch v1bis ;)

r=julienw

This code is perfect, let's land this :)
Attachment #702396 - Flags: review?(felash) → review+
(Assignee)

Comment 16

6 years ago
https://github.com/mozilla-b2g/gaia/commit/4d08e77a9635b8760d88c5c33618ebdbbc517b4b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: verifyme
QA Contact: jsmith

Updated

6 years ago
status-b2g18: --- → fixed
Verified on 1/24.
Status: RESOLVED → VERIFIED
Keywords: verifyme
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.
status-b2g18-v1.0.0: --- → fixed
You need to log in before you can comment on or make changes to this bug.