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

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
8 years ago
3 months ago

People

(Reporter: mounir, Assigned: baku)

Tracking

(Depends on 2 bugs, Blocks 4 bugs, {dev-doc-complete, site-compat, testcase})

Trunk
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 65+, firefox65 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

Reporter

Description

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

Updated

8 years ago
Blocks: eviltraps
Reporter

Comment 1

8 years ago
Posted file testcase (obsolete) —
Reporter

Updated

8 years ago
Assignee: nobody → mounir
Keywords: testcase
Reporter

Updated

8 years ago
Blocks: 669045
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

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

Comment 4

8 years ago
Posted patch That might work (obsolete) — Splinter Review
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.
Attachment #549925 - Flags: review?(jst)
Reporter

Updated

8 years ago
Status: NEW → ASSIGNED
Whiteboard: [needs review]
Reporter

Comment 5

8 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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! :)
Attachment #549925 - Attachment is obsolete: true
Attachment #549925 - Flags: review?(jst)
Attachment #550302 - Flags: review?(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.
Attachment #550302 - Flags: review?(jst) → review+
Reporter

Comment 7

8 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...
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.
Whiteboard: [needs review]
Reporter

Comment 9

8 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #550302 - Attachment is obsolete: true
Attachment #553814 - Flags: review?(jst)
Reporter

Updated

8 years ago
Whiteboard: [needs review]
Attachment #553814 - Flags: review?(jst) → review+
Whiteboard: [needs review]
Reporter

Updated

8 years ago
Whiteboard: [needs tests fixes]
Reporter

Comment 10

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

Updated

8 years ago
Blocks: popups
Reporter

Comment 11

8 years ago
Finally pushed with tests fixes.

Hope we will not get too many complaints.
Flags: in-testsuite+
Whiteboard: [needs tests fixes] → [inbound]
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/2780356be1a1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
This appears to have caused a new frequent test failure on Windows: bug 690220.
Depends on: 690220
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout merged to mozilla-central: https://hg.mozilla.org/mozilla-central/rev/3bb95f690d75
Target Milestone: mozilla10 → ---
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).

Updated

7 years ago
Duplicate of this bug: 550238

Updated

7 years ago
Blocks: 197919

Updated

7 years ago
Duplicate of this bug: 783012

Updated

2 years ago
Duplicate of this bug: 1310659
Duplicate of this bug: 1354168
Duplicate of this bug: 1364962
Priority: -- → P2

Updated

2 years ago
Duplicate of this bug: 1407684

Comment 23

2 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...
Assignee: mounir → nobody

Comment 24

2 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!
Assignee: nobody → overholt
Priority: P2 → P1
Duplicate of this bug: 1167023
Duplicate of this bug: 1340634
Duplicate of this bug: 685828
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
Flags: needinfo?(peterv)
Overholt: Can you followup on this? This was identified as an evil trap.
Flags: needinfo?(overholt)
No time to work on this right now due to other urgent priorities. We'll get to it as soon as we can.
Assignee: overholt → nobody
Flags: needinfo?(peterv)
Flags: needinfo?(overholt)
Priority: P1 → P2

Comment 32

Last year
Would the intended fix prevent developers using the web console from using window.open multiple times?
Duplicate of this bug: 1484810
Assignee

Comment 34

8 months ago
Assignee: nobody → amarchesini
Attachment #549722 - Attachment is obsolete: true
Attachment #553814 - Attachment is obsolete: true
Attachment #9022646 - Flags: review?(ehsan)
Assignee

Comment 35

8 months ago
Attachment #9022648 - Flags: review?(ehsan)
Assignee

Comment 36

8 months ago
Attachment #9022648 - Attachment is obsolete: true
Attachment #9022648 - Flags: review?(ehsan)
Attachment #9022649 - Flags: review?(ehsan)
Assignee

Updated

8 months ago
Attachment #9022649 - Attachment is obsolete: true
Attachment #9022649 - Flags: review?(ehsan)
Assignee

Comment 37

8 months ago
Posted patch patch (obsolete) — Splinter Review
Let's merge the 2 patches.
Attachment #9022646 - Attachment is obsolete: true
Attachment #9022646 - Flags: review?(ehsan)
Attachment #9022654 - Flags: review?(ehsan)
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.
Attachment #9022654 - Flags: review?(ehsan) → review-
Assignee

Comment 39

8 months ago
Attachment #9022654 - Attachment is obsolete: true
Attachment #9022931 - Flags: review?(bugs)
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.
Attachment #9022931 - Flags: review?(bugs) → review+

Comment 41

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

Comment 42

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c34557dc70d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee

Updated

8 months ago
Duplicate of this bug: 1208950
What pops the newly-pushed popup control state?
Flags: needinfo?(amarchesini)
Assignee

Comment 45

8 months 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'.
Flags: needinfo?(amarchesini)
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!
Assignee

Comment 47

7 months ago
All good to me! Thanks.

Comment 48

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

Comment 49

7 months 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).
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
relnote-firefox: --- → ?
Keywords: site-compat
Assignee

Updated

5 months ago
Duplicate of this bug: 1457693
Depends on: 1523830

Comment 52

5 months ago

This broke js bookmarklets opening multiple tabs (Bug 1524137)

Comment 53

5 months ago

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.

(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

5 months ago

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

Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.