1147808 - Be smarter about unfinished add-on installations whose install UI is gone because the tab / window closed

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Dão Gottwald [:dao]]

From bug 1139656 comment 16:

> > > How about when the tab is closed? Do the installs just sit dormant somehow?
> > 
> > These popup notifications are always tab-specific, so they would go away
> > with a tab. However I'm not sure PopupNotifications.jsm fires a "removed"
> > event in that case, so the installs might linger in the background... and
> > are hopefully automatically cancelled when restarting the application?
> > There's also the scenario of the whole window closing, in which case there's
> > no TabClose event, so just listening to that wouldn't be sufficient.
> 
> We should get a follow-up bug on file for doing sane things here rather than
> just dropping the installs on the floor. At the least I note that the
> add-ons manager UI looks bad for these cases so we either need to fix that
> or properly cancel the installs when they are no longer reachable through
> the UI.

Comment 1

[Dave Townsend [:mossop]]

Similar to bug 1147812, the new mockups in bug 1120996 suggest that we should show a tab-model prompt asking if the user wants to leave the site or not before cancelling the install. I think that can be follow-up fodder though.

Comment 2

[Dão Gottwald [:dao]]

Attachment: cancel installation when the tab or window closes while downloading the add-on or waiting for the user to confirm the installation

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8605220 [details] [diff] [review]
cancel installation when the tab or window closes while downloading the add-on or waiting for the user to confirm the installation

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

Looks good. A little awkward that it doesn't remove the listeners when the install completes, you could use a weakmap keyed off the installInfo object to get the function to do that. I'd love to see a test here.

Comment 4

[Dão Gottwald [:dao]]

Attachment: cancel installation when the tab or window closes while downloading the add-on or waiting for the user to confirm the installation (v2)

(In reply to Dave Townsend [:mossop] from comment #3)
> A little awkward that it doesn't remove the listeners when the
> install completes,

That was an oversight (actually caused test failures because the code tried to cancel installs that couldn't be cancelled anymore)

> you could use a weakmap keyed off the installInfo object
> to get the function to do that.

I'm not sure how exactly this is supposed to work. I just removed the listeners explicitly now when the progress notification goes away. The confirm notification already did that.

> I'd love to see a test here.

added

Comment 5

[Dão Gottwald [:dao]]

Attachment: cancel installation when the tab or window closes while downloading the add-on or waiting for the user to confirm the installation (v3)

missed a return statement in the test

Comment 6

[Pulsebot]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for addon manager test failures.
https://hg.mozilla.org/integration/fx-team/rev/1725e7758d6b

https://treeherder.mozilla.org/logviewer.html#?job_id=3093819&repo=fx-team

Comment 8

[Dão Gottwald [:dao]]

Attachment: cancel installation when the tab or window closes while waiting for the user to confirm the installation (v4)

I removed the cancel-while-downloading part since this confuses tests and isn't part of the regression this bug was filed for anyway.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7aa664ca77c9

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7aa664ca77c9

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8606616 [details] [diff] [review]
cancel installation when the tab or window closes while waiting for the user to confirm the installation (v4)

Approval Request Comment
[Feature/regressing bug #]: bug 1139656
[User impact if declined]: see comment 1
[Describe test coverage new/current, TreeHerder]: has test
[Risks and why]: reasonably straightforward fix, shouldn't be very risky
[String/UUID change made/needed]: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d1bc226bc2cf

Comment 13

[Vasilica Tamas, QA [:vtamas]]

I understood throughout the comments that the expected behavior when a tab/window is closed during the add-on download process is that the installation is canceled. Am I right?

I have tested on Firefox 41.0a1 (2015-06-02) and Firefox 40.0a2 (2015-06-02) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 and when a tab/window is closed during the add-on download process, the tab/windows is successfully closed without being displayed a tab-model prompt to require the user approval before cancelling the install.

Is this the expected behavior? 
If the answer is yes, I have noticed a potential issue: closing the tab during the add-on download process causes an incorrect add-on display in about:addons http://i.imgur.com/llGZo7g.jpg .A browser restart solves this issues.

Comment 14

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #13)
> I understood throughout the comments that the expected behavior when a
> tab/window is closed during the add-on download process is that the
> installation is canceled. Am I right?
> 
> I have tested on Firefox 41.0a1 (2015-06-02) and Firefox 40.0a2 (2015-06-02)
> under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 and when a
> tab/window is closed during the add-on download process, the tab/windows is
> successfully closed without being displayed a tab-model prompt to require
> the user approval before cancelling the install.
> 
> Is this the expected behavior? 
> If the answer is yes, I have noticed a potential issue: closing the tab
> during the add-on download process causes an incorrect add-on display in
> about:addons http://i.imgur.com/llGZo7g.jpg .A browser restart solves this
> issues.

Yes, we haven't implemented the modal prompt at the moment. The UI issue shouldn't be happening though, I've filed bug 1171148 for this.