581229 - Post install notification is dismissed if the web page redirects after the installation (whether it succeeds or fails)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Tony Chung [:tchung]]

Some extensions are experiencing redirects and cert issues with the new Addons Manager work.   As a result, the notifications flicker really quickly due to the site redirect, and never shows the user why the install failed.

Examples:  personas plus, test pilot.

Repro:
1) install Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2) Gecko/20100720 Firefox/4.0b2
2) Visit http://www.getpersonas.com/en-US/ or http://mozillalabs.com/testpilot/ and download the extension thre
3) Verify the page notification flickers a split second, then tries to redirect to AMO, and then back.  Nothing installs, neither does it appear in addons manager
4) open error console, and see warning with the redirect

"WARN addons.xpi: Download failed: TypeError: channel.securityInfo is null

Expected:
- failing to download due to AMO cert should show the notification for a reasonable amount of time

Actual:
- notification flickers once AMO tries to redirect

Comment 1

[Dave Townsend [:mossop]]

Realized this will happen anytime the webpage redirects immediately after the install completes. Currently notifications go away the moment the webpage redirects. We should add a timer so page loads within the first 30 seconds or something does not dismiss the notification.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

See also bug 578683 for the Personas installation. Isn't it somehow related / the same?

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #2)
> See also bug 578683 for the Personas installation. Isn't it somehow related /
> the same?

Same problem, different notifications

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch adds the persistence and timeout options to popup notifications. I'm not a big fan of the naming but they are consistent with the notification bars so I stuck with it.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 460953 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm

>+   * @param options
>+   *        An options JavaScript object holding additional properties for the
>+   *        notification.

Would be good to at least mention which options are currently supported. Documenting what they do (and how they interact) would be even better!

>   locationChange: function PopupNotifications_locationChange() {

You could use this._currentNotifications.filter() here to simplify things a bit.

Comment 6

[Dave Townsend [:mossop]]

Fixed: http://hg.mozilla.org/mozilla-central/rev/e1e1e7db3df5

Comment 7

[Nochum Sossonko [:Natch]]

This failure: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280453097.1280454913.8883.gz came from a change made in this bug (according to hg blame).

There seems to be a syntax error in the test.

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> This failure:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280453097.1280454913.8883.gz
> came from a change made in this bug (according to hg blame).
> 
> There seems to be a syntax error in the test.

I *think* the error was different than how you expected, as I think the function was just added in the wrong place, it shouldn't be inside |function runNextTest()| but inside |var TESTS| from what I see.

IIUC, the added test case for this bug in browser_bug553455.js is not run currently!

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> (In reply to comment #7)
> > This failure:
> > http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280453097.1280454913.8883.gz
> > came from a change made in this bug (according to hg blame).
> > 
> > There seems to be a syntax error in the test.
> 
> I *think* the error was different than how you expected, as I think the
> function was just added in the wrong place, it shouldn't be inside |function
> runNextTest()| but inside |var TESTS| from what I see.
> 
> IIUC, the added test case for this bug in browser_bug553455.js is not run
> currently!

You're right, I think the result of a strange merge or something. I just landed the correction as http://hg.mozilla.org/mozilla-central/rev/6f99979d11bf

Comment 10

[Robert Kaiser]

Hehe, nice, sometimes it helps when you have someone else port your work to another product. ;-)

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed by testing the Walnut theme and Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b3pre) Gecko/20100802 Minefield/4.0b3pre