Popups from a Site that is in the "Allowed List" (whitelist) are blocked, starting with the n-th popup (dom.popup_maximum)

RESOLVED FIXED in mozilla1.9.2

Status

()

P2
normal
RESOLVED FIXED
14 years ago
2 months ago

People

(Reporter: C.Voelker, Assigned: mozilla+ben)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

(URL)

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.3) Gecko/20040913 Firefox/0.10

I encounter this phenomenon on a Site where you have to be logged in, so it does
not make sense to give a URL. 

With the first -lets say- twenty popups everthing works fine but after half an
hour or so, the browser starts blocking popups and shows a related information
bar. I cant tell exactly whether it is related to the time spent on the site or
to the number of popups but the fact that popups are blocked after a while is
reproducable. It might even have to do with the number of windows opened
subsequently this way: Clicking opens a popup wherein I click another link that
opens another popup and so on. You might think of that as poor interface design,
and I would partly agree but I havent built the site and it is quite valuable
for me anyway.

Closing the tab or the whole window does not cure the problem. Closing all
windows and opening the URL again does not do the job either, I have to restart
Firefox to return to normal operation. When I go to "Edit Popup blocker
Options…" I can see that the site is in the list already. Ah, obviously I have
to mention that popup blocking is active ;-). This did not happen with version
0.9.3 that I downloaded on August, 3rd and other versions before.


Reproducible: Always
Steps to Reproduce:
Clicking Popup Links extensively on a Site that is unblocked
Actual Results:  
The browser stops opening user requested popups after a while

Expected Results:  
the browser should open the popups
(Reporter)

Comment 1

14 years ago
When clicking the menu entry "Show 'http://www.blockedsite.com/blockedlink.html'
in the drop menu of the info bar at the top of the window, the popup is not
shown either.
(Reporter)

Updated

14 years ago
Version: unspecified → 1.0 Branch

Comment 2

14 years ago
I can reproduce it also... takes 20-25 clicks of opening and closing the popup
within the unblocked site, then the site will become blocked until you restart
the browser.

Using latest Slackware Linux, Kernel 2.6.7, on x86 hardware.
Never saw this in Firefox versions prior to 1.0 Preview Release.

Comment 3

14 years ago
I have also seen this bug. It even happens, if the option "block popup windows"
is completely turned off.

Comment 4

14 years ago
According to the description something is wrong with dom.popup_maximum (this
setting is respected even if the popup blocking is disabled). However I cannot
reproduce this, neither in my Seamonkey debug build, nor in FF1.0PR. Tabbrowser
Extensions (TBE) is known to interfere with the popup blocker in some way, maybe
you have it installed? If so - does removing this extension solve the problem?
(Reporter)

Comment 5

14 years ago
> Tabbrowser Extensions (TBE) is known to interfere with the popup blocker ...

Well, I deactivated several Extensions now and wil try to reproduce it again. I
could not do it so far. If I will find it to work fine from now on, I will
activate Extensions again step by step. In case I find a culprit I will update
it and try again. This whole process will certainly take a week or so, so dont
expect me to report soon. 

I havent had such thing as a TBE installed. I thought that tabbed browsing was a
standard feature of Firefox not dependent of an extension. Currently, I run a
german language pack, the web developers toolbar and magpie as extensions.
Actually, if the site has to be in the whitelist for the popups to open, its not
using a method that gets interpreted as "user-requested" otherwise the popups
would always work.  This could be because of redirects, or really weird
javascript, or basically anything that isn't "if a user clicks on X, open popup."

If someone can produce a testcase that replicates this problem, great, but I'm
thinking this is INVALID.  Dan, am I reading this right?

Comment 7

14 years ago
Might as well give Christian time to work through his extensions. Christian, if
you discover that you can reproduce this problem without extensions, please
provide a testcase. If you discover that an extension is responsible, let us
know. But we'll be closing this bug and hopefully passing the info on to the
extension writer.

I anticipate closing this bug WFM. In my experience with FF 0.10 you are indeed
restricted to 20 simultaneously open allowed popup windows, as expected. But
close one and you can open another. Note you're generally not allowed to open
the replacement popup immediately: it usually takes a few seconds. So if you're
right against your 20 window limit you may notice some latency like that.

Note you can encourage running into the limit more quickly (or more slowly) by
adjusting the dom.popup_maximum pref.
(Reporter)

Comment 8

14 years ago
Ok, this makes up for three more tasks on my side:

- Try what happens exactly when I remove the Site
  from the "allowed" List.

- I will ask the programmer of the Site to follow
  this bugs thread.

- I will check the JavaScript Code as good as I
  can myself. I am not a programmer, but I have
  a working knowledge of so called "copy and
  paste programming" and reading code.

The Site is heavily trying to push IE to extremes
and I blamed them several times that they ignore
Mac Users and others but it turned out wrong every
time. It is made by a real good guy who knows what
he does.

Phew, never though, bugwriting could turn out to 
be work by itself ;-). Keep on coding!

Yours, Christian

Comment 9

14 years ago
dom.popup_maximum proved very helpful...

I upped this value from 20 to 10,000 and now I don't seem to have a problem...
(of course, I'm not going to sit here and open the popup 10,000 times, so....)

Prior to that, I could open and close the popup only 20 times before it would
refuse to allow another to be opened. (no change with delay after closing last one)

(I also had webdev extension installed, but uninstalling it did not change
anything -- it was the only extension I had installed)

Apparently the problem is with the counter not going back down when the popup is
closed. But... it probably *is* related to some of my funky javascript (or the
complexity of the popup), because I wasn't able to reproduce it with a very
simple "click here and open a popup" example. However, I'd still call it a bug
because IMHO, the popup control *should* work the same regardless of how the
popup was opened. 

One more note -- I *wasn't* able to reproduce the problem on Windows at all - so
maybe whatever there is checking to see if the popup has closed must be working
correctly? -- at least for the testing I was doing.

If the person (or people) responsible for confirming or closing this bug would
like a test account in my system where the problem may be seen, or if you want
to see the code which opens the popups in question, just let me know.
> However, I'd still call it a bug
> because IMHO, the popup control *should* work the same regardless of how the
> popup was opened. 

This kind of misses the point on how popup blocking works.  If the user clicks
on something with an onclick function that opens the window, that's definitely a
user-initated action.  If the user does a mouseover, etc, that's trying to load
a popup via a sneaky way, so we block it.  The 20 popups max is to ensure that
even if you whitelist a site, it can't crash your app/PC by killing you with a
massive number of popups.

A reduced testcase (only the code needed to reproduce the bug) would be good in
determining what's going on in your case.

Comment 11

14 years ago
Ok, I got a test case together that works to reveal the problem..

It also tells what it is about the problem that makes it appear or not...
Apparently, if the popped-up window has a bunch of onclick's in it, then you
have a problem. If it doesn't you're OK.

Also, problem does not show up ever in windows. (xp pro)

Test and a little more info is at: http://www.mutualgravity.com/testpopup.php
(Reporter)

Comment 12

14 years ago
Hello,

here are some interim results.
in effect, they dont tell a thing.
Probably I made the old mistake and changed too many things at once so I am
confused about what relates to what.

Currently I cant see bockup blocking when I am surfing the Site mentioned first.
I have removed this Site from the allowed Sites list.
I asked the Sites programmer to follow this bugs thread (no answer yet).

I tried to reactivate my Extensions step by step but only one of them could be
reactivated without updating. When updating I could not test which of them
caused the problem. Most of them turned out to be of limited use so I dont want
to activate them again. After activating the one Extension, no popup blocking
appeared again. Hey, this great actually!

The javaScript Code of the Site cant be understood cmpletely within five Minutes
and yes, it contains onclick mehtods.

The mutualgravity test does not work for me in my current config.

I tried to implement this dom.popup_maximum stuff to ease testing, but I am not
sure about how to use this. I created a file named user.js containing a single
line 'user_pref("dom.popup_maximum", 5);' and a newline. Do I have to put this into 
~/Library/Application\ Support/Firefox/Profiles/default.44h/ or 
~/Library/Firefox/Profiles/default/famx1ns5.slt/ ?

The thing most likely solution as of now is that one of the Extensions is
broken. I dont know whether I will spend more time which of them.

Christian
(Reporter)

Comment 13

14 years ago
Hello,

I have just learned about this cool about:config stuff (thanks Bill Sechrist)
and configured the dom.maximum_popup to four.

Well, everything starts to get weird. Now, I can open neither popup from
mutualgravities sample page any more. It does not matter whether I open the page
in a tab or in separate window. Putting it into the Allowed Sites list does not
help either. Obviously, some very basic things are screwed up now. Well, it will
take more time to test properly so that anybody can make sense of these results.

Is there a safe way to return to a defined setup? My whole environment remains
the same when I install a new version and I think of this to be rather
comfortable. But now, I am not sure where Firefox stores all his extensions,
preferences and so on. I would like to keep my Bookmarks (well I know how to do
this), stored passwords and form fields and so on but remove Extensions to make
sense of my testing. 

I havent found the extensions searching for extension_name.jar and I havent
found parts of Firefox in the /Library folder. Everything seems to reside in the
application bundle (extensions can hardly be there because they would be missing
after updating) and in the ~/Library folder. Am I right? Still then, I cant find
the extensions. I dont trust the Extension Manager because he shows texts like
"will be disabled after restart" still after several restarts and he also mixes
up the text for disabling with the disabled state and vice versa.

The Programmer of my community Site is following this thread now. He has send me
a snippet of code that he has found to be relevant. He says that it is "plain
simple" JavaScript. I will put it here if he agrees with me to do so. It is
formatted quite readable and contains only one german word "Fenster" which means
Window.
(Reporter)

Comment 14

14 years ago
function OpenHPWindow(URL){
iLeft = window.screen.availWidth-580;
iHeight = window.screen.availHeight-200;
iTop = 0;
Fenster = "HP";
WinURL = new String();
WinURL = URL;
WinURL = WinURL.toLowerCase()

if (WinURL.indexOf("hp.asp") >= 0 || WinURL.indexOf("gu.asp") >= 0 ||
WinURL.indexOf("ug.asp") >= 0)
{
	Fenster = "HP";
	if (document.location.href.toLowerCase().indexOf("gb.asp") >= 0)
		Fenster = "HP2";
}
if (WinURL.indexOf("msg") >= 0)
{
    myDate = new Date()
    myWin =
myDate.getMilliseconds()+"_"+myDate.getMinutes()+"_"+myDate.getHours()+"_"+myDate.getDay()+"_"+myDate.getMonth()
	Fenster = "MSG" + myWin;
	iLeft = 0;
	iTop  = 0;
}
if (WinURL.indexOf("gb") >= 0)
{
	Fenster = "GB";
	iLeft -= 60;
	iTop  += 60;
}
if (WinURL.indexOf("history") >= 0)
{
	Fenster = "HIST";
	iLeft -= 90;
	iTop  += 90;
}

HP =
window.open(URL,Fenster,'width=570,height='+iHeight+',screenX='+iLeft+',screenY=0,left='+iLeft+',top='+iTop+',scrollbars=yes,status=yes,resizable=yes,menubar=no,toolbar=no');
HP.focus();
}

Comment 15

14 years ago
comment 11: Myself I have access only to a Windows box so I can't verify your
testcase. So the mutualgravity.com/testpopup.php testcase works as described
exactly as given? I mean to ask, you've duplicated your findings using the
testcase as posted, and the php script isn't handing back anything different to
a Mac browser? That's a head-scratcher. By the way we do like to have testcases
local to Bugzilla. For one thing it answers my question about what the server is
up to.

comment 14: I'm not sure what to do with all that. It's probably the context of
the function call that's more important than the function itself. I would
caution to be careful that Fenster always contains a unique value if you always
want a new window.

comment 13: Use a new profile to return to a defined setup. Search on
http://forums.mozillazine.org/ or ask around for specific instructions.

Comment 16

14 years ago
Created attachment 160028 [details]
html doc with popup links

I made the links pop up copies of itself at my site. See doc for details.

Comment 17

14 years ago
Yes DanM, it behaved as described for me just as it was. =)

Also the new much simpler test case I just uploaded in the attachment works the
same right from the bug server. It's just simple html with simple
"onclick="window.open(blahbla..." code to open copies of itself in popups. See
attachment for details.

Comment 18

14 years ago
The bug is that popups > dom.popup_maximum are blocked even if the site is in
the popups-allowed list.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 19

14 years ago
*** Bug 261344 has been marked as a duplicate of this bug. ***

Comment 20

14 years ago
Happens in OS/2 also. I don't think it is OS specific.

Comment 21

14 years ago
This problem is occuring for me under Windows XP.

Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1

I have Chatzilla 0.9.66 and Mouse Gestures 1.0 installed.

Comment 22

14 years ago
THis also happens for me, e.g. on sites with the "Gallery" PHP-Skript
(gallery.sf.net) installed.

The popup that shows the progress bar while adding photos is blocked. Other
popups like the login-popup or the "add photos" popup are not (they are triggerd
by a mouse click).

The site is in the "Allow popups from" list - if I try to open the popup using
the  "Show $URL" option in "the blocked popup"-warning, the popup still does not
open, but the number of popups in the "Firefox prevented this site from opening
$number popup windows" is increased by one. Also, one more option "Show $URL" is
available.


The site is in the allowed list, so in the warning message there is only to
option to "block popups from this site".

Comment 23

14 years ago
Another website to reproduce it:
-set dom.popup_maximum to 2
-goto http://www.friendscout24.de/
-on the right side, you'll see a list of logged in people.
 if not, do a "suche" (search)
-click on 3 of these nicknames
-the 3rd one should not open
-close the opened windows
-click on other nicknames. from now on, it is 
 not possible to open the profile window any longer. 
 I'll be blocked.

Updated

14 years ago
OS: MacOS X → All
Hardware: Macintosh → All

Comment 24

14 years ago
*** Bug 269535 has been marked as a duplicate of this bug. ***

Comment 25

14 years ago
*** Bug 262810 has been marked as a duplicate of this bug. ***

Comment 26

14 years ago
see also
http://forums.mozillazine.org/viewtopic.php?t=175798&highlight=debug+popup+blocker
Summary: Popups from a Site that is in the "Allowed List" are blocked, starting with the n-th popup → Popups from a Site that is in the "Allowed List" (whitelist) are blocked, starting with the n-th popup (dom.popup_maximum)

Comment 27

14 years ago
*** Bug 272628 has been marked as a duplicate of this bug. ***

Comment 28

14 years ago
*** Bug 275915 has been marked as a duplicate of this bug. ***

Comment 29

14 years ago
*** Bug 276011 has been marked as a duplicate of this bug. ***

Comment 30

14 years ago
(In reply to comment #4)
> ... Tabbrowser
> Extensions (TBE) is known to interfere with the popup blocker in some way, maybe
> you have it installed? If so - does removing this extension solve the problem?

I experience this behavior on Windows XP (SP2) with FF1.0, and have no
extensions installed.

Comment 31

14 years ago
Created attachment 171205 [details]
Zip file with sample files that reproduce the bug.

OK, I've hit this bug one too many times. (It makes our product look silly,
because we pop up windows....and after the 20th pop-up...boom....no more
pop-ups allowed.)
So, I stripped and stripped and now I have a simple case that reproduces the
bug every time.
I'm running Windows XP SP2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107
Firefox/1.0

Extract the 3 HTML files. In Firefox, using "about:config" set
dom.maximum_popup to a low value. (I used 5).
Open page1.html, and close the resulting pop-up window. Refresh the window 5
times (each time, closing the pop-up).
On the 6th refresh...no more pop-ups are allowed, when they should be.

Comment 32

14 years ago
linux, ff1.0. Problem gone after uninstalling TabExtensions.

Comment 33

14 years ago
*** Bug 257501 has been marked as a duplicate of this bug. ***

Comment 34

14 years ago
Running into this problem on www.dba.dk which is a private market place.
Hovering over a link shows a preview picture of the item clicking the link will
open a new window with a picture of the item. 

The mouse over preview does not work in FireFox which is not that critical. The
real critical thing is that all of a sudden the Popup blocker will block all
atempts at view an item. To solve this I disabled the popup blocker altogether
this had no effect at all - the popup blocker still prevents me from viewing any
items after some time - i just get the yellow info that popups have been blocked.  

Comment 35

14 years ago
I have had the exact same problem for a long time now. I'm running Windows 2000
SP4, Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041110
Firefox/1.0.

All popups become blocked after a while (even if I have turned off popup
blocking completely).

Comment 36

14 years ago
*** Bug 284528 has been marked as a duplicate of this bug. ***

Comment 37

14 years ago
*** Bug 261678 has been marked as a duplicate of this bug. ***
*** Bug 268591 has been marked as a duplicate of this bug. ***

Comment 39

14 years ago
Is this bug going to be fixed in the upcoming 1.1 release? 

I believe its quite well described now and its embarrassing when talking about
web applications that use dialog windows to display important information.

Comment 40

14 years ago
Also seeing this with Lotus Notes Webmail 6.5.
*** Bug 302563 has been marked as a duplicate of this bug. ***
*** Bug 275670 has been marked as a duplicate of this bug. ***

Comment 43

13 years ago
*** Bug 310692 has been marked as a duplicate of this bug. ***

Comment 44

13 years ago
from bug 260264 comment 3:

This is a limit imposed in nsGlobalWindow, and therefore doesn't really have any
clue about any whitelists.

http://lxr.mozilla.org/mozilla/source/dom/src/base/nsGlobalWindow.cpp#3162

Comment 45

13 years ago
*** Bug 297105 has been marked as a duplicate of this bug. ***

Comment 46

13 years ago
(In reply to comment #44)

Correct is "from Bug 297105 comment 3". Sorry.

Comment 47

13 years ago
This isn't quite correct. The limit is enforced in
nsGlobalWindow::CheckForAbusePoint() and it ignores the whitelist by design. It
sets the state to openOverridden whereas openAbused would be required to block
the popup respecting the whitelist and popup blocker deactivation. The reasoning
behind this was explained in some other bug (closed as INVALID as far as I
remember).

The original point of this bug was a very different one. According to the report
gOpenPopupSpamCount isn't always reset correctly, causing popups to be
disallowed even if there are no open popups at all. Unfortunately it seems that
almost nobody can reproduce this problem.
Assignee: firefox → nobody
QA Contact: mconnor → preferences
Version: 1.0 Branch → Trunk
Using the testcase provided in comment #11, popups will always open. You've whitelisted the site and are performing a user initiated action that opens a popup on the allowed site. This is expected.

Try http://www.popuptest.com/popuptest4.html to do the mouseover technique described in comment #10, this works as expected with Bon Echo from 2006073104 On Win XP. Once you hit the the dom.popup_maximum pref limit of 20, the sneaky mouse over popup is blocked.

WFM
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME

Comment 49

11 years ago
Created attachment 303529 [details]
reproduce popup blocker bug

First set the field dom.popup_maximum (in about:config) to 3 for example,
open the test.html file.

A popup will open, here click the link 'open popup'...
close the popup and retry it 3 times...

It's important that the test.html opens the test.html as popup from the 'onload' event once. 

In the test.html is an Object called 'btnClass', here you can see that teh 'onclick' calls a function that returns 'false'. And the 'onmouseup' calls the callback (that opens a popup). 

In the test2.html is a simple 'a'-tag and an 'init' function called at document's 'onload'. 

If i do not call the init function at the test2.html or use the 'onclick' methode in the btnClass-object to call the callback, then it works.

Comment 50

11 years ago
This is still happening and is a duplicate of Bug 320031 as well as others.  I would recommend that firefox be shipped with default value in dom.popup_maximum of -1 to disable it.   It should not block popups ever if the site is in the allowed list.

Comment 51

10 years ago
This bug is still there, especially affecting pandora.com 's song/artist information pop-ups. If the limit is not going to be disabled, would it be possible to at least fix the pop-up blocked message? 

That is, when the limit is reached, show the pop-up blocked message, but have an option to tell Firefox that the pop-ups are benign, or to reset the counter at least?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Duplicate of this bug: 314781

Comment 53

9 years ago
RESOLVED WORKSFORME is not appropriate!

On autoscout24 I'm loading the search results, each car article is the loaded manually into tabs. If I want to view the images of that particular car, they are forcefully opened in a popup window through window.open which initiates the counter. Now if I close that popup window and the parent tab, shouldn't the counter be reset?

If I try to open another 20 car articles in separate tabs, as was done previously, the counter keeps its state and the popup blocker stops the window.open action to perform as expected.

If I allow the popup from the status bar icon, the counter resets and the operation can continue for another 20/19 times.

This behavior doesn't work as intended as the counter should be applied for each document separately, since it's marked as a per document setting (dom.) and not as a global setting.

As a side note, my dom.popup_allowed_events are set to dblclick reset to prevent rogue on click events to be executed (ex: uploading.com download button). Adding click to the list overcomes the limit but leaves the browser open to other unwanted behaviors during this event.

Workaround: dom.popup_maximum = -1 (Thanks Don!)

Comment 54

9 years ago
Agreed.  Can someone please re-open this
Flags: blocking-firefox3.6?
REOPENED due to comment 53
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 56

9 years ago
I have come up against this same limitation (dom.popup_maximum = 20) on a daily basis while working with the Oracle PeopleSoft Enterprise software at my workplace.

If I set the limit to say, 23, I am then able to open up 3 more popup windows and no more.

I have my corporate domain (.arrisi.com) in the whitelist, and  have even tried it with the popup blocker entirely disabled...

Yet after clicking repeatedly on interface widgets that cause popup windows to appear, there is no ability to get them to open again short of restarting the browser, or setting dom.popup_maximum=-1 as suggested in comment 53.  

I don't enjoy restarting the browser as it usually breaks logged-in sessions, however due to the memory management bugs still plaguing FF it's not like I have much of a choice but that's another story (@ 645MB Real+650MB Virt right now...)

I'm honestly flabbergasted that this is still an issue in FireFox 3.5.2... 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
So the deal here is that after 20 popup windows are created, user initiated or otherwise, we block any subsequent popup, user initiated or otherwise?

Does that persist across sessions?
From skimming the code, this might not be trivial to fix, since it seems we treat user-action-initiated popups as "bad" here.  There's also the non-trivial aspect of not having a way to treat popups as user-initiated when opened by plugins.

This is a core DOM bug, since that's where this logic is implemented, but what seems like a complete solution is:

a) The primary maximum should be per-event (and probably lower, since > 3 popups per click is almost certainly an outlier)
b) User-initiated actions should never count as abuse for this count.
c) Ideally we'd make it possible for plugins to play nice with user-initiation.
d) For page-initiated events that are not user-initiated, we should simply let the abuse count reset over time so long-running apps don't hit this when they're not being abusive.  Alternatively, we could decrement the abuse count when windows are closed (look for the opener onclose).
Component: Preferences → DOM: Events
Flags: blocking-firefox3.6?
Product: Firefox → Core
QA Contact: preferences → events

Updated

9 years ago
Flags: blocking1.9.2?
Marking this a blocker for now, but if this turns out to be very scary I could be convinced we don't need it for 1.9.2.
Assignee: nobody → bnewman
Flags: blocking1.9.2? → blocking1.9.2+
(Assignee)

Comment 60

9 years ago
One big flaw in the implementation of the current model (limiting the maximum number of open popups) is that gOpenPopupSpamCount is decremented inside nsGlobalWindow::Cleanup, which is called from the nsGlobalWindow destructor.  Once upon a time, the window might have been destroyed immediately after it was closed, but nowadays the reference graph is more complicated, and nsGlobalWindows tend to be destroyed during cycle collection.  The non-deterministic delay before cycle collection contributes to the appearance that the gOpenPopupSpamCount never resets.

At a bare minimum, gOpenPopupSpamCount needs to be tied more directly to the closing of windows.
(Assignee)

Comment 61

9 years ago
The other flaw in the current implementation is that we do not honor all dom.popup_allowed_events.  The onmouseup event, for instance, still does not count as a user-initiated event, even after I append " mouseup" to the popup_allowed_events pref.
(Assignee)

Comment 62

9 years ago
Setting aside the question of whether the popup maximum should be global or per-event, here's some pseudocode that I propose for determining if a popup should count against the max:

  def countsAgainstPopupMaximum(event):
    assert "click" in dom.popup_allowed_events
    if popup blocker enabled:
      if host whitelisted:
        return event not in dom.popup_allowed_events
      else:
        return event is not "click"
    else:
      if host whitelisted:
        return false
      else:
        return event not in dom.popup_allowed_events

Implementation is complicated by nsGlobalWindow not knowing what the event actually was, but enough information is provided by gPopupControlState.
(Assignee)

Comment 63

9 years ago
Created attachment 401353 [details] [diff] [review]
Simplification of PopupControlState logic in nsGlobalWindow.

mconnor, I believe this patch satisfies (b) and (d) from your list.

Specifically, whitelisted hosts are never restricted from opening popups in response to events listed in dom.popup_allowed_events, but the dom.popup_maximum limit applies for events not listed in dom.popup_allowed_events.

Non-whitelisted hosts can open popups from "click" events without restriction, but other events in dom.popup_allowed_events are subject to the dom.popup_maximum limit.  Events not listed in dom.popup_allowed_events are prevented altogether from opening popups.

When popup blocking is turned off, all hosts are effectively whitelisted (i.e., the dom.popup_maximum limit applies only to events not in dom.popup_allowed_events).

(d) is satisfied just because of the delay before cycle collection: non-user-initiated events count against the dom.popup_maximum limit (assuming the page is whitelisted), but at some indeterminate time after a popup window closes, the penalty is erased by window destruction.  If the popups are infrequent, they will never be blocked in practice.  Yes, I admit I'm doing an about-face since posting comment 60, as I'm now claiming the non-determinism as a benefit ;)

For example, if your bank pops up a "Click here to avoid auto-logout!" window, and you click the button, and you go back to your banking, then by the time another such popup appears, the penalty will certainly have been forgotten.  The delay is on the order of 10-30 seconds.

Given the trickiness of implementing (a), I'd like to file a followup bug.

I'm tempted to do the same for (c).  After whitelisting pandora.com, I've found their buttons perfectly usable.  I'm curious what use cases remain for plugins to override the default behavior.
Attachment #401353 - Flags: ui-review?
Attachment #401353 - Flags: review?(jst)
(Assignee)

Comment 64

9 years ago
Comment on attachment 401353 [details] [diff] [review]
Simplification of PopupControlState logic in nsGlobalWindow.

I should probably also explain that the crux of the new logic is in the switch statement:

+  switch (abuse) {
+  case openControlled:
+  case openAbused:
+  case openOverridden:
+    if (PopupWhitelisted())
+      abuse = PopupControlState(abuse - 1);
+  case openAllowed: break;
+  default:
+    NS_WARNING("Strange PopupControlState!");
+  }

Whitelisting counts for a single reduction in the abuse level (abuse - 1), instead of just granting a free pass (openAllowed).  On a case-by-case basis, this seems to give the desired behavior.  An initial value of abuse >= openAbused corresponds to events not in dom.popup_allowed_events, >= openControlled to events in dom.popup_allowed_events other than "click," and >= openAllowed to "click" events.  I wish there were a more elegant way of motivating this heuristic, but if you play around with 

  http://people.mozilla.org/~bnewman/bugs/260264/winopen.html

the behavior should speak for itself.

The rest of the patch has to do with removing CheckOpenAllow and the OpenAllowValue enum, which turned out to be unnecessary.
Attachment #401353 - Flags: ui-review? → ui-review?(mconnor)
(Assignee)

Comment 65

9 years ago
(In reply to comment #64)
> An initial value of abuse >=
> openAbused corresponds to events not in dom.popup_allowed_events, >=
> openControlled to events in dom.popup_allowed_events other than "click," and >=
> openAllowed to "click" events.

The second two of those three >='s should have been =='s.  My bad.

Very sorry for the flurry of bugmail.
(Assignee)

Comment 66

9 years ago
Mac and Windows tryserver builds available here:
https://build.mozilla.org/tryserver-builds/bnewman@mozilla.com-try-2ba191dacee2/try-2ba191dacee2-macosx.dmg
https://build.mozilla.org/tryserver-builds/bnewman@mozilla.com-try-2ba191dacee2/try-2ba191dacee2-win32.zip

Linux failed to build for no good reason.  My apologies.

Can those of you who have been frustrated in the past try out these builds and report your level of satisfaction?  The URL given in comment 64 above may help test various kinds of events:

http://people.mozilla.org/~bnewman/bugs/260264/winopen.html

In particular, though, I'm interested to hear whether actual applications see improved behavior with the latest patch.

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
(Assignee)

Comment 67

9 years ago
Created attachment 405645 [details] [diff] [review]
OMG TESTS

I may have gotten a little carried away but these tests cover almost everything.  I say "almost" only because they do not currently toggle the preference that turns popup blocking on and off (the first checkbox under the content tab of the preferences dialog).  I'm assuming the popup blocker will be turned on by default.

The crux of the test logic is here:

+/**
+ * Please be patient; run_tests opens/closes a LOT of windows.
+ */
+function run_tests() {
+  hold(popupEvents, "click mouseup", function() {
+    // Note: pm.UNKNOWN_ACTION is the same as pm.DENY_ACTION.
+    hold(popupPrivilege, pm.DENY_ACTION, function() {
+      testIntentional("click");
+      testProbablyIntentional("mouseup");
+      testProbablyUnintentional("mouseover");
+    });
+    hold(popupPrivilege, pm.ALLOW_ACTION, function() {
+      testIntentional("click");
+      testIntentional("mouseup");
+      testProbablyIntentional("mouseover");
+    });
+  });
+
+  hold(popupEvents, "click", function() {
+    // Note: pm.UNKNOWN_ACTION is the same as pm.DENY_ACTION.
+    hold(popupPrivilege, pm.DENY_ACTION, function() {
+      testIntentional("click");
+      testProbablyUnintentional("mouseup");
+      testProbablyUnintentional("mouseover");
+    });
+    hold(popupPrivilege, pm.ALLOW_ACTION, function() {
+      testIntentional("click");
+      testProbablyIntentional("mouseup");
+      testProbablyIntentional("mouseover");
+    });
+  });
+
+  window.open.close(); // just in case
+}

The |hold| function assigns a value (second parameter) to a preference/privilege (first parameter) for just as long as it takes to execute the callback function (third parameter).  No matter what happens during the callback, the preference/privilege gets reset to its original value.  This seemed like a good idea because these tests do a lot of preference manipulation to exercise all the popup blocker cases, and I wanted to be sure everything got undone in the end.
Attachment #401353 - Attachment is obsolete: true
Attachment #405645 - Flags: ui-review?
Attachment #405645 - Flags: review?(jst)
Attachment #401353 - Flags: ui-review?(mconnor)
Attachment #401353 - Flags: review?(jst)
(Assignee)

Comment 68

9 years ago
(In reply to comment #67)
> I may have gotten a little carried away but these tests cover almost
> everything.  I say "almost" only because they do not currently toggle the
> preference that turns popup blocking on and off (the first checkbox under the
> content tab of the preferences dialog).  I'm assuming the popup blocker will be
> turned on by default.

No need to assume this: the general popup blocking preference is just tied to dom.disable_open_during_load, which is easy to force to be true:

 +<!DOCTYPE HTML>
 +<html>
 +<!--
@@ -423,6 +423,7 @@ new file mode 100644
 + */
 +var popupMax = makePrefAccessor("dom.popup_maximum"),
 +    popupEvents = makePrefAccessor("dom.popup_allowed_events"),
++    blockPopups = makePrefAccessor("dom.disable_open_during_load"),
 +    popupPrivilege = function(permission) {
 +      var old = pm.testPermission(ownUri, "popup");
 +      if (arguments.length) {
@@ -528,7 +529,7 @@ new file mode 100644
 +setTimeout(function() {
 +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
 +  check_sanity();
-+  run_tests();
++  hold(blockPopups, true, run_tests);
 +  SimpleTest.finish();
 +}, 200);
 +
(Assignee)

Comment 69

9 years ago
Created attachment 405885 [details] [diff] [review]
Making sure the popup blocker is enabled

Applying the changes from comment 68.
Attachment #405645 - Attachment is obsolete: true
Attachment #405885 - Flags: ui-review?
Attachment #405885 - Flags: review?(jst)
Attachment #405645 - Flags: ui-review?
Attachment #405645 - Flags: review?(jst)
Comment on attachment 405885 [details] [diff] [review]
Making sure the popup blocker is enabled

- In nsGlobalWindow::PopupWhitelisted():

+  nsCOMPtr<nsIDOMWindow> topWindow;
+  GetTop(getter_AddRefs(topWindow));
+
+  nsCOMPtr<nsPIDOMWindow> topPIWin(do_QueryInterface(topWindow));
+
+  return topPIWin && (!IsPopupBlocked(topPIWin->GetExtantDocument()) ||
+                      !IsPopupBlocked(mDocument));

This isn't new in this patch, but since we're changing this code it seems worth making this change that the window in question either doesn't have popups blocked or that any of its parents don't, rather than just checking the top window. Assuming that tests out well, that is.

r=jst with that looked into.
Attachment #405885 - Flags: ui-review?(mconnor)
Attachment #405885 - Flags: ui-review?
Attachment #405885 - Flags: review?(jst)
Attachment #405885 - Flags: review+
(Assignee)

Comment 71

9 years ago
Created attachment 407648 [details] [diff] [review]
Addressing review feedback, adding a frame-nesting test.

The simplest way to re-implement nsGlobalWindow::PopupWhitelisted was to use recursion. I figure that's ok because we'll probably hit other frame-nesting limits before we overflow the stack checking for popup permissions.
Attachment #405885 - Attachment is obsolete: true
Attachment #405885 - Flags: ui-review?(mconnor)
(Assignee)

Comment 72

9 years ago
Created attachment 407677 [details] [diff] [review]
Forgot that window.top breaks iframed mochitests.
Attachment #407648 - Attachment is obsolete: true
(Assignee)

Comment 73

9 years ago
Created attachment 408099 [details] [diff] [review]
Simpler and more defensive mochitests.

Using the window.open(null, win.name) trick to communicate between grandchild and grandparent iframes.

It seems as though gOpenPopupSpamCount is incremented at some point in the preceding mochitests without being decremented, so I can't open dom.popup_maximum windows before being blocked.  The desired behavior can still be tested pretty well, though: in this patch I'm verifying that opening more than dom.popup_maximum windows results in a blocked popup, and that closing windows after a block reclaims the gOpenPopupSpamCount allowance.
Attachment #407677 - Attachment is obsolete: true
Attachment #408099 - Flags: review+
(Assignee)

Updated

9 years ago
Depends on: 523885
Comment on attachment 408099 [details] [diff] [review]
Simpler and more defensive mochitests.

sr=jst as well. Let's get this in!
Attachment #408099 - Flags: superreview+
(Assignee)

Comment 76

9 years ago
(In reply to comment #75)
> (From update of attachment 408099 [details] [diff] [review])
> sr=jst as well. Let's get this in!

After running it past the tryserver one last time, with no problems:
http://hg.mozilla.org/mozilla-central/rev/95c4abb606ee
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago9 years ago
Resolution: --- → FIXED
Depends on: 543528

Comment 78

4 years ago
Created attachment 8499990 [details]
open_test.html Selenium IDE Test Case

I have a similar problem with FF 32.0.1 (Windows)
Popup Blocker is disabled, but after opening the 21st popup, I get a "Firefox prevented this site from opening a popup window"
sometimes everything seems to work OK (all popups are opened as expected), but after a while it starts failing again.
For now it seems to start failing after I used "Clear Recent History", Time range Everything

I use Selenium IDE 2.8.0 with the attached open_test.html as test case to reproduce the problem

currently, my workaround is to set dom.popup_maximum to a large value

Comment 79

3 years ago
I see that this bug is fixed but I noticed that this is happening in FF 38.0.1. Changing dom.popup_maximum works. Is this a new bug or regression of old one?
This is an ancient bug. Please file a new one.

Updated

3 years ago
See Also: → bug 1176510

Comment 81

2 months ago
(In reply to Ilya from comment #79)
> I see that this bug is fixed but I noticed that this is happening in FF
> 38.0.1. Changing dom.popup_maximum works. Is this a new bug or regression of
> old one?

(In reply to Olli Pettay [:smaug] from comment #80)
> This is an ancient bug. Please file a new one.

Did a new one ever get filed?
You need to log in before you can comment on or make changes to this bug.