Mounir Lamouri (:mounir) Reporter
	Description • 13 years ago

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...

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 13 years ago

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).

	Jesse Ruderman
	Comment 3 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 4 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 5 • 13 years ago

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! :)

	Johnny Stenback (:jst)
	Comment 6 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 7 • 13 years ago

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...

	Johnny Stenback (:jst)
	Comment 8 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 9 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 10 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 11 • 13 years ago

Finally pushed with tests fixes.

Hope we will not get too many complaints.

	Michael Wu [:mwu]
	Comment 12 • 13 years ago

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

	Matt Brubeck (:mbrubeck)
	Comment 13 • 13 years ago

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

	Matt Brubeck (:mbrubeck)
	Comment 14 • 13 years ago

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

	Matt Brubeck (:mbrubeck)
	Comment 15 • 13 years ago

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

	Matt Brubeck (:mbrubeck)
	Comment 16 • 13 years ago

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).

	:Gijs (he/him)
	Comment 23 • 7 years ago

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...

	Wennie
	Comment 24 • 6 years ago

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!

	Marion Daly [:mdaly]
	Comment 29 • 6 years ago

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

	Marion Daly [:mdaly]
	Comment 30 • 6 years ago

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

	Andrew Overholt [:overholt]
	Comment 31 • 6 years ago

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

	0joshuaolson1
	Comment 32 • 6 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 37 • 6 years ago

Let's merge the 2 patches.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 38 • 6 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 40 • 6 years ago

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

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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.

	Pulsebot
	Comment 41 • 6 years ago

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

	Natalia Csoregi [:nataliaCs]
	Comment 42 • 6 years ago
bugherder

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

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 6 years ago

What pops the newly-pushed popup control state?

	Andrea Marchesini [:baku] Assignee
	Comment 45 • 6 years ago

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'.

	Chris Mills [:cmills]
	Comment 46 • 6 years ago

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!

	Andrea Marchesini [:baku] Assignee
	Comment 47 • 6 years ago

All good to me! Thanks.

	Daniel C
	Comment 48 • 5 years ago

> +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).

	:Gijs (he/him)
	Comment 49 • 5 years ago

(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).

	Kohei Yoshino [:kohei]
	Comment 50 • 5 years ago

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