1163973 - The simultaneous install process of two separate add-ons from webpages does not work properly

Status: VERIFIED FIXED

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

Description

[Vasilica Tamas, QA [:vtamas]]

Attachment: multiple-addons-nightly.gif

Prerequisites: create xpinstall.signatures.dev-root pref and set it to true

Reproducible on: Firefox 41.0a1 and Firefox 40.0a2 across all platforms

STR
1.Launch Firefox with clean profile
2.Go to https://addons-dev.allizom.org/en-US/firefox/extensions/?sort=users
3.Click “Add to Firefox” green button to install the first add-on (e.g. Adblock Plus)
4.Click “Allow” button from add-on install doorhanger.
5.During the first install click “Add to Firefox” green button to install the second add-on (e.g Video DownloadHelper) 
6.Click “Allow” button.

ER
Two overlapped doorhangers appear displaying the install process. 

AR
The first add-on install doorhanger is replaced by the second one, when the second download process is completed.

Additional notes:
 - This issue is reproducible on Firefox 41.0a1 (2015-05-11) and Firefox 40.0a2 (2015-05-11) using Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 
 - I am attaching a video recorded on Windows 7 x64 where I captured this issue.

Comment 1

[Dave Townsend [:mossop]]

[Tracking Requested - why for this release]:

Regressed by bug 1139656. I have no idea how common this kind of behaviour is but we probably want to correct it.

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding tracking flags for both firefox40 and firefox41.

Comment 3

[Dão Gottwald [:dao]]

I guess I'll take this. Feel free to steal if you have free cycles though. I'll leave another note when I actually start working on this.

Comment 4

[Dave Townsend [:mossop]]

This is turning out to be hard. The popup notifications aren't really intended for showing multiple instances of the same notification. I have a plan to fix this but it is pretty invasive and I would be wary of asking for such a change to be uplifted to aurora. I might be able to do a simpler version that is safer but that increases the development time. If not we're left with a problem for 40. I'd like product's input on what to do here, we have a couple of options I can think of:

1. Do nothing. I don't have any data on how often people try to do this multiple install thing but I'm betting it isn't a lot. Having it broken for six weeks might not be terrible.

2. Revert to the old modal install dialog for all installs. We know this works well. Right now it will display a note about the add-on author not being verified but we can hide that easily. The problem case is where add-on signing is preffed off and they try to install an unsigned add-on. They'll not see any warning about that.

Comment 5

[Dão Gottwald [:dao]]

My plan was to queue the install confirmations, i.e. only show the second one once the first one is gone.

Comment 6

[Dave Townsend [:mossop]]

That might work as a temporary measure. It doesn't work well with the case where the user just dismisses the doorhanger rather that cancels/installs the first confirmation.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch

This is a bit ugly but it seems to get the job done. Most of the patch is just moving things around, we wait for the security delay then get the progress notification size and close it then attempt to show the confirmation. If a confirmation is already open we stash the info and when a confirmation closes we open the next in the queue.

We only show the next notification when the user explicitly cancels or installs the current one. In odd cases the notification might get the size from the wrong progress notification (we don't support multiple of those either).

So not perfect but seems safe enough that we would be able to uplift these changes to aurora. I've filed bug 1171636 for a more comprehensive solution.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8615513 [details] [diff] [review]
patch

>+  showInstallConfirmation: function(browser, { installInfo, height }) {
>+    // If the confirmation notification is already open cache the installInfo
>+    // and the new confirmation will be shown later
>+    if (PopupNotifications.getNotification("addon-install-confirmation", browser)) {
>+      let pending = this.pendingInstalls.get(browser);
>+      if (pending) {
>+        pending.push({
>+          installInfo,
>+          height
>+        });
>+      } else {
>+        this.pendingInstalls.set(browser, [{
>+          installInfo,
>+          height
>+        }]);

Storing 'height' probably isn't very useful since the popup will have changed anyway by the time this would be used.

>+      // Make sure the browser is still alive.
>+      if (gBrowser.browsers.indexOf(browser) == -1)
>+        return;
>+
>+      let pending = this.pendingInstalls.get(browser);
>+      if (pending && pending.length)
>+        this.showInstallConfirmation(browser, pending.shift());

Looks like "Make sure the browser is still alive" shouldn't be needed. You're already null-checking 'pending'.

>+      ok(true, "1");

info(1); or just remove this in case it's a development leftover. Doesn't look overly useful.

>+        ok(true, "2");

ditto

Comment 9

[Dave Townsend [:mossop]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 8615513 [details] [diff] [review]
> patch
> 
> >+  showInstallConfirmation: function(browser, { installInfo, height }) {
> >+    // If the confirmation notification is already open cache the installInfo
> >+    // and the new confirmation will be shown later
> >+    if (PopupNotifications.getNotification("addon-install-confirmation", browser)) {
> >+      let pending = this.pendingInstalls.get(browser);
> >+      if (pending) {
> >+        pending.push({
> >+          installInfo,
> >+          height
> >+        });
> >+      } else {
> >+        this.pendingInstalls.set(browser, [{
> >+          installInfo,
> >+          height
> >+        }]);
> 
> Storing 'height' probably isn't very useful since the popup will have
> changed anyway by the time this would be used.

Good point.

> >+      // Make sure the browser is still alive.
> >+      if (gBrowser.browsers.indexOf(browser) == -1)
> >+        return;
> >+
> >+      let pending = this.pendingInstalls.get(browser);
> >+      if (pending && pending.length)
> >+        this.showInstallConfirmation(browser, pending.shift());
> 
> Looks like "Make sure the browser is still alive" shouldn't be needed.
> You're already null-checking 'pending'.

I think it still is, garbage collection may not have happened between the browser being removed and this running so it may still be alive (we might even be keeping it alive with the closure here) so I'd like to leave this in for safety.

> >+      ok(true, "1");
> 
> info(1); or just remove this in case it's a development leftover. Doesn't
> look overly useful.
> 
> >+        ok(true, "2");
> 
> ditto

Oh yeah this was for debugging, I'll drop it.

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0109dcffb95d

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8615513 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1139656
[User impact if declined]: Users installing two add-ons from the same site will only see the install prompt for one of them
[Describe test coverage new/current, TreeHerder]: Automated testing and on m-c for a few days.
[Risks and why]: Low, we have pretty good automated tests here and nightly users generally cry quickly if add-on installation breaks.
[String/UUID change made/needed]: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 14

[Dave Townsend [:mossop]]

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

Comment 15

[Vasilica Tamas, QA [:vtamas]]

Tested this issue on Firefox 40.0a2 (2015-06-10) and Firefox 41.0a1 (2015-06-10) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5. Multiple add-ons are successfully installed simultaneously. 

I have noticed 2 potential issues:
  1.The first restart doorhanger is replaced by the second one, when the second download process is completed: 
       - This issue is not so important, considering that the purpose is the same
       - It is reproducible across all platforms

  2. Click “Add to Firefox” green button to install an add-on -> Wait until the download process is completed -> Click “Add to Firefox” to install a second add-on -> Wait for the second download process to be completed: 
       - The first doorhanger does not remain displayed during the second download process 
       - After the second download is finished the doorhanger is closed and the user has to click on the add-on icon from location bar to reopen the doorhanger
       - This issue reproduces only on Ubuntu.

Dave, do you think it is necessary to file new bugs for this issues or they should be fixed by Bug 1171636 ?

Comment 16

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #15)
> Tested this issue on Firefox 40.0a2 (2015-06-10) and Firefox 41.0a1
> (2015-06-10) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X
> 10.9.5. Multiple add-ons are successfully installed simultaneously. 
> 
> I have noticed 2 potential issues:
>   1.The first restart doorhanger is replaced by the second one, when the
> second download process is completed: 
>        - This issue is not so important, considering that the purpose is the
> same
>        - It is reproducible across all platforms
> 
>   2. Click “Add to Firefox” green button to install an add-on -> Wait until
> the download process is completed -> Click “Add to Firefox” to install a
> second add-on -> Wait for the second download process to be completed: 
>        - The first doorhanger does not remain displayed during the second
> download process 
>        - After the second download is finished the doorhanger is closed and
> the user has to click on the add-on icon from location bar to reopen the
> doorhanger
>        - This issue reproduces only on Ubuntu.
> 
> Dave, do you think it is necessary to file new bugs for this issues or they
> should be fixed by Bug 1171636 ?

The first issue should be solved by bug 1171636. The second issue is worth filing, I'm surprised it's happening since we should be catching that in the tests.

Comment 17

[Vasilica Tamas, QA [:vtamas]]

Filed Bug 1174684 for the second issue mentioned in Comment 15

Based on my previous testing, I am marking this bug as Verified since the other issues are tracked separately.