933462 - [e10s] Popup blocker notifications don't work in e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The path is roughly as follows:

1. nsGlobalWindow fires a DOMPopupBlocked event.
2. browser.xml handles it, adds to a list of "page reports", and calls triggers a DOMUpdatePageReport event.
3. browser.js handles that event via gPopupBlockerObserver.

We'll need to handle the DOMPopupBlocked in a content script and then send a message to the parent. Hopefully the UI steps can remain the same after that. The final step of allowing popups goes though Services.perms via toggleAllowPopupsForSite. I'm not sure how that code works.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: popup-blocking

This was pretty straightforward.

Comment 2

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

Comment on attachment 8390136 [details] [diff] [review]
popup-blocking

Can we get rid of the stupid "pageReports" name while we're at it? It's a horrible old name that is meaningless.

"blockedPopup" or "blockedPopupInfo" would be better.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> Comment on attachment 8390136 [details] [diff] [review]
> popup-blocking
> 
> Can we get rid of the stupid "pageReports" name while we're at it? It's a
> horrible old name that is meaningless.
> 
> "blockedPopup" or "blockedPopupInfo" would be better.

I can change it in most places. Changing the one in browser.xml is a little trickier because we use that field in Firefox desktop, android, and metro. I think a good compromise would be to support a new name and the old name in browser.xml and start using the new name in Firefox desktop. Then we can file bugs to fix android and metro, and hopefully those teams can take care of fixing it and testing the fix (which is that part I'm reluctant to do).

Comment 4

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

That sounds great - thanks!

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: popup-blocking v2

This mostly gets rid of pageReports.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8390807 [details] [diff] [review]
popup-blocking v2

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

::: browser/base/content/browser.js
@@ +512,1 @@
>      var uri = gBrowser.currentURI;

change this to browser.currentURI ?

@@ +541,5 @@
>      var foundUsablePopupURI = false;
> +    var blockedPopups = browser.blockedPopups;
> +    if (blockedPopups) {
> +      for (let i = 0; i < blockedPopups.length; i++) {
> +        let blockedPopup = blockedPopups[i];

why not keep the for as |(let blockedPopup of blockedPopups)| ?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c129d201f96

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c129d201f96

Comment 9

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

(In reply to :Felipe Gomes from comment #6)
> why not keep the for as |(let blockedPopup of blockedPopups)| ?

(because we need "i" later down)

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, I should have been more explicit. Felipe and i talked about it on IRC.

Comment 11

[[e-mail]]

Huh, at some point between the attached patch and the checkin the type of popupWindowURI mysteriously changed from a URI to a string...

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I noticed that the version of the patch where it was a URI was broken because we were sending the URI over a message manager, which doesn't support URIs. Do you want me to change the name to popupWindowURL?

Comment 13

[u279076]

Henrik, I wonder if we should have an e10s testrun in Mozmill which runs our existing tests with e10s pref'd on to catch bugs like this? FWIW, I'd be fine if we isolated this to Nightly testruns for now.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Fx32 in bug 1024391. Due to time constraints, it will be shipping as-is in Fx31. Hopefully Fx33 will contain a proper fix for bug 1024391 while leaving this fix in place.

Comment 15

[[e-mail]]

Comment on attachment 8390807 [details] [diff] [review]
popup-blocking v2

I think I may have found a bug, but I'm commenting here because it's easier to comment on the patch this way.

>-    if (!gBrowser.pageReport.reported) {
>+    if (!gBrowser.selectedBrowser.blockedPopups.reported) {
So, you switched from pageReport to blockedPopups. Seems OK, but...

>+            this.blockedPopups = aBlockedPopups;
>+            if (aFreshPopup) {
>+              this.blockedPopups.reported = false;
... aBlockedPopups is a completely new array every time...

>-            this.pageReport.push(obj);
>-            this.pageReport.reported = false;
... while the old code merely appended on to the existing array.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The code that handles the DOMPopupBlocked event has moved to browser-content.js (onPopupBlocked there). It appends to its own array, just as the old code did. That array is then sent up to the parent periodically. The parent replaces its copy of the array with the one from the child, which is the code you pointed out. I think everything is correct there.

Comment 17

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

Removing my name from in-qa-testsuite flag for a better query.

Comment 18

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

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #13)
> Henrik, I wonder if we should have an e10s testrun in Mozmill which runs our
> existing tests with e10s pref'd on to catch bugs like this? FWIW, I'd be
> fine if we isolated this to Nightly testruns for now.

Mozmill doesn't support e10s yet (see bug 695026). But once we do we will run tests for e10s for sure. Given that we already have popup blocker tests in the suite, nothing actionable for us here. I will set it to + given that we have coverage, but simply can't run them at the moment.