854740 - PopupNotifications.jsm doesn't handle stacked notifications well

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tanvi Vyas[:tanvi]]

In about:config set plugins.click_to_play to true and security.mixed_content.block_active_content to true.

When two notifications are stacked, and both dismissed by default, I don't see an issue: https://people.mozilla.com/~tvyas/mixedctp.html

When at least one notification is not dismissed by default, that notification doesn't appear as it should:
* Go to https://people.mozilla.com/~tvyas/javascript_post_https_mixed.html and enter a bogus username and password.  After you hit submit, you will get two icons next to the location bar. One from the password manager to save the password, and another from the mixed content blocker.  The password manager notification doorhanger is displayed for a split second and then disappears.

* Go to https://people.mozilla.com/~tvyas/passwordctp.html and enter a bogus username and password.  You will see the remember password doorhanger appear for a split second and then disappear.

One thing I haven't tested - when there are two notifications and both are not dismissed (ex: password manager and geolocation?), what is the current behavior?

This bug is to make PopupNotifications.jsm "smarter" and able to handle these cases.  I think the issue may be with the code here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PopupNotifications.jsm#574

Please update the Firefox Component appropriately.  I have just put this in General for now.

Comment 1

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

So what cases does it not handle exactly? It sounds like perhaps it's "dismissed notification being shown after non-dismissed notification hides the non-dismissed notification"? That should be relatively easy to fix, I think.

PopupNotifications._update() would just need to be smarter about picking the anchor to update. Might be as simple as doing the notificationsToShow filtering before it sets anchorElement.

Comment 2

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

Attachment: potential patch

Tanvi, can you test this and see if it gives the desired behavior?

Comment 3

[Tanvi Vyas[:tanvi]]

Comment on attachment 729500 [details] [diff] [review]
potential patch

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> So what cases does it not handle exactly? It sounds like perhaps it's
> "dismissed notification being shown after non-dismissed notification hides
> the non-dismissed notification"? That should be relatively easy to fix, I
> think.
> 

This isn't quite right: "dismissed notification being shown after non-dismissed notification hides the non-dismissed notification"

When we have one notification that is dismissed and one notification that is not dismissed, the notification that is not dismissed should still show up.  It shows up for a split second and then disappears (when it should stick around for 10 seconds or whatever it's timeout says).

I think the problem is that when one notification is in a default dismissed state and a second notification should NOT be dismissed, both notifications somehow get dismissed.  


The attached patch doesn't solve the problem.  I think the problem is that notificationsToShow.length is zero, when it should be one here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PopupNotifications.jsm#581

My guess is that something is off here, but I could very well be wrong because I haven't spent the time to see if the return logic here makes sense:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PopupNotifications.jsm#575

Comment 4

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

Attachment: patch

Oops, dumb mistake in the last patch. This should work better.

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> Created attachment 729673 [details] [diff] [review]
> patch
> 
> Oops, dumb mistake in the last patch. This should work better.

Your updated patch now shows both the dismissed and the un-dismissed notifications.  And it actually is showing the dismissed notification first (which might just be a coincidence).  We need a way to show only the one that is un-dismissed.

Also, the new code produces a bug in the error console:
Error: TypeError: notificationsToShow[0] is undefined
Source File: resource://gre/modules/PopupNotifications.jsm
Line: 565

To see how this works, just go to about:config and set security.mixed_content.block_active_content to true.  Go to 
https://people.mozilla.com/~tvyas/javascript_post_https_mixed.html and enter a bogus username and password.

Comment 6

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

Attachment: round 3

This one seems to work. You still get a weird flicker (because the password notification is shown, and then the mixed content one replaces it). There should be a way to avoid that (avoid overriding a shown popupnotification with a new one?), though I thought we maybe already tried to do that.

Comment 7

[Tanvi Vyas[:tanvi]]

Comment on attachment 729786 [details] [diff] [review]
round 3

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> Created attachment 729786 [details] [diff] [review]
> round 3
> 
> This one seems to work. You still get a weird flicker (because the password
> notification is shown, and then the mixed content one replaces it). There
> should be a way to avoid that (avoid overriding a shown popupnotification
> with a new one?), though I thought we maybe already tried to do that.

The mixed content blocker notification is dismissed by default and should not show up (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6866).  Only the passwords doorhanger should pop up.  With this patch, first the mixed content doorhanger pops up.  Once it is closed, the passwords doorhanger pops up.  The desired behavior is for only the passwords doorhanger to pop up.

What is causing the flicker of the passwords manager doorhanger?

Comment 8

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

Attachment: round 4

I'm just going to keep throwing patches up until one of them sticks (this is what happens when writing patches between meetings :)

I think this one works. I make no guarantees that it is complete! Fails tests, though, I think, and needs tests of its own.

Comment 9

[Tanvi Vyas[:tanvi]]

This latest patch seems to work :)  I pushed it to try to 1) see if it fixes the password manager tests that were previously breaking, and 2) see what it breaks.

https://tbpl.mozilla.org/?tree=Try&rev=15c162d0cd6d

Comment 10

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

Attachment: final patch

Here's a (simplified) final patch that passes the tests failing in that try run, and includes one of its own. Confirmed that the added test fails without the patch.

The basic problem was that PopupNotifications._update(), when called from show(), would dismiss the panel if there were no notifications to display for the given anchor. We don't really ever want show() to result in closing an open panel, AFAICT, so let's just have it not do that.

My previous patches were fixing other issues which I'm not sure yet matter in practice - I will investigate in a followup.

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> Created attachment 730169 [details] [diff] [review]
> final patch
> 
> Here's a (simplified) final patch that passes the tests failing in that try
> run, and includes one of its own. Confirmed that the added test fails
> without the patch.
> 
> The basic problem was that PopupNotifications._update(), when called from
> show(), would dismiss the panel if there were no notifications to display
> for the given anchor. We don't really ever want show() to result in closing
> an open panel, AFAICT, so let's just have it not do that.
> 
> My previous patches were fixing other issues which I'm not sure yet matter
> in practice - I will investigate in a followup.

The test pages (in comment 0) work as expected now, but with an odd flicker where the plugin or mixed content notification (not the doorhanger) appears, disappears, and then reappears.


The password manager test_prompt.html test is still failing, but for a later test - test 1004 instead of test 1003.  The error is the same - notification is undefined.  Not sure if this has to do with the odd flicker.  Will keep investigating.

TEST-PASS | unknown test url | handleLoad running for test 1004
TEST-PASS | unknown test url | Time is Wed, 27 Mar 2013 21:07:51 GMT
TEST-PASS | unknown test url | is popup panel open? false
TEST-PASS | unknown test url | Found 1 popup notifications.
TEST-PASS | unknown test url | #0: password-change
TEST-PASS | unknown test url | Found 0 notification bars.
TEST-PASS | unknown test url | handleDialog was invoked
TEST-PASS | unknown test url | Checking for successful authentication
TEST-PASS | unknown test url | Checking for echoed username
TEST-PASS | unknown test url | Checking for echoed password
TEST-PASS | unknown test url | Looking for password-change popup notification
TEST-PASS | unknown test url | got popup notification
TEST-PASS | unknown test url | Looking for action at index 0
TEST-UNEXPECTED-FAIL | unknown test url | at least one notification displayed
TEST-PASS | unknown test url | 0 notifications
TEST-PASS | unknown test url | Triggering main action
TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - TypeError: notification is undefined at http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/notification_common.js:43
JavaScript error: http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/notification_common.js, line 43: notification is undefined

Comment 12

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

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> The test pages (in comment 0) work as expected now, but with an odd flicker
> where the plugin or mixed content notification (not the doorhanger) appears,
> disappears, and then reappears.

Isn't this because the navigation hides the notification, but then it is redisplayed again? I.e. this is a problem with when we call .show(), not with PopupNotifications itself.

> The password manager test_prompt.html test is still failing

Passes for me. Does it pass for you without the patch applied?

Comment 13

[Tanvi Vyas[:tanvi]]

Without Gavin's patch to PopupNotifications.jsm applied, but with the patch to set the block mixed active content pref to true (https://bugzilla.mozilla.org/attachment.cgi?id=713069&action=edit) we get the following errors: https://tbpl.mozilla.org/?tree=Try&rev=bff4225d1454

With Gavin's "round 4" patch and with the patch to set the block mixed active content pref to true, we get: https://tbpl.mozilla.org/?tree=Try&rev=15c162d0cd6d.  More specifically
* Test http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_prompt.html?force=1 succeeds.
* Test https://mxr.mozilla.org/mozilla-central/find?text=&string=%2Ftests%2Ftoolkit%2Fcomponents%2Fpasswordmgr%2Ftest%2Ftest_notifications_popup.html still timesout (I haven't investigated this one yet)
* Other tests fail.

With Gavin's "final patch" and with the patch to set the block mixed active content pref to true:
* Here is a ne push to try: https://tbpl.mozilla.org/?tree=Try&rev=8fda47c3ed70.  Let's see if test_prompt.html or test_notifications_popup.html have any problems.
* A run of test_prompt.html locally passes, but I get inconsistent behavior when I refresh the test. Sometimes it will get an error saying "notifications is undefined" and sometimes it will pass.
* A run of test_notifications_popup.html passes locally without a timeout, but again I am getting inconsistent behavior when I refresh the test.  Sometimes it will time out, sometimes it will pass.

Comment 14

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #13)
> With Gavin's "final patch" and with the patch to set the block mixed active
> content pref to true:
> * Here is a ne push to try:
> https://tbpl.mozilla.org/?tree=Try&rev=8fda47c3ed70.  Let's see if
> test_prompt.html or test_notifications_popup.html have any problems.

Try shows that test_prompt.html is failing again (in the round 4 patch it didn't fail).

test_notifications_popup.html still times out.

(I still don't understand where the mixed content on the passwords tests is coming from; going to run this through gdb to see if I can figure that out.)

Comment 15

[Tanvi Vyas[:tanvi]]

> (I still don't understand where the mixed content on the passwords tests is
> coming from; going to run this through gdb to see if I can figure that out.)

The try logs for test_prompt.html show that 2 popup notifications are found on the try server (one for mixed content, one for password manager):

Found 2 popup notifications.
#0: mixed-content-blocked
#1: password-change

Locally, I add a breakpoint in nsMixedContentBlocker.cpp and run the tests.  The breakpoint is never hit, and in my local logs, only 1 popup notification is found:

Found 1 popup notifications.
#0: password-change

The words "mixed-content-blocked" never show up.  So where is the mixed content coming from?  I'm running this on a mac and the try push is only to windows, so maybe this is a windows only issue.  Pushed to more platforms to see what happens:
https://tbpl.mozilla.org/?tree=Try&rev=f8cfbda6db00



The test still does fail locally on refresh though.

Comment 16

[Tanvi Vyas[:tanvi]]

Try shows that this failure is not platform specific.

Comment 17

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

I pushed my patch and an "enable active mixed content blocking" patch to try, and I don't see any failures in the passwordmgr tests (
test_notifications_popup.html/test_prompt.html):

https://tbpl.mozilla.org/?tree=Try&rev=644c0d79a164

So I think the failures you're seeing are caused by other stuff in your Try push?

Comment 18

[Tanvi Vyas[:tanvi]]

I believe everything else I had pushed to try has already landed.  Maybe it was because I was working off of a March 22nd parent?  Not sure.
https://tbpl.mozilla.org/?tree=Try&rev=f8cfbda6db00

I talked to Matt today and now understand why the mixed content notification appears when there is no mixed content on the password manager tests.  Some previous test probably did have mixed content and prompted the notification.  This is expected behavior.  If you have an https page with mixed content you will get the mixed content notification.  If after interacting with the page or having the page open for some time that mixed content goes away (the page no longer wants to load mixed content), then the notification won't disappear.  I don't believe this is an issue.

Anyway, I am going to look into a few more things and then will comment on this bug again, and actually give feedback on the patch, since it seems to be working in Gavin's push to try.

Comment 19

[Tanvi Vyas[:tanvi]]

Pushed to try again with the pref turned on and Gavin's "final patch": https://tbpl.mozilla.org/?tree=Try&rev=ecb21e5584a1

I am also running mochitest-5 locally with an alert in the popup notification for mixed content blocker so that I can see which test has mixed content.  Unless it is specifically testing mixed content, our mochitests shouldn't' have mixed content in them.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 730169 [details] [diff] [review]
final patch

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

r+ with some possible improvements.

This doesn't seem to fix all of the issues Tanvi is seeing because the password manager test fails in a different place on trees with bug 834836 included (e.g. inbound and Tanvi's try pushes).  Tanvi is looking into them more and we can address them in a different bug if there's a different cause.

::: toolkit/content/PopupNotifications.jsm
@@ +550,5 @@
>    /**
>     * Updates the notification state in response to window activation or tab
>     * selection changes.
>     */
> +  _update: function PopupNotifications_update(anchor, fromShow) {

How about dismissShowing/dismissCurent/etc. instead of tying the argument to a caller's name? An options object (that defaults to {}) is a bit easier to read from callers than a boolean argument. If we keep this as a boolean, I think we should have the default value be false rather than undefined.

Nit: adding the arguments to the comment wouldn't hurt.

@@ +581,5 @@
>      if (notificationsToShow.length > 0) {
>        this._showPanel(notificationsToShow, anchorElement);
>      } else {
>        // Notify observers that we're not showing the popup (useful for testing)
>        this._notify("updateNotShowing");

It seems like this should be inside the |if (!fromShow)| block because the popup will continue to be shown if fromShow is true. I'm not sure if this is going to break the handful of tests that use the notification but I suspect they aren't testing multiple notifications.

I guess it depends on whether this notification indicates that the panel is no longer showing or whether it's about a specific notification not showing. Since _notify doesn't pass along a subject to the observers, this may be better off referring to the panel since consumers won't know which notification is being closed.  OTOH, we already have the popuphidden event for that.  What do you think?

Comment 21

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #20)
> This doesn't seem to fix all of the issues Tanvi is seeing because the
> password manager test fails in a different place on trees with bug 834836
> included (e.g. inbound and Tanvi's try pushes).

Actually, it's failing in the same place so we should probably figure out the cause before pushing this.

Comment 22

[Tanvi Vyas[:tanvi]]

Attachment: Reproduce Failure - Add mixed content to password manager test

Okay I can reproduce the failures locally.  I will attach the logs in the next bug comment.  Here is how to reproduce, along with a bunch of other details:

1) Apply Gavin's "final patch".
2) Apply patch https://bugzilla.mozilla.org/attachment.cgi?id=713069&action=edit to turn mixed active blocking on.
3) Apply this attached patch that hacks up a passwords manager mochitest and just adds some mixed content to it.  This way, the mixed content notification is present[1]
4) Run the passwordmgr mochitests:
TEST_PATH=toolkit/components/passwordmgr/test/ make -C obj-ff-dbg/ mochitest-plain

[1] The mixed content notification is present in mochitest-5 because of previous tests that are run with mixed content in them (for example, the mochitests that test mixed content).  The reason I wasn't able to reproduce this earlier was because I was only testing individual mochitests that had no mixed content present and hence no mixed content notification to interfere with the password manager notification.  The testing harness tests all mochitests in mochitest-5 one after another without navigating/reloading the top level page.  Once the mixed content notification became present, it didn't go away.  The password manager tests saw two notifications when they were used to only seeing one.  That is how we discovered that there are issues in stacking notifications.

Gavin's final patch does fix the manual test cases listed in comment 0.  But there must be some other issue with the stacked notifications, since even with his patch applied, we still have failures in 



For a sanity check, here are the patches I have applied.  They are on top of:
changeset:   126538:2f63e2f90d5c
user:        Aaron Klotz <[e-mail]>
date:        Thu Mar 28 09:38:18 2013 -0600
files:       testing/talos/talos.json
description:
Bug 855507: Update talos.json to capture new xperf main thread I/O counters. r=aki

hg qser:
 0 A FixTests-dom.patch
 1 A FixTests-csp.patch
 2 A FixTests-mixedcontent.patch
 3 A pref-on-fx.patch
 4 A Bug854740.patch
 5 A ReproducePassmgrFailures.patch

Note that the three "FixTests-*" patches are all updates to mochitests that are not in toolkit/components/passwordmgr/test/.  And hence have no effect on the tests run.  They are from bug 834836, have landed in inbound but have not merged to central yet.

Comment 23

[Tanvi Vyas[:tanvi]]

Attachment: Logs from reproducing the mochitest failures

Logs showing the password manager test failures.  Specifically:

mochitest-plain failed:
1294 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_notifications_popup.html | Test timed out.
1982 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_prompt.html | at least one notification displayed
1985 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components

Comment 24

[Tanvi Vyas[:tanvi]]

Attachment: Reproduce Failure Take 2 - Add mixed content to the password manager tests that fail on try.

Making this a little bit easier to reproduce, so that you only need to run the two failing tests instead of all the password manager tests...

1) Apply Gavin's "final patch".
2) Apply patch https://bugzilla.mozilla.org/attachment.cgi?id=713069&action=edit to turn mixed active blocking on.
3) Apply this attached patch that add mixed content to test_notifications_popup.html and test_prompt.html

4) Run:
TEST_PATH=toolkit/components/passwordmgr/test/test_notifications_popup.html make -C obj-ff-dbg/ mochitest-plain

It will timeout.  (BUT I discovered that if you manually click on the key icon in the notifications panel when you see that the test is starting to hang, the test will continue.  It will start to timeout again later.  Clicking on it again works, and the test finishes and passes.  Not sure if this info helps or not)

5) Run: 
TEST_PATH=toolkit/components/passwordmgr/test/test_notifications_popup.html make -C obj-ff-dbg/test_prompt.html mochitest-plain

You will get an error:
TEST-PASS | unknown test url | handleLoad running for test 1003
TEST-PASS | unknown test url | Time is Sat, 30 Mar 2013 02:36:35 GMT
TEST-PASS | unknown test url | is popup panel open? false
TEST-PASS | unknown test url | Found 2 popup notifications.
TEST-PASS | unknown test url | #0: mixed-content-blocked
TEST-PASS | unknown test url | #1: password-change
TEST-PASS | unknown test url | Found 0 notification bars.
TEST-PASS | unknown test url | handleDialog was invoked
TEST-PASS | unknown test url | Checking for successful authentication
TEST-PASS | unknown test url | Checking for echoed username
TEST-PASS | unknown test url | Checking for echoed password
TEST-PASS | unknown test url | Looking for password-change popup notification
TEST-PASS | unknown test url | got popup notification
TEST-PASS | unknown test url | Looking for action at index 0
TEST-UNEXPECTED-FAIL | unknown test url | at least one notification displayed
TEST-PASS | unknown test url | 0 notifications
TEST-PASS | unknown test url | Triggering main action
TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - TypeError: notification is undefined at http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/notification_common.js:43
JavaScript error: http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/notification_common.js, line 43: notification is undefined
1982 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_prompt.html | at least one notification displayed
1985 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components

Comment 25

[Tanvi Vyas[:tanvi]]

I updated my repo yesterday, applied the following patches and pushed to try:
 0 A Bug855730-3.patch - Small change in an unrelated mochitest
 1 A pref-on-fx.patch - Turns the mixed content pref on
 2 A Bug854740.patch - Gavin's "final patch".

https://tbpl.mozilla.org/?tree=Try&rev=103150888efe

This time, the password manager tests did not fail!!!  Perhaps I pulled in another patch that helped resolve the issue.

I tried to reproduce the failure locally again (as discussed in comments 22-24) and I can no longer reproduce it.

Seems like this patch does in fact solve the problem.  Once Gavin makes the changes proposed by Matt, we do another push to try,  and this patch lands, I can land bug 834836 to start blocking mixed active content.

Thanks Matt and Gavin for your help with this!

Comment 26

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #25)
> This time, the password manager tests did not fail!!!  Perhaps I pulled in
> another patch that helped resolve the issue.
> 
Perhaps the patches in bug 839516 or bug 825804 helped.

Comment 27

[Tanvi Vyas[:tanvi]]

Comment on attachment 730169 [details] [diff] [review]
final patch

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

Feedback +.  I agree with Matt that the parameter name "fromShow" should be changed.

Comment 28

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

(In reply to Matthew N. [:MattN] from comment #20)
> How about dismissShowing/dismissCurent/etc. instead of tying the argument to
> a caller's name?

Good call, will switch to dismissShowing.

> An options object (that defaults to {}) is a bit easier to read from callers than
> a boolean argument.

This is internal utility code only, so I think that may be overkill. I'd agree if this was exposed to external callers.

> If we keep this as a boolean, I think we should have the default value be false
> rather than undefined.

Makes sense.

> Nit: adding the arguments to the comment wouldn't hurt.

Added.

> >        this._notify("updateNotShowing");
> 
> It seems like this should be inside the |if (!fromShow)| block because the
> popup will continue to be shown if fromShow is true. I'm not sure if this is
> going to break the handful of tests that use the notification but I suspect
> they aren't testing multiple notifications.

This notification is meant to indicate "call to _update did not show a new notification", so I think dismissShowing shouldn't have any impact on whether it is fired. Since this is intended as a test-only notification anyhow, I'm not inclined to change anything unless the tests require it.

Comment 29

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

Attachment: patch with comments addressed

Comment 30

[Tanvi Vyas[:tanvi]]

Pushed Gavin's new patch + the patch to turn on mixed content blocking to try:
https://tbpl.mozilla.org/?tree=Try&rev=fb2c9f53efe6

Comment 31

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5b662231b8

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a5b662231b8