675574 - Do not allow more than one call to window.open() when we allow popups

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Mounir Lamouri (:mounir)]

I think we should try to do that... I've never seen any legitimate use of two popups with one user action. Actually, it might be very confusing for the user. Currently, according to the code, we shouldn't allow more than 20 popups opened at the same time but I have a code that allows 24 popups with a simple click handler. I wonder if I could flood your browser with popups with a simple click handler...

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

It's worth looking at the history here; I'm pretty sure at least some real sites wanted two popups in a click handler (e.g. opening up panels of a web app UI).

Comment 3

[Jesse Ruderman]

This will take http://www.thewildernessdowntown.com/ from 90% broken to 100% broken. I'm all for that; I never got it to work properly, even in Chrome.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: That might work

The popup management is quite weird and should probably get its own class to not have the logic distributed to a lot of classes...

Anyway, that code is working but I've no idea if that couldn't trigger some unexpected side effects.

Boris, I agree that it might break some legitimate use but I think it is worth trying to push this patch at the beginning of the next cycle and see what is happening.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

It happens that I did update the wrong patch and I did lost the correct one. Had to rewrite it... I guess being dumb has a price! :)

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 550302 [details] [diff] [review]
Patch v1

   NS_AUTO_POPUP_STATE_PUSHER(PopupControlState aState, PRBool aForce = PR_FALSE)
     : mOldState(::PushPopupControlState(aState, aForce))
+    , mPreviousOpenState(::GetPopupOpenedState())
   {
+    SetPopupOpenedState(noOpenedPopup);

Should we check if the current popup opened state is openedPopup here and not set the state to noOpenedPopup in that case, to deal with any possible re-entrant cases or what not here (not sure if that's acutally possible though)?

r=jst either way.

Comment 7

[Mounir Lamouri (:mounir)]

Actually, I don't think that could happen. I mean, having a script being able to get |SetPopupOpenedState(noOpenedPopup);| being called more than once, thus being able to open multiple popups. I believe that we allow popups (via this popup pusher class) only for trusted events and the content can't create a trusted event...

Comment 8

[Johnny Stenback (:jst)]

The cases where I was thinking something like that *might* be possible is with window.showModalDialog(), but I believe we reset popup state etc while we're processing that modal dialog, so we're probably ok. Either way, I believe this patch moves us in the right direction.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

comment 6 was actually true. It was possible to abuse the restriction doing this:
<button onclick="clickHandler();">click</button>
function clickHandler() {
  window.open('url', '', 'foo');
  button.click();
}

This patch fix this case and adds a few tests. So, re-asking a review. for the tests and the change.

Comment 10

[Mounir Lamouri (:mounir)]

This patch is breaking a few tests. Some explicitly assume that two popups are opened. They might need to be removed but I need some approval regarding this.

Comment 11

[Mounir Lamouri (:mounir)]

Finally pushed with tests fixes.

Hope we will not get too many complaints.

Comment 12

[Michael Wu [:mwu]]

https://hg.mozilla.org/mozilla-central/rev/2780356be1a1

Comment 13

[Matt Brubeck (:mbrubeck)]

This appears to have caused a new frequent test failure on Windows: bug 690220.

Comment 14

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound because even after the patch from bug 690220 we are also still seeing an increase in M-oth shutdown timeouts that we think are likely to be caused by this patch.  After bug 690220 landed on inbound, the shutdown timeouts happened on all platforms.  (Before that they were seen on Windows only.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb95f690d75

Comment 15

[Matt Brubeck (:mbrubeck)]

Backout merged to mozilla-central: https://hg.mozilla.org/mozilla-central/rev/3bb95f690d75

Comment 16

[Matt Brubeck (:mbrubeck)]

Some rough stats that might help with testing and re-landing this patch:  Between the m-c and m-i, there were 13 pushes that included both this change and the patch from bug 690220.  In those 13 pushes, there were a total of 10 mochitest-other shutdown timeouts.

In the 7 pushes on m-i after both changes were backed out, there are 0 mochitest-other shutdown timeouts (although not all of those pushes have full test runs yet, and some of them suffered from other M-oth bustage).

Comment 23

[:Gijs (out for now; he/him)]

Mounir isn't working on this right now, so unassigning so hopefully someone else can pick this up and retry. Our test infra is a lot more robust now...

Comment 24

[Wennie]

Hi Andrew, Can you have someone in your team to take a look at this bug? Is it possible to land this this quarter? Thanks!

Comment 29

[Marion Daly [:mdaly]]

Hey :peterv, this has been identified as a priority by Wennie, can you either give this a look or suggest who else on the DOM team would be a good fit for this bug? 

Thank you,
Marion

Comment 30

[Marion Daly [:mdaly]]

Overholt: Can you followup on this? This was identified as an evil trap.

Comment 31

[Andrew Overholt [:overholt]]

No time to work on this right now due to other urgent priorities. We'll get to it as soon as we can.

Comment 32

[0joshuaolson1]

Would the intended fix prevent developers using the web console from using window.open multiple times?

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 1 - one window.open() allowed for event

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 2 - Remove dom.popup_maximum

Comment 36

[Andrea Marchesini [:baku]]

Attachment: part 2 - Remove dom.popup_maximum

Comment 37

[Andrea Marchesini [:baku]]

Attachment: patch

Let's merge the 2 patches.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

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

this let's click to open just one popup but mouseup up to 20. I don't think we want such weird setup.

Comment 39

[Andrea Marchesini [:baku]]

Attachment: window_open_1.patch

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9022931 [details] [diff] [review]
window_open_1.patch

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  d71404ed02ef6861773ab39abd70e1977aaeec7b
>Bug 675574 - Allow just 1 window.open() per event, r?ehsan
r=smaug then :p



>+add_task(async _ => {
>+  info("2 window.open()s in a single keypress event: only the first one is allowed.");
s/keypress/keydown/


>+add_task(async _ => {
>+  info("2 window.open()s by non-event code: only the first one is allowed.");
this tests is for the case when no window is opened.

>+
>+  await SpecialPowers.pushPrefEnv({"set": [
>+    ["dom.block_multiple_popups", true],
>+    ["dom.disable_open_during_load", true],
>+  ]});
>+
>+  let tab = BrowserTestUtils.addTab(gBrowser, TEST_DOMAIN + TEST_PATH + "browser_multiple_popups.html?openPopups")
>+  gBrowser.selectedTab = tab;
>+
>+  let browser = gBrowser.getBrowserForTab(tab);
>+  let count = 0;
>+  let p = ContentTask.spawn(browser, null, () => {
>+    return new content.Promise(resolve => {
>+      content.addEventListener("DOMPopupBlocked", function cb() {
>+        if (++count == 2) {
>+          content.removeEventListener("DOMPopupBlocked", cb);
>+          ok(true, "The popup has been blocked");
>+          resolve();
>+        }
>+      });
>+    });
>+  });
>+
>+  await BrowserTestUtils.browserLoaded(browser);
>+
>+  await p;
>+  ok(true, "We had 0 windows.");
See this^

>+add_task(async _ => {
>+  info("1 window.open() executing another window.open(): only the first one is allowed.");
>+
>+  await SpecialPowers.pushPrefEnv({"set": [
>+    ["dom.block_multiple_popups", true],
>+    ["dom.disable_open_during_load", true],
>+  ]});
>+
>+  let tab = BrowserTestUtils.addTab(gBrowser, TEST_DOMAIN + TEST_PATH + "browser_multiple_popups.html")
>+  gBrowser.selectedTab = tab;
>+
>+  let browser = gBrowser.getBrowserForTab(tab);
>+  await BrowserTestUtils.browserLoaded(browser);
>+
>+  // We don't receive DOMPopupBlocked for nested windows. Let's use just the observer.
>+  let obs = new WindowObserver(1);
well, WindowObserver resolves when one window has been opened. It doesn't really check if there has been another one.
So this test relies on the framework to warn about unclosed tab. I guess that is ok.

>+
>+  await BrowserTestUtils.synthesizeMouseAtCenter("#openNestedPopups", {}, tab.linkedBrowser);
>+
>+  await obs;
>+  ok(true, "We had only 1 window.");
but please clarify this. Drop 'only'=

>+
>+add_task(async _ => {
>+  info("window.open() and .clock() on the element opening the window.");
click


 
>+VARCACHE_PREF(
>+  "dom.block_multiple_popups",
>+   dom_block_multiple_popups,
>+  bool, true
>+)

Could you add a comment what this pref is about. StaticPrefs seems to be in general
rather well documented.


Make sure to push to try with all the tests run, including in debug and opt builds. This is a bit risky, so good to land rather early in a cycle.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c34557dc70d
Allow just 1 window.open() per event, r=smaug

Comment 42

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/0c34557dc70d

Comment 44

[Boris Zbarsky [:bzbarsky]]

What pops the newly-pushed popup control state?

Comment 45

[Andrea Marchesini [:baku]]

None and it's not needed: before the newly-pushed state, there was another state pushed by a nsAutoPopupStatePusher.
That nsAutoPopupStatePusher will pop the state, restoring its known old state, overwriting the newly-pushed value.

If there is not a nsAutoPopupStatePusher, we could not be in that code path, because the default value of nsContentUtils::sPopupControlState is 'openAbused'.

Comment 46

[Chris Mills [:cmills]]

OK, so I've documented this:

Filed a PR to update compat data around this:
https://github.com/mdn/browser-compat-data/pull/3112

Added a note to the Fx 65 rel notes to cover it:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#APIs

Let me know if you think anything else is needed. Thanks!

Comment 47

[Andrea Marchesini [:baku]]

All good to me! Thanks.

Comment 48

[Daniel C]

> +user_pref("dom.block_multiple_popups", false);

Which bug tracks flipping this to true? (Or have I misunderstood? It sounds to me from the naming of the pref that it needs to be true to fix this bug as it is titled).

Comment 49

[:Gijs (out for now; he/him)]

(In reply to Daniel Cater from comment #48)
> > +user_pref("dom.block_multiple_popups", false);
> 
> Which bug tracks flipping this to true? (Or have I misunderstood? It sounds
> to me from the naming of the pref that it needs to be true to fix this bug
> as it is titled).

That's the unit-test pref file, where this is turned off so as not to mess with tests trying to open popups. The default in shipped builds is `true` (see the StaticPrefList.h entry, or about:config in any recent nightly).

Comment 50

[Kohei Yoshino]

Posted site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/window-open-can-now-be-called-only-once-per-event/

I think this can be added to the end-user release notes as well.

Release Note Request (optional, but appreciated)
[Why is this notable]: Improves UX by blocking multiple pop-ups/tabs
[Affects Firefox for Android]: Maybe?
[Suggested wording]: Improved the pop-up blocker to prevent more annoying pop-up windows from being opened.
[Links (documentation, blog post, etc)]: None

Comment 52

[ephbase-moz]

This broke js bookmarklets opening multiple tabs (Bug 1524137)

Comment 53

[Bob Babcock]

I don't see any way to set a site-specific exception so I have to use the global dom.block_multiple_popups. The site I maintain that opens multiple tabs is https://www.wssddc.com/comic-favorites.html. Windows and Android FF65 both need the exception.

Comment 54

[Johann Hofmann [:johannh]]

(In reply to Bob Babcock from comment #53)

I don't see any way to set a site-specific exception so I have to use the global dom.block_multiple_popups. The site I maintain that opens multiple tabs is https://www.wssddc.com/comic-favorites.html. Windows and Android FF65 both need the exception.

On desktop at least you should get a yellow notification bar (or if you turned that off at least an indicator in the left-hand side of the url bar) that allows you to show the popup and add an exception.

Comment 55

[Bob Babcock]

On Windows and Linux I get the yellow notification bar, but not on my Android tablet. (And on Windows and Linux I usually use SeaMonkey, not Firefox.)