Closed Bug 1147808 Opened 5 years ago Closed 4 years ago

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

Categories

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

defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Whiteboard: [hijacking][fxsearch])

Attachments

(1 file, 3 obsolete files)

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.
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.
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Iteration: 40.2 - 27 Apr → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Whiteboard: [fxsearch][searchhijacking]
Priority: -- → P1
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Rank: 10
Whiteboard: [fxsearch][searchhijacking] → [hijacking][fxsearch]
Iteration: 40.3 - 11 May → 41.1 - May 25
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.
Attachment #8605220 - Flags: review?(dtownsend) → review+
(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
Attachment #8605220 - Attachment is obsolete: true
Attachment #8606254 - Flags: review?(dtownsend)
missed a return statement in the test
Attachment #8606254 - Attachment is obsolete: true
Attachment #8606254 - Flags: review?(dtownsend)
Attachment #8606256 - Flags: review?(dtownsend)
Attachment #8606256 - Flags: review?(dtownsend) → review+
I removed the cancel-while-downloading part since this confuses tests and isn't part of the regression this bug was filed for anyway.
Attachment #8606256 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7aa664ca77c9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
Attachment #8606616 - Flags: approval-mozilla-aurora?
Attachment #8606616 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
You need to log in before you can comment on or make changes to this bug.