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.

regression from bug 804571? Could you place which build are you on please, julienw?

oops. undid the tef... replacing it back

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.

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

apply this after etienne's patch

(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!

Comment on attachment 702305 [details] [diff] [review] Patch v2 Review of attachment 702305 [details] [diff] [review]: ----------------------------------------------------------------- ¿ no test failing with your previous patch ?

Comment on attachment 702305 [details] [diff] [review] Patch v2 other than the missing test, r+ for me :)

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 :)

(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 :)

(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).

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 :)

Comment on attachment 702396 [details] [diff] [review] Patch v1bis ;) r=julienw This code is perfect, let's land this :)

Verified on 1/24.

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.