Tanvi Vyas[:tanvi] Assignee
	Description • 11 years ago

We got a report in bug 844556 comments 67-69 for a page where Mixed Content is blocked by the MCB but the Shield Icon is missing.  This occurs on a university page that requires login and I have a test account.  Note that this means that there is no way for the user to "disable protection" and load the content.

I can reproduce this on various versions of Firefox and the most recent Nightly.  Note that if I open a new tab and then return to the university webpage tab, the shield shows up as it should.

When you go to the specific page, a youtube HTTP iframe attempts to load.  MCB blocks the load, reports the blocked load to webconsole and properly updates the security state.  Then browser.js reads the security state and calls PopupNotifications.show()
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6618

PopupNotifications.show() then calls PopupNotifications._update:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#275

This is when things get a little confusing.  While reproducing, I've come across two scenarios:
1) PopupNotifications.show continues to execute and _updates is called later.  When it is called, the notifications have disappeared:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#615

and we end up here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#654

2) _update is called within PopupNotifications.show.  The notifications are found.  But then _update is called again with no notifications found and the UI has no shield.


The last/second call to _update is always after a terminal warning:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /home/mozilla/dev/mozilla-central/content/base/src/nsFrameLoader.cpp, line 510
++DOMWINDOW == 35 (0x7fc9052a88b8) [serial = 40] [outer = 0x7fc904d830b8]

Which is a return value for a LoadURI - http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#510

Do any of the folks I've cc'ed have a clue as to what might be going on here?  I can't reproduce this on my own test cases (or any of the other websites that have mixed content).  Perhaps it is something specific that this site is doing that is causing the nsFrameLoader warning and somehow interfering with the call to _updates.  Any help/ideas/thoughts would be helpful.

Thanks!

	Tanvi Vyas[:tanvi] Assignee
	Comment 1 • 11 years ago

I did some more debugging and this is what I found.

When you visit the University page with mixed content, a youtube HTTP iframe attempts to load.  MCB blocks the load, reports the blocked load to webconsole and properly updates the security state.  Then browser.js reads the security state and calls PopupNotifications.show().  PopupNotifications.show() then calls PopupNotifications._update.  _update finds the notifications and appears to go through the correct code path to show the notification.

However, I never see the notification because right after PopupNotifications.show() completes, we get a terminal warning from nsFrameLoader.cpp[1] followed by a call to onLocationChange in browser.js.  I'm not sure why onLocationChange() gets called, because the top level url is the same.  Can onLocationChange get called when a frame's location is changed?  I tried testing this with a meta-refresh in a frame, but that doesn't prompt an onLocationChange (https://people.mozilla.org/~tvyas/mixediframe_locationchange.html).

browers.js' onLocationChange properly gets the notification via _getNotificationsForBrowser.  Then it does some notification filtering based on persistence - http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#320.  This is where the Mixed Content Blocker notification disappears.  We don't want to set persistWhileVisible, since MCB is page specific and shouldn't persist across true location changes.  But since we don't set this and since the top level domain doesn't change (and doesn't seem to be doing a reload either), the notification disappears in this block.  When I comment out this block of code, the MCB notification appears (but of course that breaks lots of other things).

I don't want to set persistWhileVisible on the MCB notification (because that would mean navigating from an mixed content site to a non-mixed content site would propagate the shield when it shouldn't).  I need to dig further into what is causing the onLocationChange() call.  Is this something that is specific to this webpage and something odd it might be doing?  Or could this issue exist elsewhere as well?  Does the terminal warning have anything to do with it[1]?  Should we be calling onSecurityChange() in onLocationChange() so that PopupNotifications.show() is called again?


[1] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /home/mozilla/dev/mozilla-central/content/base/src/nsFrameLoader.cpp, line 510
++DOMWINDOW == 35 (0x7fc9052a88b8) [serial = 40] [outer = 0x7fc904d830b8]

	Tanvi Vyas[:tanvi] Assignee
	Comment 2 • 11 years ago

I wrote a test case the reproduces this issue:
https://people.mozilla.org/~tvyas/missing_shield4.html

It is an HTTPS page with mixed script and an HTTPS iframe.  When the HTTPS iframe loads, we use javascript in the iframe to click a link in the frame and navigate it to another HTTPS location.  When this happens, onLocationChange() is called and the Mixed Content Blocker notification disappears (you see it for a second and then it's gone).  The top-level page has not changed, so we want to persist the notification.  If the top-level page had changed, we would not want to persist the navigation.

Any thoughts on how I should go about fixing this?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 11 years ago

Putting the "locationChange()" call at:
http://hg.mozilla.org/mozilla-central/annotate/59beb1868522/browser/base/content/browser.js#l4229

in the "isTopLevel" block would probably fix it. That has wider implications, and there may be some relevant bug history (we've changed that code not too long ago, in bug 839445 and related bugs).

	Tanvi Vyas[:tanvi] Assignee
	Comment 4 • 11 years ago

We could fix this by moving the popupnotification code inside this isTopLevel block:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4244

What would that break?  I'm going to propose a patch and push it to try to see what happens.

	Tanvi Vyas[:tanvi] Assignee
	Comment 5 • 11 years ago

Thanks Gavin!  I will read the bug and the related bugs.

Here is a proposed patch.  Will push to try shortly.

	Tanvi Vyas[:tanvi] Assignee
	Comment 6 • 11 years ago

Push to try: https://tbpl.mozilla.org/?tree=Try&rev=180be380ab5d

	Tanvi Vyas[:tanvi] Assignee
	Comment 7 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #6)
> Push to try: https://tbpl.mozilla.org/?tree=Try&rev=180be380ab5d
There are two main tests that are failing on try:
/toolkit/components/passwordmgr/test/test_notifications_popup.html
/browser/base/content/test/general/browser_popupNotifions.js

The first passes locally after a recent pull, so maybe I just needed a more up-to-date Firefox.  Will push to try again for just mochitest-5 on one platform to see what happens: https://tbpl.mozilla.org/?tree=Try&rev=109f1d526e53

The second fails because of a test that checks the very thing that I'm changing with my patch:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_popupNotification.js#816

Test #28 in popupNotifications checks to see if the notification disappears after an iframe changes location (while my patch prevents the notification from disappearing).  Are there any notifications where this behavior would be desirable?  After some digging, I found some information about this here: https://bugzilla.mozilla.org/show_bug.cgi?id=839516#c22 and comment 24.  Dao and Gavin - are you okay with changing this behavior?  It creates the opposite problem than the one described in the comments - notifications may become stale.  But it fixes this specific mixed content blocker problem - where a notification disappears because of an unrelated subdocument location change.

Consider a case where a page has a hidden plugin that is click to play.  The page has a framed document (maybe an ad or something unrelated to the functionality of the page and the plugin) that changes location.  After the change in location, the plugin doorhanger would disappear and the user would not be able to play the content.  This patch would also fix this situation.

However, consider a case where MCB blocks an HTTP iframe in an HTTPS page.  The HTTPS page could change the location of the subframe to an HTTPS location after some timeout.  If the HTTP frame was the only piece of Mixed Content on the HTTPS page, then the notification would become stale and would exist there for no reason.  Personally, I'd rather have a stale notification that a user could choose to ignore instead of causing a page to be permanently broken.

Thoughts?

Thanks!

	Dão Gottwald [:dao]
	Comment 8 • 11 years ago

Comment on attachment 807482 [details] [diff] [review]
Bug915951-09-19-13.patch

>   onLocationChange: function (aBrowser, aWebProgress, aRequest, aLocationURI,
>                               aFlags) {
>     // Filter out location changes caused by anchor navigation
>     // or history.push/pop/replaceState.
>     if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT)
>       return;
> 
>-    // Only need to call locationChange if the PopupNotifications object
>-    // for this window has already been initialized (i.e. its getter no
>-    // longer exists)
>-    if (!Object.getOwnPropertyDescriptor(window, "PopupNotifications").get)
>-      PopupNotifications.locationChange(aBrowser);
>-
>-    gBrowser.getNotificationBox(aBrowser).removeTransientNotifications();
>-
>     // Filter out location changes in sub documents.
>     if (aWebProgress.isTopLevel) {
>+      // Only need to call locationChange if the PopupNotifications object
>+      // for this window has already been initialized (i.e. its getter no
>+      // longer exists)
>+      if (!Object.getOwnPropertyDescriptor(window, "PopupNotifications").get)
>+        PopupNotifications.locationChange(aBrowser);
>+
>+      gBrowser.getNotificationBox(aBrowser).removeTransientNotifications();
>+
>       FullZoom.onLocationChange(aLocationURI, false, aBrowser);
>     }

At this point, it might be better to stop indenting this whole block and have it preceded by another early return:

     // Filter out location changes in sub documents.
     if (aWebProgress.isTopLevel)
       return;

	Tanvi Vyas[:tanvi] Assignee
	Comment 9 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #7)
>/toolkit/components/passwordmgr/test/test_notifications_popup.html

> The first passes locally after a recent pull, so maybe I just needed a more
> up-to-date Firefox.  Will push to try again for just mochitest-5 on one
> platform to see what happens:
> https://tbpl.mozilla.org/?tree=Try&rev=109f1d526e53

Running this individual test succeeds.  But the test fails on the try server or when you run mochitest-5 locally, because you end up with a bunch of popup notifications that were caused by other tests and never dismissed because the top-level page in mochitest never navigates - only the testing iframe changes location - 
https://people.mozilla.org/~tvyas/popup-notification-test-failing.png

Perhaps we should clear all notifications at the start of this test (and the start of other popup notification tests).  What do you think of that idea?  And is there a method to remove all notifications?

Thanks!

	Dão Gottwald [:dao]
	Comment 10 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #9)
> Perhaps we should clear all notifications at the start of this test (and the
> start of other popup notification tests).  What do you think of that idea? 
> And is there a method to remove all notifications?

I don't think so. PopupNotifications.jsm has a private _getNotificationsForBrowser method.

	Tanvi Vyas[:tanvi] Assignee
	Comment 11 • 11 years ago

Per dao's comment 8, I've simplified the patch to return early in the subdocument case:


dao- do you want to re-review?

	Tanvi Vyas[:tanvi] Assignee
	Comment 12 • 11 years ago

Tried to update the tests.

* For https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_notifications.html?force=1, I tried to add a cleanUpPopupNotifications() function in notifications_common.js.

This only half worked, meaning only half the notifications in mochitest-5 were removed.  Compared the previous screenshot with the new screenshot - 
Previous has 4 notifications - https://people.mozilla.org/~tvyas/too-many-popup-notifications.png
After this patch, you see 2 notifications - https://people.mozilla.org/~tvyas/too-many-popup-notifications2.png

Perhaps I need to use the _getNotificationsForBrowser method in notifications_common.js.  But how do I get the browser to pass it in?  To use the private method and to get the browser, don't I need a browser chrome test?

* For https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_popupNotification.js#816, I replaced the event in test 28 from "removed" to "showing" and test is passing.  But now when running the test, test 29 and 30 have multiple messages in the actual doorhanger (messages from tests 28, 29, and 30).  And the total tests that have succeeded is 271 instead of 294.  Hence, there is more to it than the changes I've made.  Perhaps the "showing" event is invoked multiple times and hence executeSoon() is called too many times.

Matt, since you are familiar with the password manager and popup notification tests, if you could provide some pointers or feedback on these tests that would be very helpful.  Thanks!

	Tanvi Vyas[:tanvi] Assignee
	Comment 13 • 11 years ago

Comment on attachment 812392 [details] [diff] [review]
Bug915951-tests-09-30-13.patch

Since Matt is out, feedback? to dao instead.  Dao, can you take a look at comment 12 and provide some advice on how to update these tests.  Thanks for your help!

	Matthew N. [:MattN]
	Comment 14 • 11 years ago

Comment on attachment 812392 [details] [diff] [review]
Bug915951-tests-09-30-13.patch

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

(In reply to Tanvi Vyas [:tanvi] from comment #12)
Some info from our in-person discussion.

> * For
> https://mxr.mozilla.org/mozilla-central/source/toolkit/components/
> passwordmgr/test/test_notifications.html?force=1, I tried to add a
> cleanUpPopupNotifications() function in notifications_common.js.
> 
> This only half worked, meaning only half the notifications in mochitest-5
> were removed.  Compared the previous screenshot with the new screenshot - 
> Previous has 4 notifications -
> https://people.mozilla.org/~tvyas/too-many-popup-notifications.png
> After this patch, you see 2 notifications -
> https://people.mozilla.org/~tvyas/too-many-popup-notifications2.png
> 
> Perhaps I need to use the _getNotificationsForBrowser method in
> notifications_common.js.  But how do I get the browser to pass it in?  To
> use the private method and to get the browser, don't I need a browser chrome
> test?

It may be okay to use the private method in this test helper.

> * For
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/browser_popupNotification.js#816, I replaced the event in test 28
> from "removed" to "showing" and test is passing.  But now when running the
> test, test 29 and 30 have multiple messages in the actual doorhanger
> (messages from tests 28, 29, and 30).  And the total tests that have
> succeeded is 271 instead of 294.  Hence, there is more to it than the
> changes I've made.  Perhaps the "showing" event is invoked multiple times
> and hence executeSoon() is called too many times.

As discussed in-person, I think you can simply call remove on the notification after you verify it is showing.

::: browser/base/content/test/general/browser_popupNotification.js
@@ +822,1 @@
>              ok(true, "Notification removed in background tab after reloading");

Update this text

	Tanvi Vyas[:tanvi] Assignee
	Comment 15 • 11 years ago

This creates a cleanUpPopupNotifications function in notfications_common.js that is called at the start of a test_notifications_popup.html.

I still am working on the fix to browser_popupNotifications.js.

	Matthew N. [:MattN]
	Comment 16 • 11 years ago

Comment on attachment 815581 [details] [diff] [review]
Fix to test_notifications_popup.html v1

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

This is not ideal IMO as I think we should fix tests so they don't leave the browser with PopupNotifications hanging around. This is essentially a band-aid to fix this one affected test but a new test could come along in another component (without access to passwordmgr's notification_common.js) and have to also cleanup the leftover notifications.

I'll leave it up to you to decide whether to land this for now. We should at least have a follow-up to remove this helper and have tests running before this one do their own PopupNotification cleanup before subsequent tests are run.

::: toolkit/components/passwordmgr/test/notification_common.js
@@ +87,5 @@
> + * left behind from previous tests.
> + */
> +function cleanUpPopupNotifications() {
> +  var container = getPopupNotifications(window.top);
> +  ok(true, "is popup panel open? " + container.isPanelOpen);

I don't think this line is relevant

@@ +89,5 @@
> +function cleanUpPopupNotifications() {
> +  var container = getPopupNotifications(window.top);
> +  ok(true, "is popup panel open? " + container.isPanelOpen);
> +  var notes = container._currentNotifications;
> +  ok(true, "Found " + notes.length + " popup notifications.");

Change this from "ok(true, " to "info(" and update the message to indicate it is removing N leftover popup notifications.

	Tanvi Vyas[:tanvi] Assignee
	Comment 17 • 11 years ago

Updated per Matt's comment 16.  Carrying over r+.

	Tanvi Vyas[:tanvi] Assignee
	Comment 18 • 11 years ago

Comment on attachment 820530 [details] [diff] [review]
Fix to test_notifications_popup.html v2

This includes an incomplete patch to another test.  Obsoleting.

	Tanvi Vyas[:tanvi] Assignee
	Comment 19 • 11 years ago

I have been stuck on updating Test 28 in https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_popupNotification.js#864 for sometime.

This test originally tested that an iframe location change would remove a popup notification.  It did this by waiting for the "removed" event.  The "removed" event was triggered by TabsProgressListener onLocationChange() in browser.js:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4190

With the changed behavior, we want to test that it does not remove a popupNotification.  From the event callback options we have (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_popupNotification.js#134) the "showing" event would make sense.  However, the popupNotification is already "showing" before the location change.  I don't know how to test for the showing event only AFTER the location change.  Is there anyway to detect that the onLocationChange() function was called and only then check for the "showing" event?  I've tried various methods, including checking the iframes source has changed or adding and waiting for a load listener, but onLocationChange() seems to be called after these (perhaps asynchronously).

Alternatively, I could wait for a "removed" event for some seconds, and then consider the test successful if we never get a removed event.  This is not ideal by any means.

Any help or suggestions would be greatly appreciated.  Thanks!

	Dão Gottwald [:dao]
	Comment 20 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #19)
> Is there anyway to detect that the onLocationChange()
> function was called and only then check for the "showing" event?  I've tried
> various methods, including checking the iframes source has changed or adding
> and waiting for a load listener, but onLocationChange() seems to be called
> after these (perhaps asynchronously).

See https://developer.mozilla.org/en-US/docs/Listening_to_events_on_all_tabs

	Tanvi Vyas[:tanvi] Assignee
	Comment 21 • 11 years ago

(In reply to Dão Gottwald [:dao] from comment #20)
> See https://developer.mozilla.org/en-US/docs/Listening_to_events_on_all_tabs

Thanks Dao for the suggestion!  How about something like this:

http://pastebin.mozilla.org/3377929

This passes with the patch in this bug and fails without it.  Notice that I use the anchor id to test if the notification is still shown (similar to test 27).  I do this to avoid looking for two events (first for TabProgressListener's onLocationChange, then for a showing event for the notification).  What do you think?

Alternatively, I could try to the Test 14, where I check that the onHidden event isn't triggered until an onLocationChange.

	Tanvi Vyas[:tanvi] Assignee
	Comment 22 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #21)
> Alternatively, I could try to the Test 14, where I check that the onHidden
> event isn't triggered until an onLocationChange.

Second method: http://pastebin.mozilla.org/3378271

Is the anchor creation and checking code over kill here?  Should I remove lines 8-14 and 28?

	Matthew N. [:MattN]
	Comment 24 • 11 years ago

This is a combination of attachment 824419 [details] [diff] [review] by bsmedberg and https://pastebin.mozilla.org/3378271 by Tanvi that avoid executeSoon. Credit is given in the commit message.

I confirmed this fails without the browser.js fix.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c79275a58348

	Dão Gottwald [:dao]
	Comment 26 • 11 years ago

Comment on attachment 824556 [details] [diff] [review]
Fix browser_popupNotification.js v.1

>+      let progressListener = {
>+        onLocationChange: function onLocationChange(aBrowser) {
>+          let notification = PopupNotifications.getNotification(self.notifyObj.id,
>+                                                                self.notifyObj.browser);
>+          ok(notification != null, "Test 28: Notification remained when subframe navigated");
>+          self.notifyObj.options.eventCallback = undefined;
>+
>+          notification.remove();
>+          gBrowser.removeTabsProgressListener(progressListener);
>+        },
>+      };
>+
>+      info("Test 28: Adding progress listener and performing navigation");
>+      gBrowser.addTabsProgressListener(progressListener);

You should sanity-check aBrowser or use gBrowser.addProgressListener if there's no tab switching involved and you don't care about location changes in background tabs.

	Benjamin Smedberg
	Comment 27 • 11 years ago

Comment on attachment 824556 [details] [diff] [review]
Fix browser_popupNotification.js v.1

dao's right, this should use addProgressListener not addTabsProgressListener. r=me with that change

While you're at it also change the test 28 heading? It should say "location change in embeded frame should not remove notification"

	Tanvi Vyas[:tanvi] Assignee
	Comment 28 • 11 years ago

version 2 and 2.1 of the fix to test_notifications_popup are old versions that don't work.  Uploading the working version and will push to try.  Sorry, should have uploaded it earlier.

It addresses Matt's comments from Comment 16.

	Tanvi Vyas[:tanvi] Assignee
	Comment 29 • 11 years ago

Updated per comment 27.

Push to try:
https://tbpl.mozilla.org/?tree=Try&rev=5d328ce6b7c4

	Tanvi Vyas[:tanvi] Assignee
	Comment 30 • 11 years ago

Turns out using addProgressListener won't work.  The test succeeds with or without the code change.  addProgressListener's onLocationChange is triggered too soon.  We need to wait for addTabsProgressListener's onLocationChange.

 0:14.09 XULBrowserWindow onLocationChange
 0:14.09 onLocationChange event triggered
 0:14.09 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popupNotification.js | Test 28: Notification remained when subframe navigated
 0:14.11 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popupNotification.js | [Test #28] popup hidden (0 hides remaining)
 0:14.11 addTabsProgressListener onLocationChange

	Tanvi Vyas[:tanvi] Assignee
	Comment 31 • 11 years ago

New patch that uses addTabsProgressListener and compares aBrowser to gBrowser.selectedBrowser to make sure they are the same.

Test 28 fails without the code change.  Succeeds with it.

	Benjamin Smedberg
	Comment 32 • 11 years ago

Comment on attachment 824836 [details] [diff] [review]
Fix browser_popupNotification.js v.3

+          is(aBrowser, gBrowser.selectedBrowser, "Test 28: Using the correct browser");

If that test ever randomly fails, we should instead make it just a check;

if (aBrowser != gBrowser.selectedBrowser) {
  return;
}

	Tanvi Vyas[:tanvi] Assignee
	Comment 33 • 11 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #32)
> Comment on attachment 824836 [details] [diff] [review]
> Fix browser_popupNotification.js v.3
> 
> +          is(aBrowser, gBrowser.selectedBrowser, "Test 28: Using the
> correct browser");
> 
> If that test ever randomly fails, we should instead make it just a check;
> 
> if (aBrowser != gBrowser.selectedBrowser) {
>   return;
> }

Done.  Carrying over r+.

This change shouldn't need a new push to try.  Although we are already seeing issues with the previous push.  We are getting an error in test_prompt.html in mochitest-5 on Mac.  Mochitest-5 on linux (which I had tested locally) passes on try because test_prompt.html isn't run on linux.

	Tanvi Vyas[:tanvi] Assignee
	Comment 34 • 11 years ago

Still investigating the test_prompt.html failure.  A few notes.

We believe test_prompt.html is failing because of a MCB doorhanger that should not be there.  The call to cleanUpPopupNotifications in test_notifications_popup.html should have removed it.  Neither test_notifications_popup.html or test_prompt.html seem to introduce Mixed Content, so why is the notification still there?

If you call the cleanUpPopupNotifications() in test_prompt.html, mochitest-5 will succeed:
https://tbpl.mozilla.org/?tree=Try&rev=d069dd1355d1
INFO TEST-INFO | /tests/toolkit/components/passwordmgr/test/test_prompt.html | true - Removing 1 popup notifications.

When you remove the browser.js code change and just update the tests (specifically, test_notifications_popup.html), mochitest-5 succeeds.

	Matthew N. [:MattN]
	Comment 35 • 11 years ago

Comment on attachment 824737 [details] [diff] [review]
Fix to test_notifications_popup.html v3

I did the investigation I asked for in comment 16 and found that mixed content tests were unintentionally leaving a mixed content doorhanger around. I have a patch in bug 933085 to fix this so as long as there aren't other tests leaving doorhangers around in the try push, this band-aid won't be needed.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=527a637da30b

	Tanvi Vyas[:tanvi] Assignee
	Comment 36 • 11 years ago

Comment on attachment 824737 [details] [diff] [review]
Fix to test_notifications_popup.html v3

Un-obsoleting for now.  Bug 933085 will help but won't fix this test completely.  There are 3 popupnotifications that are lurking around from previosu tests in test_notifications_popup.html (https://people.mozilla.org/~tvyas/too-many-popup-notifications.png).  A hidden plugin notification, a plugin notification, and a MCB notification.  But 933085 will remove the MCB notification, but the other two will still be around.  In order to fix that, we have to identify the tests that introduce them and clean them up at the end of those tests.  Matt was looking into that earlier today.

	Tanvi Vyas[:tanvi] Assignee
	Comment 37 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #34)

> We believe test_prompt.html is failing because of a MCB doorhanger that
> should not be there.  The call to cleanUpPopupNotifications in
> test_notifications_popup.html should have removed it.  Neither
> test_notifications_popup.html or test_prompt.html seem to introduce Mixed
> Content, so why is the notification still there?

With the fix to test_notifications_popup.html, we remove all existing doorhangers. test_notifications_popup.html invokes a call to an nsSecureBrowserUIImpl function that checks docShell->GetHasMixedActiveContentBlocked() (https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#309).  Since Mixed Active Content has been blocked on this docshell before (in the /security tests), the flag for GetHasMixedActiveContentBlocked is true and we set nsIWebProgressListener::STATE_BLOCKED_MIXED_ACTIVE_CONTENT.  By setting the security state, a call to browser.js' onSecurityChange() results in a call to showMixedContentDoorhanger() and hence re-introduces the doorhanger we removed earlier:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6499

Hence, this is expected behavior.  Even though the doorhanger was removed by the test (or by a user) it shows up again when onSecurityChange is called.  You can test this locally by visiting https://people.mozilla.org/~tvyas/mixedcontent.html, clicking the doorhanger, clicking keep blocking.  The doorhanger will disappear.  Visit another tab and then come back to this tab.  The doorhanger is back because onSecurityChange was called and hence showMixedContentDoorhanger() was called.

For mochitest-5, test_notifications_popup.html, and test_prompt.html we can solve this problem by the change to the /security tests Matt made in Bug 933085.  With that change, content that we were initially blocking is now allowed.  Hence the state is set to nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT and hence showMixedContentDoorhanger() is never called.  If there were a mochitest-X that had Mixed Content Blocking tests that required invoking the MCB doorhanger followed by popup notificatation tests that required starting with a clean state, there would be no way to fix this.


Here is some log information from running test_notifications_popup.html: http://pastebin.mozilla.org/3387112
(There are still some parts of this that are puzzling to me; like why sometimes the security state is 4 and other times it is 20.  Perhaps it is 4 when onSecurityChange is being called on the iframe and not the top level page.  Would have to do more debugging to determine that.)

	Tanvi Vyas[:tanvi] Assignee
	Comment 38 • 11 years ago

Current state for this bug:

* All 3 patches are currently still valid and necessary.  If we can identify and remove the popups for plugins created in previous mochitest-5 tests, then we could potentially remove "Fix to test_notifications_popup.html v3". 

* We also need the patch to bug 933085 to pass test_prompt.html OR you need to call cleanUpPopupNotifications() in test_prompt.html (https://hg.mozilla.org/try/rev/72a63a76d6a4, https://tbpl.mozilla.org/?tree=Try&rev=d069dd1355d1)

* There is a consistent Win 7 only failure in mochitest-1 -> 
ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug388794.html | Unexpected href for frame test6 - got data:text/html,?x=5&y=6, expected data:text/html,?x=5&y=5

https://tbpl.mozilla.org/?tree=Try&rev=5d328ce6b7c4
https://tbpl.mozilla.org/?tree=Try&rev=527a637da30b
https://tbpl.mozilla.org/?tree=Try&rev=c79275a58348

I have not investigated this test at all.

	Matthew N. [:MattN]
	Comment 39 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #38)
> * All 3 patches are currently still valid and necessary.  If we can identify
> and remove the popups for plugins created in previous mochitest-5 tests,
> then we could potentially remove "Fix to test_notifications_popup.html v3". 

These failures highlight real issues in the wild for users so ideally we would remove the icon when it's stale (see comment 7) but apparently we want it in some cases (e.g. bug 745187 for CTP). Perhaps we can fix this for the mixed content case? I also didn't see a pref to flip to avoid the CTP doorhanger for the tests. I would have though disabling plugins.click_to_play would do so but that doesn't seem to work or maybe requires a restart but we could possibly fix these things.

> * We also need the patch to bug 933085 to pass test_prompt.html OR you need
> to call cleanUpPopupNotifications() in test_prompt.html
> (https://hg.mozilla.org/try/rev/72a63a76d6a4,
> https://tbpl.mozilla.org/?tree=Try&rev=d069dd1355d1)

At this point I think we should just use cleanUpPopupNotifications in both passwordmgr files and figure out how to have the tests cleanup better in the future to handle this new behaviour. r=me on adding cleanUpPopupNotifications to the beginning of the additional test.

> * There is a consistent Win 7 only failure in mochitest-1 -> 
> ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug388794.html | Unexpected href for
> frame test6 - got data:text/html,?x=5&y=6, expected data:text/html,?x=5&y=5
> 
> https://tbpl.mozilla.org/?tree=Try&rev=5d328ce6b7c4
> https://tbpl.mozilla.org/?tree=Try&rev=527a637da30b
> https://tbpl.mozilla.org/?tree=Try&rev=c79275a58348
> 
> I have not investigated this test at all.

I have no idea how these patches could have caused this. I'm hoping we can tweak the test again to get it to round differently (see http://hg.mozilla.org/mozilla-central/rev/3db8899c075c ). Another option is to disable the test on Win7-only but that would probably require more discussion and investigation.

	Matthew N. [:MattN]
	Comment 40 • 11 years ago

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #39)
> (In reply to Tanvi Vyas [:tanvi] from comment #38)
> > * There is a consistent Win 7 only failure in mochitest-1 -> 
> > ERROR TEST-UNEXPECTED-FAIL |
> > /tests/content/html/content/test/test_bug388794.html | Unexpected href for
> > frame test6 - got data:text/html,?x=5&y=6, expected data:text/html,?x=5&y=5

Note that this test fails on Windoes 7 in that same way locally without any patches (on the UX branch).

	Benjamin Smedberg
	Comment 41 • 11 years ago

The mochiteset failures have a fairly straightforward cause: the mochitest runner itself is a page which loads tests in subframes. Because the top mochitest frame never navigates, no notifications are going to be ever dismissed.

Arguably the notification tests shouldn't be plain mochitests (the specialpowers goop is going to break with e10s anyway), but for now we can just remove all existing notifications at the beginning of each test that uses notification_common.js

	Tanvi Vyas[:tanvi] Assignee
	Comment 42 • 11 years ago

Adds the clean up function to the top of browser_prompt.html.  Matt, can you check that the call is in the ideal place?

	Tanvi Vyas[:tanvi] Assignee
	Comment 43 • 11 years ago

Given the last few comments, I think that the 4 patches in this bug are sufficient to fix the issue.  But, what do you want to do about the Win 7 failure?  I don't see it on any recent pushes to central.

	Tanvi Vyas[:tanvi] Assignee
	Comment 44 • 11 years ago

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #39)
> I also didn't see a pref to flip to avoid the CTP
> doorhanger for the tests. I would have though disabling
> plugins.click_to_play would do so but that doesn't seem to work or maybe
> requires a restart but we could possibly fix these things.
> 
Some tests may depend on the pref to be enabled (and hence the doorhanger existing).  The way to clean these up would be to remove the notification at the end of the test; not flip the pref.  Flipping the pref worked for the MCB tests in /security because for those specific tests we actually wanted to the feature disabled.

	Benjamin Smedberg
	Comment 45 • 11 years ago

test fixes for password manager mochitests - because mochitest runs all its tests in an iframe, it collects popup notifications from all tests and never dismisses them: remove them explicitly before each password manager test that needs to check popups

	Matthew N. [:MattN]
	Comment 46 • 11 years ago

Comment on attachment 825451 [details] [diff] [review]
This is the version which I pushed to try this morning, which removes all notification before any of the password manager tests which use notifications. This passes try, except for the win-only mochitest-1 failures in test_bug388794.html

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

The alternative is to change these tests to ignore irrelevant popup notifications and only care about password maanger ones.

	Matthew N. [:MattN]
	Comment 47 • 11 years ago

Comment on attachment 825386 [details] [diff] [review]
Fix browser_prompt.html v.1

I prefer bsmedberg's patch to do this in notification_common.js

	Matthew N. [:MattN]
	Comment 48 • 11 years ago

Comment on attachment 824737 [details] [diff] [review]
Fix to test_notifications_popup.html v3

Obsoleted by attachment 825451 [details] [diff] [review]

	Matthew N. [:MattN]
	Comment 49 • 11 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #44)
> Some tests may depend on the pref to be enabled (and hence the doorhanger
> existing).  The way to clean these up would be to remove the notification at
> the end of the test; not flip the pref.  Flipping the pref worked for the
> MCB tests in /security because for those specific tests we actually wanted
> to the feature disabled.

That may be the case for one or two but I think most of those tests intend to be testing real plugins rather than the CTP overlay. I wouldn't be surprised if a some of those tests are no longer testing what they are supposed to since CTP was enabled by default. I'll file a separate bug for this.

	Matthew N. [:MattN]
	Comment 50 • 11 years ago

Here's what I know so far about the mochitest-1 test_bug388794.html failure:
* A clean UX tip repo exhibits the failure on my Win7 desktop
* A clean UX tip repo exhibits the failure on my Win7 desktop (but I believe it worked the first time unless I was mistaken)
* Changing the test to use 4.99, 4.99 instead of 4.999 causes the test to pass locally. Technically only the one coordinate is wrong but I figured I'd keep them consistent. It's possible that this is a recent regression that only shows up in certain cases. I guess I can check that.

Try push of m-c tip to see if there are infra differences: https://tbpl.mozilla.org/?tree=Try&rev=5f1e3f3e1636 (I don't believe their is since Tanvi pointed out that this is passing for other people on try)

Try push with 4.99: https://tbpl.mozilla.org/?tree=Try&rev=c0921e3aaf99

	Matthew N. [:MattN]
	Comment 51 • 11 years ago

Thanks everyone.

https://hg.mozilla.org/integration/fx-team/rev/b75be6e8024d
https://hg.mozilla.org/integration/fx-team/rev/fe6775e8fab4
https://hg.mozilla.org/integration/fx-team/rev/33e81ac59100
https://hg.mozilla.org/integration/fx-team/rev/c5649330a242

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b75be6e8024d
https://hg.mozilla.org/mozilla-central/rev/fe6775e8fab4
https://hg.mozilla.org/mozilla-central/rev/33e81ac59100
https://hg.mozilla.org/mozilla-central/rev/c5649330a242

	Matthew N. [:MattN]
	Comment 53 • 11 years ago

Comment on attachment 812388 [details] [diff] [review]
browser.js patch

[Approval Request Comment (for all 4 patches)]
Bug caused by (feature/regressing bug #): Not exactly a regression but this became more of an issue with CTP plugins and mixed content features using notifications.

User impact if declined: Notifications (e.g. CTP and mixed content) will continue to be removed for all subframe navigations regardless of whether the subframe had anything to do with the notification's purpose. This makes it very hard or impossible to take action on the notification in certain cases. My understanding is that this fix is required to ship with CTP enabled.

Testing completed (on m-c, etc.): Landed on m-c and will be in tomorrow's Nightly.

Risk to taking this patch (and alternatives if risky): Known issue: There are times when notifications will linger when they are no longer relevant due to subframe navigations but that is considered better than not allowing the user to fix broken pages (e.g. plugins or content not working) in other cases.

The patch to what ships (browser.js) doesn't add new code, only changes whether sub-frame navigation affects notification removal and is very low risk IMO.

String or IDL/UUID changes made by this patch: none

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 54 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b75be6e8024d
https://hg.mozilla.org/mozilla-central/rev/fe6775e8fab4
https://hg.mozilla.org/mozilla-central/rev/33e81ac59100
https://hg.mozilla.org/mozilla-central/rev/c5649330a242

	Paul Silaghi, QA [:pauly]
	Comment 55 • 11 years ago

Tested on nightly 28.0a1 (2013-11-03).
The CTP doorhanger (bug 932633) looks fixed now.
Opening the testcase from here, I see the MCB doorhanger and also the "Hello People! " message displayed. Is it ok?

	Ryan VanderMeulen [:RyanVM]
	Comment 56 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d08677c7983
https://hg.mozilla.org/releases/mozilla-aurora/rev/47dad5d66614
https://hg.mozilla.org/releases/mozilla-aurora/rev/337662d08207
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c4bd25aeaed

https://hg.mozilla.org/releases/mozilla-beta/rev/a5decfb9ad78
https://hg.mozilla.org/releases/mozilla-beta/rev/d260d7976ccb
https://hg.mozilla.org/releases/mozilla-beta/rev/62d2bb402593
https://hg.mozilla.org/releases/mozilla-beta/rev/7c916ca5b8e0

	Matthew N. [:MattN]
	Comment 57 • 11 years ago

(In reply to Paul Silaghi, QA [:pauly] from comment #55)
> Tested on nightly 28.0a1 (2013-11-03).
> The CTP doorhanger (bug 932633) looks fixed now.
> Opening the testcase from here, I see the MCB doorhanger and also the "Hello
> People! " message displayed. Is it ok?

Yes, that is correct.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 58 • 11 years ago

(In reply to Tanvi Vyas [:tanvi - on leave] from comment #30)
> Turns out using addProgressListener won't work.  The test succeeds with or
> without the code change.  addProgressListener's onLocationChange is
> triggered too soon.  We need to wait for addTabsProgressListener's
> onLocationChange.

That doesn't really make any sense. It sounds like the actual problem is that you're depending on something happening in another onLocationChange handler, in which case you should just use an executeSoon rather than relying on the fact that the tabs progress listeners are invoked after the per-browser ones.

	Paul Silaghi, QA [:pauly]
	Comment 59 • 11 years ago

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #57)
> (In reply to Paul Silaghi, QA [:pauly] from comment #55)
> > Tested on nightly 28.0a1 (2013-11-03).
> > The CTP doorhanger (bug 932633) looks fixed now.
> > Opening the testcase from here, I see the MCB doorhanger and also the "Hello
> > People! " message displayed. Is it ok?
> 
> Yes, that is correct.
Verified fixed FF 28.0a1(2013-11-04), FF 26b2 on Win 7, Mac OS X 10.8.4, Ubuntu 12.04

	Paul Silaghi, QA [:pauly]
	Comment 60 • 11 years ago

Verified fixed 27.0a2 (2013-11-05)

	Ryan VanderMeulen [:RyanVM]
	Comment 61 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a5decfb9ad78
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d260d7976ccb
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/62d2bb402593
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7c916ca5b8e0