User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3) Gecko/20030312 Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3) Gecko/20030312 I noticed that the open unrequested windows checkbox has moved to popup windows but it has a different and undesirable action now. It is nice that you can create an exception list but it operates differently than 1.2.1. 1.2.1 suppressed unrequested windows, i.e. the ones that come automatically but did not suppress links. I thought that was the most effective implementation of it. The exception list is a good thing now but I think each website in the exception list should have another extension of permissions depending on the event handler. For example, if you go to a website in which you want to click to get a popup window, the current implementation is good BUT what if that same website has automatic popup banner windows as well? In this case, you want to suppress onload but not onclick. So I think it would be a good idea to have a more aggressive permissions/exceptions window. Reproducible: Always Steps to Reproduce:

Amen, brother! Adding per site capabilities is nice, but I found the 1.2.1 blocking of unrequested popups to be much better for my purposes. If you are going to go to lists of valid (or invalid websites) it would be best if Mozilla differentiated between unrequested and requested popups.

marking enhancement & -> all

Odd. Something tells me that this is an actual bug, rather than an RFE - or else, more likely, it's simply INVALID for reasons explained shortly. Requested popups shouldn't ever be blocked - which is why this would be a bug, not an RFE. (At least, I can't conceive of a circumstance in which anybody would *want* to block *requested*, link-based, popup - nor do I recall that this was how things were ever supposed to work. Although I could be mis-remembering.) > you want to suppress onload but not onclick But what's being seen may not be a bug at all (code-wise) but a case of a link (onclick) calling another function that's doing an onload. In that case, the popup blocking code is considering it to be an unrequested popup (onload), even though it was initiated by user action (onclick). I'm not sure how this could be addressed - perhaps it can't be and it's just an unfortunate fact that will have to be lived with. (Or taken to Evangelism on each specific site that does this.) This may be related to (or a dupe of) bug 187904, bug 181877, bug 174507, or bug 155351 (the testcase in the 1st one is WFM, and the remaining 3 are marked as camino specific but may be broader). It would be nice to see a testcase of a requested popup that's being blocked. Logic tells me that this shouldn't be happening - although I know I've experienced it occasionally. (Again, it may be that the coding of the site is designed in such a way that an onload function is being called (and blocked) as a result of the user action.) Most recently, it was with a friend of mine using my computer to check his Web based Netscape email. I had to disable popup blocking in order for him to do this properly. Unfortunately, I didn't take the time to properly document what was happening (either that time or in the few other times it happened to me) in such a way that I can now make it into an actual testcase. -> Confirming. -> XP Apps: GUI Features CCing danm who actually wrote the code and can give a definitive answer. But until we get a testcase, as opposed to all of our we won't know anything for sure. Incidentally, if onclick is NOT being blocked by the popup code (I don't think it is), then the specifically stated intent of this bug could make it INVALID - unless it gets morphed into something that might address the onclick -> onload issue, if that's technically feasible somehow.

Oops. Meant to say: But until we get a testcase, as opposed to all of our "urban legend" reports, we won't know anything for sure.

The specific site that I've noticed the problem for was www.gartner.com (Which redirects me to www4.gartner.com/Init) I hadn't been to that site before I installed 1.2.1, but I hadn't noticed a problem with a requested popup not occurring until then. In any case the text of the pop-up window preferences pane makes it sound like all popups no matter whether requested or not get blocked. If that is not the intent that that pane needs to be revised.

I created a test page for many possible different popup scenarios but unfortunately I was not able to reproduce a requested window popup being denied. Although I did find two new problems. The onload window on this page throws a js error. When this window is suppressed, it might be a good idea to exit the js gracefully. Also, the onFocus of an input field seems to execute more than once when focusing on the input field only once. In relation to the loop window, perhaps a good enhancement might be to have a number limit input for the number of windows that can popup as a result of the action of an event handler.

I think I finally figured out what was causing the popup window not to pop up. If you click on a popup link or any link for that matter before the page is finished loading, the page stops loading and the popup link ceases to function. I can reproduce this every time and it is most easily reproducable on a page that takes a little while to load.

specifics? www.gartner.com is full of links. I don't want to have to try them all. Some of the links open windows. I've found none whose window is considered a popup by Mozilla. That said, as Jason said (comment 3) sometimes Mozilla considers a window a popup when the user wouldn't. www.carsdirect.com is my favourite example: their script is coded so the window answering your click on a link requesting details on a car option is opened in a timeout. Mozilla considers such windows popups. We could loosen the rules to let these by but I think we won't. This is the primary reason for supporting exceptions to a general "no popup" policy. It does mean the user has to allow the entire domain to open popups. Luckily I haven't (yet) found a site that's coded to use popups and also abuses the privilege. I'm thinking this bug is invalid. If you've found some kind of page loading fault (comment 7) that might be a different story. But if so you should start a new bug. (Nice Popup Tester attachment, by the way. It should be the subject of separate bugs detailing which types of popup we don't block. And note that there already are a few such bugs. Bug 126224 and bug 187255 for instance. There isn't one for mouseovers though, I'm pretty sure.)

Yeah, going back to www.gartner.com, I confirmed the behavior described in (comment 7) . The page does take a while to load and I clicked in the news link that I wanted before the page had finished loading. As per the suggestion in (comment 8) I've filed a new bug report (Bug 198319)

Then I'm closing this one invalid and taking that new bug. I encourage you/somebody/mjs15451 to do the grungy bugzilla search for unfiled bugs covering popups we don't catch and make new bugs using that fine popup tester attachment.

.

I still think an enhancement should be considered for window popup permissions. Perhaps in an advanced tab so as to not confuse users. Granted, there may not be too many websites which abuse popups and use them legitimately, it would be nice to control permissions within a domain in case that is ever encountered.

One good example of this would be the mouseover event handler. It is quite easy to abuse this event handler in such a way that it brings up an "unrequested" window.

I would like to state that despite my original example of this bug is somewhat inaccurate and has spawned Bug 198319, the original intent of this bug is stated in the summary as a needed enhancement. We need to have two modes for popup windows: Simple and Advanced. The Simple mode would assume certain windows as "unrequested" and the others "requested". The Advanced mode would allow the user to pick and choose which popup windows they will accept to not have to worry about certain problems cropping up as a result of what I stated in Comment 13.

Comment on attachment 117613 [details] Popup Tester As per my original comment on the top of the attachment, it seems the it will depend on the developer to check to make sure the popup does not return null before they try to focus the window. Also, avoid clicking on the onFocus example. Bug 58441 has been filed to deal with this problem.

comment 0: onload windows are popups. onclick windows aren't. The rest of the initial comment I don't follow. comment 13: mouseover windows are popups. The current codebase doesn't recognize that, is all. Attempting to clarify your proposal, I gather that it's a popup suppression UI fine-tunable by event type: --------------------- for domain | jiggy.noise.org | --------------------- suppress popups from [ ] script timeout [ ] onload/unload [ ] onfocus/blur [ ] mousedown/up [ ] mouse movement [ ] clicking buttons Or perhaps with even greater detail; I'm not certain. Hmmmmm. OK. Personally, in practice I don't see myself wanting to put this much thought into popup suppression. --------------------- domain | jiggy.noise.org | --------------------- [ (*) is ( ) isn't ] run by abusive jerks is about all the time I want to devote to any site. I think Mozilla as an organization is leaning my way: a movement to simplify the entire UI is gaining momentum fast. Still, I'll let the UI tech honcho worry about this. Reassigning.

As per comment 16, that is exactly what I am proposing. I understand that things need to be simplistic, that is why I proposed it as an advanced tab so the average user wouldn't worry about it unless they absolutely wanted to. But if you are a web developer like me, you like having the option to make things complicated when needs be. :-) Good one about the is or isn't run by abusive jerks! ;-) If only computers could read your mind? In the mean time, I think having a list of popup event types to suppress on a domain basis is a good idea for advanced users.

I guess what it comes down to is what is your level of tolerance for annoying popup windows? I think it is up to you (the person using the browser) to decide what your tolerance level is. Currently, unrequested popups only deal with automatic popup windows where no user action is required to make them popup. How annoying would it be to have a new popup window come up every time you move the mouse on the webpage or just to mouseover something? Better yet, how would you like it if every time you click on a link or focused on a form field, 10 advertisement popup windows appear? The simple user may not pay attention to such options or may not even know they exist but for the advanced user, it is a good tool to have. I think one of the main things that is so appealing about Mozilla as compared to M$IE, is the amount of options it offers. Kudos to the all the Mozilla developers/contributors who thought of and implemented all these great options!

Not likely to happen. I suggest you vote for bug 176564 (Should be able to manually open a blocked pop-up) and bug 159036 (pop-up blocking should use a whitelist of allowed [events in which] to open a window rather than a blacklist) instead.

As per Additional Comment #19, that is not what this enhancement is all about. I understand that as of 1.4, there is a whitelist implemented for allowed websites if you have pop up windows disabled but I am talking about coders who abuse pop ups and how the current implementation is not enough. Certain event handlers can emulate an unrequested popup. Clicking on a normal link could open up several pop up windows, just moving your mouse over a certain area of the document maybe even a transparent image could open up a dozen popup windows. As far as I'm concerned, where there is a will, there is a way. Spammers always seem to find new ways past your filters don't they?

Just to note, the popup tester that I wrote is a good example of how popups can still be abused.

I think this implementation can be simplified by just having two things: [x] (boolean) Suppress non mouse click event handlers [3] (integer) Number of popups allowed open at one time Both options would make sense and would be easy for the end user to understand.

I agree we're not catching all the types of event that can open popup windows. It simply hasn't been coded yet. The popup suppressor is surprisingly simplistic as it stands. Your idea of a limit to the number of popups is interesting. Practically speaking I think bug 176564 is a better solution, if that's the same problem you're trying to solve. But a limit in any case to prevent sites from opening popups in an endless loop (like http://www.catholicninjas.org/superfuntime/) would be useful.

As per comment 23, you do have a point. Bug 176564 does seem to cover the problem of popup limiting. I guess the only thing left in which this bug applies to is to implement more advanced popup supression to cover abuse through other additional event handlers like onClick and onMouseOver. Perhaps there should be levels of suppression somewhat like the cookie privacy settings?

At this time the only windows we consider a popup are those opened while the document is loading (or unloading) or a timeout function is running. This patch extends popup control to windows opened under a variety of event handlers (load/unload, "timer" (not a real event), change, focus/blur, mousemove/up/down/over/out/click, keypress/up/down). Notably missing are the form events -- those sidestep the EventStateManager, and I haven't figured a good way to notice them. However, the original click or keypress event that caused the form submit is available, so form popups are still controllable under the guise of the mouse/key events that caused them. The list of events under control defaults to load/unload and timer and I would leave it that way. click events in particular are wishing for some abuse control (e.g. bug 227338) but I suspect click events also occasionally open legitimate windows, as timers do. Though I haven't really been able to find a live example. Still for full-bodied popup control enjoyment, this patch would be partnered with the Mozilla/Firebird bug 176564 / bug 198846 pair, and bug 222763 comment 12. There's no UI in this patch. That may come later; perhaps an "Advanced" button in the already huge prefs panel. Maybe never in Firebird, since this sort of thing is antithetical to Firebird's UI philosophy. This is a geek feature that I don't relish trying to explain to a novice user. To use the feature without a UI, apply the patch and add this line to your prefs.js file: user_pref("dom.disable_open_during_these_events", "load timer change focus blur keydown keypress keyup "+ "click mousedown mousemove mouseout mouseover mouseup"); except, you know, all on one line as entries in prefs.js normally are. I don't envision enumerating all those options in the UI. I was thinking perhaps an Advanced prefs panel like so: --------------------------- no steenking popups on [x] load/unload [x] timer [ ] mouse move [ ] mouse down, click [ ] key [ ] focus, blur [ ] change ---------------------------- but at least the code allows finer control. There's an additional feature. Add this pref: user_pref("dom.disable_open_repeat_interval", 500); and it'll refuse to open additional popups within the given number of milliseconds since the last popup it opened, even if you have popup blocking turned off. The idea here is to put a damper on abusive sites that open popups in a loop <body onload="for (i=0; i<100; i++) open(hahahahahaaaha)"> This won't shut such a thing down completely but it'll slow it down and give the user a chance to regain control. I propose shipping with that preference set. Potential patch problem: I've taught nsEventStateManager to keep track of the current event being processed. Is this evil? I don't know! It seems like something an event state manager could reasonably be aware of. Unfortunately I wasn't able to teach it to keep track of this thing completely internally; it has to have its hand held by the caller (nsPresShell). Ah well. Critique away.

Why are you extending the list of disallowed events instead of switching to a list of allowed events (bug 159036)? Keypress events involving the Enter key should allow pop-ups. I'm not sure I like dom.disable_open_repeat_interval. If I have pop-ups set to open in new background tabs, and I click two JavaScript-new-window links quickly, it seems like that feature would cause the second "pop-up" to be blocked even though it's associated with a separate user click.

1) Allowed events vs disallowed: Huh. Didn't know about that bug. And by the way neither, I promise you, does the person to whom it's assigned. I would have been a more likely choice. However (what that bug calls) whitelist, blacklist ... who cares? The distinction is important in the permission manager because its data set, the set of all possible domains, is unbounded. Events are a finite set, and either will work fine. That said, I prefer this implementation. (1) It's written. (2) The "black list" is a natural extension of the current (load/unload/timer) implementation, which mostly works. (3) Certain event types (form submit, for example) are hard to track. A white list, whose default permission is "no," would be more error prone. Oops. Missed that one. Blast. Don't know how to get to that one. Mozilla's default setting has always been permissive: popup blocking is shipped turned off, though easily activated. I agree with that sentiment. (4) If I were to change this to a whitelist, the patch would read exactly the same but instead of PRBool abuseMatch = PR_FALSE; ... case NS_FOCUS_CONTENT: abuseMatch = PR_TRUE; it would read PRBool abuseMatch = PR_TRUE; ... case NS_EVENT_WE_LIKE: case NS_ANCHORCLICK_EVENT: abuseMatch = PR_FALSE; so I could spend time trying to track down all the events we don't want to block, hope I didn't miss any, and use that knowledge to implement what I really think is a pointless semantic argument. Or not. By the way there is no such thing as an NS_ANCHORCLICK_EVENT. In fact, there's no DOM event for that at all. It's a PL Event, which uses a completely different mechanism, and I'd rather not go there if I don't have to. Which I don't, with my "black list." 2) keypress enterkey should allow popups Can you give an example? Note that in normal link operation the enter key while the link is focused causes a link click PL event to be synthesized, so that isn't affected by this patch. I think I want to control the enter key as well as any other, but I'm ready to reconsider. 3) dom.disable_open_repeat_interval Something like that is sorely missing. I think this is a decent approach. Remember that with my suggested default preferences, click popups are not suppressed unless the user takes steps to do so. If you're already in the Advanced Prefs suppressing click popups, you can bump down the interval pref, too. I think this could only be a problem in the exact scenario you describe, which can't happen in an unmodified Mozilla. If the popup opens in a new window as it does out of the box, you'll be hard pressed to get at the offending link to hammer it so quickly. Still, it might make sense to ship with a default interval more like 1/10 sec. However, we need something along those lines.

Wow! Thanks Dan for implementing this feature. All of us web developer geeks will be happy. :-) This is exactly what I was looking for. I'm going to have to patch my mozilla to give it a whirl. Thanks again!!!

> A white list, whose default permission is "no," would be more error prone. If you have a blacklist of events, you have two options: 1. Blacklist every event that isn't on a "mental whitelist". This is harder to maintain than a whitelist but no different in function. 2. Leave out obscure events. As sites abuse increasingly obscure events, you have to add those events to the blacklist, and you end up back in situation 1. I think a whitelist makes a lot more sense.

Perhaps you're right. This version disallows popup windows if it detects any DOM event being processed at all. It allows for prefs-driven exceptions for (click dblclick mousedown mouseup select change input keydown keypress keyup reset scroll submit error resize) events. I'm worried about this; it may be too draconian. What about mutation events, for example? But what the heck. Popups suck. I've also discovered how to catch form submit and reset events as themselves, so this patch no longer treats them as the underlying mouse or key event. And I've ditched the previous patch's rather incomprehensible popup timeout limit in favor of a limit to the maximum number allowed open simultaneously. Note to users & testers: There's still no UI: control this with prefs for now. Pref settings mentioned in comment 25 have been replaced by these: user_pref("dom.popup_maximum", <integer>); user_pref("dom.popup_allowed_events", <space delimited list of exceptions>); note these are now the list of events which *are* allowed to open a popup. A reasonable shipping default may be user_pref("dom.popup_maximum", 2); user_pref("dom.popup_allowed_events", "click dblclick change error reset submit"); though personally I plan to have none. Notes to reviewers (or, what I did that's scary): Changes to nsPIDOMWindow.h and nsIEventStateManager.h. Are popups ever disallowed improperly? The count of open popup windows isn't decremented immediately upon closing a window, but only after the JS garbage collector has run. nsEventStateManager maintains a sort of stack of events being processed. This is primarily maintained by its caller, nsPresShell. ESM also makes note of internally synthesized events such as mouseover. nsGlobalWindow::HandleDOMEvent internally maintains a stack of its own current event. This supersedes ESM's event. Because I swear every third event has its own way of being invoked, handling is still a little inconsistent. Mutation events for example sometimes show up as themselves, sometimes as the mouseclick that caused to run the script that caused the mutation, and sometimes as stack overflows. But the core events we really care about all seem well behaved and I'm not sure I care about the weird outlying events. I've been playing with this popup suppressor for several hours and it seems pretty hale.

Like the last patch, but with four flaws fixed. I feel certain I can just replace the old patch without setting any reviewers back. Again, this one seems to work well. Better than the last one, even. Remaining unanswered are questions of whether I should be keeping event state as I am, what problems this may cause, and whether I should be locking out all event handlers not explicitly mentioned. Mutation events, say.

About the beneficence of this patch, and a follow-on to comment 25: After doing a nearly scientific survey of a handful of somewhat randomly chosen websites as well as some from the tripod domain (see bug 230500), I'm less pleased with the popup solution in this patch. I'm surprised how common clickhandlers in links are. Survey says: ----------- popups ----------- site click a-click advertising survey information www.amazon.com* o o - www.cnn.com o o x x www.flutterby.com www.godamongdirectors.com* www.pricegrabber.com** www.randomwebsite.com o o - yahoo mail* o o - www.zippygirl.org/zippylog o o x www.tripod.lycos.com -x -- tripod sites -- attacked911.tripod.com x classroom.tripod.com darrenlondon.tripod.com x jeff560.tripod.com other_israel.tripod.com x proclus.tripod.com x samvak.tripod.com trainland.tripod.com * - including a sub-page or two ** - including the specific subpage mentioned in bug 159036 comment 13 In the first two columns an "o" means this site does indeed have click handlers or click handlers in anchor links, respectively. This is just informative. The patch in this bug doesn't suppress click handlers, it only suppresses window.open in click handlers. It turns out link click handlers are really quite common. Color me surprised. The last three columns are all popup windows opened in click handlers, and are all affected by this patch. The first two popup columns, advertising and survey, are arguably annoying popups that we'd like to suppress. The last column denotes a clickhandler popup that really is the main subject of the HTML element to which it belongs. An "x" means the information in the popup is available only from the popup. A "-" means the information is available even without the popup. Generally that was an element like <a href="<some url>" onclick="open(<some url>)"> which tries to open a new window but settles on navigating the current window if it was unable to open a new one. Summary: it's only the 'x's we care about in the above chart. Informational xes are fairly rare but they do exist. Advertising xes are common in what I believe to be a small subset of the web. We should probably still implement this patch, but certainly not ship with click handlers disabled by default. This patch is of limited utility without some kind of extemporaneous override (see bug 222763 comment 12).

In the above comment's summary I of course meant that window.open in click handlers should be enabled by default, and other aspects of click handlers in general should remain untouched. Further, popup suppression in click handlers is a kind of loss-of-data problem, and the UI for activating it should be an advanced pref. Also, suitable extemporaneous overrides also include bug 176564 / bug 198846.

*** Bug 232115 has been marked as a duplicate of this bug. ***

Comment on attachment 139281 [details] [diff] [review] more general popup protection - In GlobalWindowImpl::CheckForAbusePoint(): ... + case NS_MOUSE_LEFT_CLICK : + prefMatch = eventPrefStr.Find(" click") >= 0 || // dblclick + eventPrefStr.Find("click") == 0; + break; + case NS_MOUSE_LEFT_DOUBLECLICK : + prefMatch = eventPrefStr.Find("dblclick") >= 0; + break; ... This string matching code could be a bit more robust IMO, how about using something like this: IsEventNameChar(const char c) { return isalnum(c) || c == '_' || c == '-'; } static PRBool ContainsEventName(const char *eventName, const nsAFlatCString& events) { nsAFlatCString::const_iterator start, end; events.BeginReading(start); events.EndReading(end); nsAFlatCString::const_iterator startiter(start); while (startiter != end) { nsAFlatCString::const_iterator enditer(end); if (!FindInReadable(nsDependentCString(eventName), startiter, enditer)) { // No match return PR_FALSE; } if ((startiter == start || !IsEventNameChar(*--startiter)) && (enditer == end || !IsEventNameChar(*enditer))) { // Found a match that's not surrounded by valid name characters. return PR_TRUE; } // Move on and see if there are other matches. startiter = enditer; } return PR_FALSE; } Then you could do something like this, and it would work more reliably: ... + case NS_MOUSE_LEFT_CLICK : + prefMatch = ContainsEventName("click", eventPref) + break; + case NS_MOUSE_LEFT_DOUBLECLICK : + prefMatch = ContainsEventName("dblclick", eventPref) + break; ... and you wouldn't need eventPrefStr at all, you can use the eventPref string directly. either way, sr=jst

Oh I *do* take code submissions :-) It's checked in with jst's suggested changes, slightly modified. I wanted to be less forgiving; I'm insisting on a space-delimited list. Default prefs are as in comment 30. So from now on users should be protected from mouseover popups and the like, as well as abusive loops that open a zillion popups. It's still wanting a UI allowing users to tweak the new prefs. Ben and I have an idea but it requires a new control type. I'd expect something for 1.7 beta. For now, power users can fine-tune their settings by visiting their prefs in text form.

Hrm... this seems to have affected Tp a bit (about 0.5%, so maybe it's noise, and having this feature is awesome).

Also, shouldn't "dom.popup_maximum" default to 0, not false?

btek does seem still to have lost about .1% on Tp, though monkey and creature are about the same. That's odd. I can't imagine why this would affect Tp at all. Txul, possibly (though before checking in I measured the last patch (which was after all slightly different from what was checked in) and saw no measurable effect on Txul). And yes, popup_maximum is supposed to be an int. Thanks. Fixed it. (It's a hard limit on the number of popups you can have open simultaneously, whether or not you have popup controls enabled. The idea is to contain the damage when visiting obnoxious sites with window-opening loops. I set it to 2, not 0.)

The only guess I could make about the regression is the extra overhead per event dispatch, but that shouldn't be _that_ much. Also, I assume none of this code gets hit for the new DOM window created for an iframe...

Hmm... I have some issues with this patch: 1) CheckForAbusePoint is now called twice on every window.open(). Is this desirable? (Though I guess that happened before the patch too...) 2) using delayPref for the "max number of popups to have open" is confusing 3) (serious) the max number code will block _all_ window.open calls once the max number of popups is reached. As a result, perfectly legitimate window.open() calls in totally unrelated browser windows will fail. At the moment, that will happen when there are two popups open (which will take about 30 seconds, if browsing with popup blocking disabled). Perhaps that counter needs to be per-window instead of global? 4) This new code (the event code and the total-number-limiting code) runs no matter whether popups are disabled or not. Is that desirable? 5) There is no way to turn off the total-number limiting code, as of this morning. There are controlled environments (eg intranets, custom Mozilla applications, etc) where it may be desirable to disable it. I'm not sure why that patch landed without getting review from jst first...

(In reply to comment #41) > Hmm... I have some issues with this patch: > > 3) (serious) the max number code will block _all_ window.open calls once > the max number of popups is reached. As a result, perfectly legitimate > window.open() calls in totally unrelated browser windows will fail. At > the moment, that will happen when there are two popups open (which will > take about 30 seconds, if browsing with popup blocking disabled). > Perhaps that counter needs to be per-window instead of global? Your point has merit, but having the counter being per window won't quite cover what needs to be done when trying stop abusive pop-up windows. How would one stop a pop-up window from itself calling a pop-up which then called a pop-up, etc., if the limit is per window? Per chain of windows is probably a good solution with the counter associated with the root window. That would avoid the global effects you don't want, while preventing a recursive string of pop-ups that I don't want.

1) No, just once. 2) Are you looking at the first patch? The one checked in uses a count, not a delay. 3,5) Quite right. Sleeping at the wheel. The attached patch fixes this. BTW jst did review the last patch. I guess there'll be less trust for me in the future. 4a) I can't think of an efficient way to turn off the event code. Besides, that doesn't seem to be the Tp problem. 4b) The popup limit isn't very useful unless it's active even if popups are enabled. But you're right, it's premature without a UI to control it. 5) see point 3. (Also comment 39: it's more like 1% on btek. 901.1ms from 8 before and 909.5 from 9 after. Still trying to track that down.)

(In reply to comment #43) > 2) Are you looking at the first patch? The one checked in uses a count, not a > delay. I was looking at the reviewed patch. It has: + // limit the number of simultaneously open popups + delayPref = 0; + gPrefBranch->GetIntPref("dom.popup_maximum", &delayPref); Which is indeed not what got checked in. <sigh>.... > I guess there'll be less trust for me in the future. I sorta doubt that; happens to all of us... Have I mentioned that this patch kicks ass overall, btw? ;)

Comment on attachment 140520 [details] [diff] [review] raise max popup limit and release non-popups from its control >Index: dom/src/base/nsGlobalWindow.cpp >@@ -3057,23 +3059,18 @@ GlobalWindowImpl::CheckForAbusePoint() > if (delta/1000 > (PRUint32) intPref) >- return openAbuseOverride; >+ abuse = openAbuseOverride; > if (mRunningTimeout) >- return openAbused; >+ abuse = openAbused; So if a timer is running after the click delay we'll downgrade the open to openAbused? That seems uncalled-for. We can probably continue to return openAbuseOverride from the click delay code... >+ abuse = prefMatch ? openControlled : openAbused; OK. So if I click on something with an |onclick="window.open(whatever)"|, I get an openControlled, right? >+ // limit the number of simultaneously open popups >+ if (abuse == openAbused || abuse == openControlled) { How does this patch help then? All click-triggered window.open()s (which are the only ones we ever really want opening windows anyway) are still treated as popups by this code for purposes of the dom.popup_maximum pref, right? Or am I missing something?

Yup, good eye again. I'll need to do @@ this instead // is a timer running? < if (mRunningTimeout) < abuse = openAbused; -- > if (abuse == openAllow && mRunningTimeout) > abuse = openAbused; @@ and this // is the document being loaded or unloaded? > if (!mIsDocumentLoaded) > abuse = openAbused; -- > if (abuse == openAllow && !mIsDocumentLoaded) > abuse = openAbused; The main thing this patch does is let a window.open with no associated event (such as <a href="x.html" target="_blank">) get through even if the popup window count is too high because abuse will remain openAllow throughout. You're wondering who should win when an allowed popup (e.g. a click event, by default) fights with gOpenPopupSpamCount. You're thinking the allowed event should be dominant; that the user has deemed it is not a popup. I'm thinking the limit is dominant; that the user has allowed it, but it's still a popup. Both interpretations make some sense to me. But to be useful, the hard popup limit must apply even if popup suppression is otherwise disabled (which is why I'm weakening it in this patch until there's a UI). I chose to go with the dominant popup limit interpretation because it seemed to me to fit best with its other role, where it's also dominant.

The problem is that from my pov there is no difference between <a href="x.html" target="_blank"> and <a href="javascript:window.open('x.html')"> and <a href="x.html" onclick="window.open('x.html'); return false;"> They behave exactly identically from the user's perspective, they behave exactly identically from the site author's perspective, they are _implemented_ pretty much identically in our code (the first of these calls window.open(), as you know) and far too many sites use the second or third version when they could just get away with the first one. This is why I'm opposed to treating them differently. Perhaps the solution is to simply treat click events as "not an event" and to immediately bail out of the event code if we are looking at a click event (and maybe an enter press event)? I agree that the other events constitute popups, but windows opened as a result of user actions that could trigger a target="_blank" link should not be considered popups, imo. I agree that this is all a little less relevant with the much higher limit, but I would actually prefer to get this right and lower that limit to 10 or so by default... The other changes look good.

*** Bug 159036 has been marked as a duplicate of this bug. ***

I do want to draw a distinction between href and onclick. That's what makes this patch a useful advanced user's solution for bug 227338 and bug 230500. But I agree that novice users would expect a link to behave the same, whether it happened to be coded as an href or an onclick. Making the event classification dominant (comment 46) would help. But on second look, I don't like that. It ripples through the code and makes openControlled pointless. That means that all events that are allowed to open popups aren't subject to the popup limit. The other solution, as you said, is to give click special treatment. Alright. New patch.

The code should not take the total number of open pop-ups into account when deciding whether to allow a pop-up. It should only limit the number of pop-ups when a single event opens a bunch of windows at once. Clicks should not be special in not being subject to a limit on the total number of pop-ups. That would just create obscure but annoying bugs ("new-window links on this site only works the first 20 times") without making it any harder for malicious sites to open pop-ups.

Comment on attachment 140559 [details] [diff] [review] max popup limit re-think, and special treatment for click handlers Yeah, ok. I like that a lot more. r=bzbarsky, though it would be great to also handle enter keypresses like clicks... but that can be a separate patch, I guess.

> That would just create obscure but annoying bugs ("new-window links > on this site only works the first 20 times") How would this situation arise?

> > That would just create obscure but annoying bugs ("new-window links > > on this site only works the first 20 times") > > How would this situation arise? If the site used a weird but legit event for its new-window links (onsubmit, onmousedown, onselect), or if the user hit Enter instead of clicking.

Jesse (comment 50) say, this is turning into a hot bug >The code ... should only limit ... pop-ups [on] a single event Perhaps, though that would open us up to the abuse mentioned in comment 42. >Clicks should not be special But they are special, for historical reasons. I've come to agree with Boris that novice users would be less confused by special treatment for click. Without special treatment, a default installation would open (once the lower popup limit is back in place with a UI) no more than two windows to click handlers. If they're legitimate, imagine the obscure bug that'll cause. Boris (comment 51) You're worried about what happens when the user focuses a link and hits the Enter key, right? That's dispatched through a separate mechanism (see comment 28 (2)). So <a href="x.html" target="_blank" onkeypress="openOnEnter(event, 'y.html')"> attempts to open both x.html and y.html. Once through a keypress event which we do catch, and once with no DOM event at all, which completely bypasses this code. We're good? Or do you know that onkeypress is a common, if puzzling, coding style much like onclick? Anyway, patch 140559 is in. Some of its fixes are important, though parts may still be contentious.

> attempts to open both x.html and y.html. That's because the keypress did not return false... ;) You're right; I wasn't thinking. The enter press wouldn't have triggered onclick event handlers anyway, so there is no reason to treat keypress specially here, imo.

>>The code ... should only limit ... pop-ups [on] a single event >Perhaps, though that would open us up to the abuse mentioned in comment 42. If we're open to that abuse when the user has disabled unrequested pop-ups, then pop-up blocking isn't working in the first place. Is there something on the whitelist that shouldn't be, or is the purpose of the limit to save users who have enabled unrequested pop-ups? >>Clicks should not be special >But they are special, for historical reasons. I've come to agree with Boris >that novice users would be less confused by special treatment for click. All events in the whitelist should be able to open at least one window per event (for the same reason that onclick should be able to), whether pop-up blocking is enabled or not. That's what I meant by "clicks should not be special".

>If we're open to that abuse when the user has disabled unrequested pop-ups, >then pop-up blocking isn't working in the first place. True enough. But I'm also worried about users with popup blocking disabled. The limit is still active even then. Obviously it needs a UI and its initial value should be chosen with care. I say 2. The limit is, as you've noted, mostly useful for loops in single windows. The comment 42 edge case would be weird to see and (hopefully) wouldn't hit anyone who knew to turn on blocking. But why knowingly expose ourselves to it? Coding the limit against individual windows would be harder and I don't see much advantage, while I do see a disadvantage, though admittedly an obscure one. >All events in the whitelist should be able to open at least one window per event It's not coded that way now. And I'm not sure I agree. Personally I don't want even one popup. I suspect you're thinking of some of the cases covered by the IE popup blocker mentioned in bug 159036 comment 29. It's true they handle some things better than we do, with this patch. Currently Mozilla either allows new windows in click handlers, or doesn't. It would be nice to accept one new window from a button click handler and none from a bug 230500 click handler. We're not there with this patch. A good solution for that will probably require we be aware of the event target, as well as the type. This would be rather more trouble and in any case should probably wait until bryner has landed a small rearchitecture he's thinking of. That's probably worthy of a new bug. I'm less anxious about that, though, than the array of cases covered by this patch.

Comment on attachment 140559 [details] [diff] [review] max popup limit re-think, and special treatment for click handlers sr=jst, fwiw

See bug 232933 for a possible regression.

*** Bug 152167 has been marked as a duplicate of this bug. ***

*** Bug 233377 has been marked as a duplicate of this bug. ***

*** Bug 235462 has been marked as a duplicate of this bug. ***

*** Bug 234726 has been marked as a duplicate of this bug. ***

I'm using 1.7a (Build: 2004021913) now and I must say the event handler allow list works quite well but I noticed in this build that the dom.popup_maximum is not having any effect when I change the number to a lower number. I changed it in about:config to 2 right now and to test, I used my Popup Tester on the "onClick open for loop 10 window popup" link and all 10 windows pop up even though I have the maximum set to 2.

We decided to special-case click event popups. See comments 45-49, especially comment 47. The popup limit applies to all windows that are determined to be popups, but click event windows aren't considered popups at all, if |click| is an allowed event in your popup_allowed_events pref. click windows can be controlled by excluding them from that pref. Otherwise, they slip completely under the radar. Similarly uncontrolled is a window opening loop in a normal link <a href="javascript:goNutsWithWindows(1000)">dare you</a> Perhaps we should (instead?) just put a limit on the number of new windows that can be opened for any reason within a certain time limit.

Hmm, a limit on the number of windows opened in a limited ammount of time sounds cool, that could be used to prevent death using while (true) alert("You're screwed!"); too! IOW, this limit should not be specific to popups at all, any window should count.

I have an answer to comment 47. They're _not_ the same. window.open can abused. target="_blank" cannot. You can't loop a target but you sure can loop window.open(). I don't really understand how |click| events aren't considered popups if they use the same window.open code as onMouseOver does. As evidenced by my Popup Tester, this can still be abused. I understand that you can disable click events but thing about click events is that loads of legimitate sites (including the ones I program) use click events to open a new window. So here we have a problem, yes it's nice to disable click events for popups (or perhaps new windows is a better term?), but it kills popups on legitimate sites. If I keep it enabled for legitimate sites (like mine), then the evil websites will still have a way of getting through. I don't think click events should be treated the differently. The dom.popup_maximum should apply to _any_ event that is not disabled including click events. I don't think the time limit idea covers this problem either. Creating a time limit, just makes the evil websites do this instead: function openWin(){ window.open('blah.html'); setTimeout("openWin()", theDefaultMozillaTimeLimit+1); } openWin();

> window.open can abused. target="_blank" cannot. You're joking, right? > You can't loop a target Yep, you must be joking. Or ignorant. Or both, I suppose. I recommend looking at the way nytimes.com poses its popups, if nothing else. > The dom.popup_maximum should apply to _any_ event The fact that you say this while wanting to use window.open and expecting it to work tells me that you don't understand what that pref does.

Any maximum number of pop-ups should be per-click (or per-event) rather than per-time-window. If it's based on time, it will affect users' attempts to open lots of links at once. It's not uncommon for me to open 25 tabs in 10 seconds, so you would have to allow at least 3 pop-ups per second if you used the time-window approach.

tabs are not windows, I'm talking real top-level windows here.

I can open 17 links in new windows in 20 seconds, and someone with a faster computer could probably double that.

As per comment 68 I admit that I might be ignorant but pointing me to the nytimes.com and saying I'm wrong doesn't really prove me wrong. Show me a testcase where <a href="blah.html" target="_blank">some link</a> can open a new window 10 times? It's not using any script. This is what I meant. Ok this is supposed to be an enhancement bug so it's still open to discussion as to how to successfully implement this enchancment. I agree with Jesse Ruderman in that dom.popup_maximum should apply to click events as well. This is how I understand dom.popup_maximum: If onMouseOver is an accepted event for opening a new window (or popup), then in one successful execution of that event, you are only allowed to open dom.popup_maximum number of windows.

Should we perhaps make yet another new variable since click events are "not the same" as every thing else? Maybe dom.click_event_maximum_num_new_windows [int]? I'm just trying to say that as per comment 65, yes click events _if_ they're enabled can still be abused to open new windows and _lots_ of them.

> pointing me to the nytimes.com and saying I'm wrong doesn't really prove me > wrong. I was somewhat hoping you'd actually make an effort and look at their code, but I see that was too much to hope for... They open popups by calling submit() on a <form> with target="_blank". Perfectly scriptable in whatever loops you want, and capable of opening as many windows as desired, all without window.open(). > Show me a testcase where <a href="blah.html" target="_blank">some link</a> can > open a new window 10 times? See attachment 141473 [details]. That just opens a single window, but it's trivial to change the script to open a lot more, of course. Again, all without window.open(). > This is how I understand dom.popup_maximum: That's not at all what the pref does, as discussed earlier in this bug.

Ok as per comment 74, you got me there. I have never seen a window opened like that before but you still used script to open it. You did bring up a _good_ point though. Your attachment 141473 [details] emulates a click event without clicking yet again demonstrating why click events need a limitation on the number of new windows opened by it. Can we implement yet another variable that can limit the number of new windows opened with click events as per comment 73?

> Your attachment 141473 [details] emulates a click event without clicking Note that in my opinion we should be detecting that as a "fake" click event and not treating it the same as "real" click events for security purposes not restricted to popup blocking....

We omitted click popups from the dom.popup_maximum control because click is so often used legitimately to open new windows. Mixing the two could cause legitimate anchors to malfunction. But of course this is true even of a limit which applies only to click windows. I can see someone wanting to open a bunch of windows simultaneously. My favourite example right now is http://www.ikwarmachine.com/minis.php. (Note that site seems to be moving away from click handler links toward href links. At the moment it's a mixture.) Also, it makes equal sense to limit the number of windows opened from href events (comment 74). I feel we've made the solution over-complicated, controlled by so many interlocking prefs, whether it's (popup_maximum, click_maximum, href_maximum) or click & href combined into, what, potentially_legitimate_maximum? The notion of time limit control has its detractors (comment 26, comment 71), and we decided to not take that route with the last patch (comment 30, para 3). But it seems more usable to me than a hard limit. I expect a user to be surprised by a hard limit more readily than I expect him to be disappointed by being tripped up in a window opening race (comment 71). The best solution is probably to have a hard limit on the number of purely script generated windows (comment 76). I don't anticipate being able to make the distinction any time soon. And I think we need to have a believable prefs picture by 1.7 final. What we have now covers our previously weakest flank, which I'd deem to be mouseover popups. It's a good compromise between safety, usability and documentability. For more safety, with current capabilities, I'd go with some sort of time limit and yes, it would apply to all windows (comment 66). But I don't think we necessarily have to do that. I think the current pref, which sets a hard limit on popup windows, anticipates a future hard limit on script windows, and could be a reasonable forward-looking pref design. We should probably just stick with it.

Since dom.popup_maximum doesn't include click events, I am wondering what do we do about a website which includes this function in every click event on every page of the website?

I really thought I'd answered that question. Lesson learned: be more terse. Nothing, for now.

I think a big part of the problem we have here is that there's no option to disable the the opening of windows by window.open() without disabling the content. I'd love to be able to force the content to load in the original window rather than opening a new one (a capability we've had for a long time with target="_blank"). I suspect this would break far fewer sites than simply disabling so-called "requested" popups, while greatly reducing the annoyance factor of the really abusive sites.

I don't completely follow. But it sounds like you're talking about browser.block.target_new_window. Personally, I want extraneous popup windows stopped, not diverted into the browser.

Well, you see, the idea is to prevent the opening of new windows while breaking the minimum number of sites. A normal window.open() link would then open in the parent window. A site that tries to spew a bunch of popup windows would display the content of the last one in the parent window, and clicking back would get you out of it. The main limitation I see is that a site that opens both content and popups with window.open() from the same click might not have the real content in the last window, but the worst case is still no worse really than simply blocking clicks from opening new windows.

*** Bug 265942 has been marked as a duplicate of this bug. ***

Relevant comments from Bug 265942: ------- Comment #7 From Boris Zbarsky ------- If you click, you expect something to happen, clearly. A number of perfectly reasonable sites make two window.open() calls in onclick handlers.. So just capping this at 1 is not an option. Note that we have existing bugs on this; this one is a duplicate. ------- Comment #8 From Johnny Stenback ------- Interestinly enough IE does seem to cap it at 1. I must say I haven't really seen any sites that open more than one window from a click... but that of course doesn't mean that they don't exist...

I've got a question, if the user allows popups for a site, why after teh dom.popup_maximum is reached, they are blocked?

sGuten: becuase we have failed to build any case scenario when any website would want to open over 20 popups on one click beside of spamming the user. Can you?

(In reply to comment #88) > sGuten: becuase we have failed to build any case scenario when any website > would want to open over 20 popups on one click beside of spamming the user. > > Can you? > Yes, Google Reader does via the "v" key.

We should get rid of the dom.popup_maximum setting. It breaks our web app. We have a web application used by clients that needs to regularly open popup notifications. They only work the 1st 20 times. There seems to be no way to reset this other than closing all browser windows and re-logging in. It is unacceptable to tell our users they must go to about:config and change this to -1 to disable it.

(In reply to comment #90) > We should get rid of the dom.popup_maximum setting. It breaks our web app. We > have a web application used by clients that needs to regularly open popup > notifications. They only work the 1st 20 times. There seems to be no way to > reset this other than closing all browser windows and re-logging in. It is > unacceptable to tell our users they must go to about:config and change this to > -1 to disable it. > I think comment 88 says it best. Most web users including me find it abhorrent that we would have to manage even 20 windows let alone an infinite number of windows. That's the whole reason why this feature was implemented in the first place. I suggest you redesign your application. Regular notifications can be accomplished with dom manipulation and ajax. A good library to accomplish this can be found here: http://www.prototypejs.org/

Who said anything about managing 20 windows. I just want to view one window more than 20 times. That's equivalent to saying why would you ever want to view a menu item more than 20 times. Our clients need a window that can be minimized but comes into focus whenever it updates or when a schedule is met, kind of outlook style. We're using plenty of Ajax & and DOM manipulation. Limiting it to 20 consecutive popups without any other interaction would be ok but this limits it to 20 popups period. You could have had your browser up for 20 days and it only popped up once a day but it fails on the 21st day.

Is this a SeaMonkey only bug, or does it affect Firefox as well?

Don, we increment the count of "popup spam" windows when a popup goes up, and decrement it when it's closed (or more precisely when the Window object is garbage-collected). We only deny a popup if there are 20 "popup spam" windows that have been opened but not closed. That's exactly the behavior I see with this testcase: it opens 30 windows, making sure to close the previous one before opening the next one. It works juts fine over here. If I don't close the windows as I go, the last 10 get blocked, as expected. So it looks to me like opening a window once a day (and then closing it) should work just fine. Is your usecase different somehow? If so, can you please point me to an HTML testcase showing what you're actually doing?

Hmmm That test case ran just fine. We're using Oracle ADF/OC4J (extension to Java Server Faces) that does some funky things, one of which is renaming the window id when you open a window from javascript that retrieves a url on the server. So it might be that it had a different window id when it is closed then it had when it was opened. Would that do it?

This is a firefox issue, I didn't notice that this thread was for the seamonkey suite since it seemed to be talking about the firefox config setting.

Don, what exactly do you mean by "renaming the window id"? Basically, the only way I can see for the popup_maximum counter to affect popups is that there are 20 extant popup windows which have not been garbage-collected, whether because they're still being held by script or for some other reason. For example, your application could be leaking window handles for the lifetime of the webpage or something. One interesting thing I do see is that when closing _windows_ via window.close() we decrement the counter immediately. When closing _tabs_, we wait until the window has been garbage-collected. This might need changing, in a separate bug.

Interesting. I set my popup_maximum to 3 and added a button that calls window.close() If I click the button I can open & close windows indefinitely. If I close the window with the close window 'X' icon, I can only open & close it 3 times. I tried setting window.onunload=closeMe to force calling window.close but it doesn't prevent the popup max from being hit.

Ben, is there another window leak lurking here? The popup count is based on windows being garbage collected, it seems... So it sounds like it could be...

Don, is there any way one of us could get access to the page in question? Closing the window using the windowmanager decoration is the same as closing a tab: the window won't stop counting until all script references to it are gone. I still think we should change that, but it would be good to figure out exactly what's going on with the page in question. It might be a very good idea to file a separate bug on just this page.

Our web site is not exposed externally yet and would be very hard to recreate without the context of the server framework. This is a Oracle Jdeveloper ADF Application. An example of it's capabilitites can be found here http://jdevadf.oracle.com/adffacesdemo/faces/feature/index.jspx It is highly ajax enabled. In my test case I'm selecting a menu item which sends up an ajax request to a backing bean. The bean then sends down some javascript in the ajax response that gets executed on the client which opens the popup window. This is the script that is send down. When the popup opens and the page is retrieved, the popup window.name gets changed. var hWin = window.open("/aquila/faces/adf.task-flow?adf.tfDoc=%2FWEB-INF%2Ftask-flow-follow-ups.xml&adf.tfId=task-flow-follow-ups","51850202_22","status=yes,toolbar=no,copyhistory=no,width=500,height=200"); hWin.focus(); I tried recreating this in a standalone html page but can't duplicate the problem without the ajax interaction. Still experimenting...

Is it possible that the hWin variable in question ends up in the closure of some callback for the XMLHttpRequest on the page and the whole mess stays alive for a while? In any case, it sounds like this page has a bigger issue (ongoing memory leak) than just the popup maximum thing...

I was able to capture a playback scenario that reproduced this problem using Fiddler (http://www.fiddler2.com). If you're not familiar with this it is a http debugging proxy that can capture requests and response. It can playback responses by matching with the requests. If someone is interested in taking a look at this, let me know and I will bundle it up with instructions on how to play it back through fiddler. Thanks.

The exact JS that executes the window.open call, and especially what's in scope when it runs, is much more important for diagnosing this, I think.

Here's the files containing the responses. 32_response.txt is the ajax response containing the script that opens the window. Instructions to run the whole test scenario below. •Download and install Fiddler from http://www.fiddler2.com •Set up firefox to use a localhost proxy Port should be same one configured in Fiddler (8888) make sure "no proxy for:" box is empty. •Verify that you can connect to the web through fiddler. •Close fidder and Unzip attached file to your “My Documents” folder (default location used by fiddler). It should replace what’s in your “My Documents\Fiddler2” directory. •Start fiddler. Switch to AutoResponder tab and check the “Enable automatic responses” checkbox •In Firefox Set your about:config dom.popup_maximum to 3 (so you don’t have to do it 20 times) •Input this url on your browser 127.0.0.1:8988/aquila/faces/authorized_home?_adf.ctrl-state=1037421319_7&test=test •Should bring up home page and popup window immediately. Close popup and select utils menu/View follow ups to open popup again. •Firefox will block after opening & closing 3rd popup window

(In reply to comment #97) > Don, what exactly do you mean by "renaming the window id"? > The window.name is reset whenever a page is loaded from the server. So if you opened a window from javascript that retrieved it's content from the oracle server the window would get renamed so if you tried to reload the window again using the same name it would pop up in a different window. We got around that by sending down the new name for the window in the ajax call every time. The first time the window pops up it's name is 'null' then it changes every time we reload it via the ajax script containing the window.open(...) call.

Don, thanks for the info! I've spun all that off into a separate bug 434095, since it's gettting a bit far afield from the original concerns in this bug. I also filed bug 434096 on the other issue I think we have here.

Bug 675574 will make Firefox disallow multiple-popups-per-click by default. Will that make everyone here happy, or do we need per-site prefs for that?

(In reply to Jesse Ruderman from comment #108) > Bug 675574 will make Firefox disallow multiple-popups-per-click by default. > Will that make everyone here happy, or do we need per-site prefs for that? 9 years later, better late than never! I look forward to seeing it in action. :-)

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046 Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5. If you have questions, please contact :mdaly.