731865 - Clicking "Allow popups from site" should also show popup(s) that were blocked

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1. Go to a site that opens a blocked popup, like http://popuptest.com/popuptest5.html
2. The "Firefox prevented this site from opening a popup" infobar appears.
3. From the infobar menu, choose "Allow popups for <site>"

Actual results: The infobar disappears.

Expected results: The infobar disappears and the blocked popup appears.

This was previous reported as bug 262363 which was resolved WORKSFORME.  However, the workaround from bug 262363 comment 1 no longer work because the infobar disappears before you have a chance to choose the desired option from the menu.  On many sites you can work around this bug by reloading the page or repeating your last action, but on other sites this does not work (bug 356377).

As a workaround, you can choose "Show <popup>" from the menu *before* choosing "Allow popups..."  However, nothing in the menu makes it obvious that you must do things in this order.

Comment 1

[:Margaret Leibovic]

Attachment: WIP

This gets the job done, but this adds some redundant code, so there might be a cleaner solution.

Also, we need a test :)

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This uses the same approach as Margaret's WIP patch, updated to current m-c, and with most of the redundant code factored out.

I'm working on a test for this next.

Comment 3

[Dão Gottwald [:dao]]

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

>+  isUsablePopupURI: function(aPopupURI, aCurrentURI)
>+  {
>+    // popupWindowURI will be null if the file picker popup is blocked.

s/popupWindowURI/aPopupURI/

>+    // xxxdz this should make the option say "Show file picker" and do it (Bug 590306)
>+    if (!aPopupURI)
>+      return false;
>+
>+    var popupURIspec = aPopupURI.spec;
>+
>+    // Sometimes the popup URI that we get back from the pageReport
>+    // isn't useful (for instance, netscape.com's popup URI ends up
>+    // being "http://www.netscape.com", which isn't really the URI of
>+    // the popup they're trying to show).  This isn't going to be
>+    // useful to the user, so we won't create a menu item for it.
>+    if (popupURIspec == "" || popupURIspec == "about:blank" ||

aPopupURI.spec can be an empty string?

>+  showAllBlockedPopups: function (aBrowser)
>+  {
>+    if (!aBrowser.pageReport)
>+      return;
>+
>+    for (let report of aBrowser.pageReport) {
>+      if (!this.isUsablePopupURI(report.popupWindowURI, aBrowser.currentURI))
>+        continue;
>+
>+      let popupURIspec = report.popupWindowURI.spec;
>+
>+      let dwi = report.requestingWindow;
>+      if (!dwi || dwi.document != report.requestingDocument)
>+        continue;
>+
>+      dwi.open(popupURIspec, report.popupWindowName, report.popupWindowFeatures);
>+    }

Could this share code with the showBlockedPopup method?

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Thanks for the review!

(In reply to Dão Gottwald [:dao] from comment #3)
> s/popupWindowURI/aPopupURI/

Fixed.

> aPopupURI.spec can be an empty string?

Umm... I don't think so?  It looks like the callers either pass a nullptr or resolve relative URIs.  (Is it even possible to construct an nsIURI with an empty spec?)  I'll go ahead and remove that check.

> >+      if (!dwi || dwi.document != report.requestingDocument)
> >+        continue;
> >+      dwi.open(popupURIspec, report.popupWindowName, report.popupWindowFeatures);
> 
> Could this share code with the showBlockedPopup method?

Done.

(I'm still working on the test, which I'll attach as a separate patch.  I'll wait until that's finished before landing anything.)

Comment 5

[Jim Mathies [:jimm]]

Sorry, this bitrotted. When you get a chance can you post a refresh so I can take this for a spin?

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

Rebased onto current mozilla-central.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: patch v4

Now with an automated test.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch v5

Removed some old cruft.  Sorry for the spam.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8491163 [details] [diff] [review]
patch v5

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

This test is failing because waitForNewTab seems to have gone missing from head.js. FWIW though manually testing shows this is working, and I don't see any issues with the core bits.

0 INFO *** Start BrowserChrome Test Results ***
1 INFO checking window state
2 INFO TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js
3 INFO Wait tab event: load
4 INFO Tab event received: load
5 INFO Waiting for popupshown
6 INFO Saw popupshown
7 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js | Blocked popup menu shown
8 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js | Two popups were blocked
*************************
A coding exception was thrown and uncaught in a Task.
Full message: ReferenceError: waitForNewTab is not defined
Full stack: test/<@chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js:38:9
test@chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js:9:3
Tester_execTest@chrome://mochikit/content/browser-test.js:659:9
Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:556:7
*************************
9 INFO TEST-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js | A promise chain failed to handle a rejection

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: patch v6

Rebased onto master to fix test failures.

Comment 12

[Matt Brubeck (:mbrubeck)]

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

Comment 13

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

Comment on attachment 8492393 [details] [diff] [review]
patch v6

Sorry for the review lag, Matt - trying to redirect this to someone who will have more cycles to dive into this largely un-owned code.

Comment 14

[Matt Brubeck (:mbrubeck)]

No problem.  Six more days to fix this by the ten-year anniversary of bug 262363. :)

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8492393 [details] [diff] [review]
patch v6

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

The code looks good, r- for the test e10s issue.

::: browser/base/content/test/general/browser.ini
@@ +364,5 @@
>  [browser_plainTextLinks.js]
>  skip-if = e10s # Bug ?????? - test directly manipulates content (creates and fetches elements directly from content document)
>  [browser_popupUI.js]
>  skip-if = buildapp == 'mulet' || e10s # Bug ?????? - test directly manipulates content (tries to get a popup element directly from content)
> +[browser_popup_blocker.js]

Your test fails with e10s (ie. when running |./mach mochitest-browser --e10s browser/base/content/test/general/browser_popup_blocker.js|).

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js | Popup a - Got about:blank, expected data:text/plain,a
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popup_blocker.js | Popup b - Got about:blank, expected data:text/plain,b

Please either fix it, or skip it for e10s (in which case you'll want to file a bug blocking bug 984139 to fix it later).

::: browser/base/content/test/general/browser_popup_blocker.js
@@ +16,5 @@
> +
> +    // Wait for the popup-blocked notification.
> +    let notification;
> +    yield promiseWaitForCondition(() =>
> +        notification = gBrowser.getNotificationBox().getNotificationWithValue("popup-blocked"));

nit: is the 4-space indent intentional here?

@@ +44,5 @@
> +      popupTabs.push(event.target);
> +    }
> +    gBrowser.tabContainer.addEventListener("TabOpen", onTabOpen);
> +    yield promiseWaitForCondition(() => popupTabs.length == 2);
> +    gBrowser.removeEventListener("TabOPen", onTabOpen);

"TabOPen" typo here.

Are you intentionally adding the listener on gBrowser.tabContainer and removing it on gBrowser?

::: browser/base/content/test/general/popup_blocker.html
@@ +5,5 @@
> +    <title>Page creating two popups</title>
> +  </head>
> +  <body>
> +    <script type="text/javascript">
> +      window.open("data:text/plain,a", "a");

You can make this data:text/plain;charset=utf-8,a to avoid this warning cluttering the test logs:
JavaScript Error: "The character encoding of the plain text document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the file needs to be declared in the transfer protocol or file needs to use a byte order mark as an encoding signature."

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: patch v7

Thanks for the review!  This revision contains minor test changes that fix all the issues from comment 15.  (The non-test code is unchanged.)

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8496082 [details] [diff] [review]
patch v7

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

Thanks!

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/5f00f4077a53

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f00f4077a53