Open Bug 197919 Opened 22 years ago Updated 2 years ago

Non Mouse Click & Max Number of Popups Exceptions

Categories

(Core :: DOM: Core & HTML, enhancement, P5)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

People

(Reporter: mattsch, Unassigned)

References

Details

Attachments

(7 files, 4 obsolete files)

Popup Tester
3.02 KB, text/html
Details
more general popup protection
22.00 KB, patch
bryner
: review+
jst
: superreview+
Details | Diff | Splinter Review
max popup limit re-think, and special treatment for click handlers
7.39 KB, patch
bzbarsky
: review+
jst
: superreview+
Details | Diff | Splinter Review
Replaces Demonstrates how click events can be abused
625 bytes, text/html
Details
Replaces Demonstrates how click events can be abused
625 bytes, text/html
Details
Testcase that shows that dom.popup_maximum shouldn't affect the use case from comment 92
232 bytes, text/html
Details
Response files for Fiddler test exhibiting memory leak
461.79 KB, application/zip
Details 
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
Severity: major → enhancement
OS: Windows 98 → All
Hardware: PC → 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.
Status: UNCONFIRMED → NEW
Component: Browser-General → XP Apps: GUI Features
Ever confirmed: true
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.
Attached file Popup Tester — 
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.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → INVALID
.
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
Resolution: INVALID → ---
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.
Assignee: asa → jaggernaut
Status: REOPENED → NEW
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.
Summary: Popup Windows Exceptions needs more permissions → Per-site, per-event popup permissions exceptions
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.
Summary: Per-site, per-event popup permissions exceptions → Non Mouse Click & Max Number of Popups Exceptions
QA Contact: asa
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?
Blocks: 227338
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.
Attachment #138971 - Flags: review?(bryner)
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.
Blocks: popups
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.
Attached patch more general popup protection (obsolete) — — Splinter Review 
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.
Attachment #138971 - Attachment is obsolete: true
Attachment #139233 - Flags: review?(bryner)
Attachment #138971 - Flags: review?(bryner)
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.
Attachment #139233 - Attachment is obsolete: true
Attachment #139233 - Flags: review?(bryner)
Attachment #139281 - Flags: review?(bryner)
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. ***
Attachment #139281 - Flags: review?(bryner) → review+
Attachment #139281 - Flags: superreview?(jst)
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
Attachment #139281 - Flags: superreview?(jst) → superreview+
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.)
Attachment #140520 - Flags: superreview?(jst)
Attachment #140520 - Flags: review?(bzbarsky)
(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.
Attachment #140520 - Attachment is obsolete: true
Attachment #140559 - Flags: review?(bzbarsky)
Attachment #140520 - Flags: superreview?(jst)
Attachment #140520 - Flags: review?(bzbarsky)
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.
Attachment #140559 - Flags: review?(bzbarsky) → review+
> 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.
Blocks: useragent
Comment on attachment 140559 [details] [diff] [review]
max popup limit re-think, and special treatment for click handlers

sr=jst, fwiw
Attachment #140559 - Flags: superreview+
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?

        Attachment #142255 -
        Attachment is obsolete: true

    
    

    
  
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.
Product: Core → Mozilla Application Suite

    
    

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

    
  
Blocks: pop-up-arms-race

    
    

    
  
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.

    
  
Assignee: jag → nobody
Component: XP Apps: GUI Features → DOM
Product: Mozilla Application Suite → Core
QA Contact: general

    
    

    
  
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?
Depends on: 675574

    
    

    
  
(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.
Priority: -- → P5
Component: DOM → DOM: Core & HTML

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: