Disable window.open from onload and onunload

VERIFIED FIXED in mozilla0.9.5



16 years ago
15 years ago


(Reporter: Robert Ginda, Assigned: Mitchell Stoltz (not reading bugmail))


(Blocks: 1 bug)


Firefox Tracking Flags

(Not tracked)


(Whiteboard: fixinhand, URL)


(5 attachments)



16 years ago
Our disable window.open functionality is nice, but not very practical.  With the
threat of pop-unders looming, "don't allow domain X to window.open" will become
less of an option, as more sites will have pop-up/unders.  Maintaining the
domain list will become a hassle, and some sites with pop-foo ads may have
other, legitimate uses for window.open.  My proposed solution is to add the
ability to deny window.open when called from onload and onunload events.  This
"good enough" solution should eliminate most of the bogus uses of window.open,
without breaking legitimate uses.

My proposed implementation (patch coming up) involves searching the JS stack
(via XPConnect) for the top JS frame before allowing the window.open to proceed.
 If the top JS frame is a function named "onload" or "onunload" (case
insensitive match), bail out, otherwise continue as planned.  The problems I see
with this approach are:

 * Any script with a top level function named "onload" or "onunload" that is not
attached to window, that also calls window.open, will be blocked.  This false
positive is in all probability rare, and should not cause problems in the real
 * Sites can get around this detection by using setTimeout (or, even worse,
setInterval) which has no constant function name to match against.

An alternative, and possibly cleaner solution would be to add a member to the
window implementation that represents the current "in commonly abused function"
counter.  This counter, initially 0, would increment by one each time the
window.onload, window.onunload, and setTimeout/Interval calls were about to be
made, and decrement by one as these routines completed.  Window.open could then
verify that the counter was 0 before allowing the new window to be created. 
This method is much more robust, but involves some ideas that I'm not sure how
to implement.  Specifically, is it possible to hook onload, etc. in this way?  I
am under the impression that this code is all generic, and hooking 2 specific
events (onload and onunload) is nontrivial.

Comment 1

16 years ago
Created attachment 44147 [details] [diff] [review]
proposed patch; take one

Comment 2

16 years ago
Here is a proposed fix.  It needs to be wrapped in a pref for sure, but I'd like
some feedback on the basic idea before I spend too much time on this.

Comment 3

16 years ago
Here are some related bugs:
Bug 29346 "Prevent repeating pop-up windows"
Bug 33448 "disable 'new window' when close box clicked (no window.open in onUnload)"
Bug 64737 "Capabilities configurable by trigger"

Comment 4

16 years ago
New patch coming up.

This patch hooks into HandleDOMEvent, in order maintain a mIsLoadComplete
boolean member.  mIsLoadComplete is PR_TRUE between the completion of
window.onload and the start of window.onunload.  This gives us the ability to
block top level scripts, as well as onload and onunload, and eliminates the JS
stack walk.

The patch also blocks window.open if mRunningTimeout is non-null, to keep
designers from getting around this stuff with a setTimeout() in onload.

Comment 5

16 years ago
Created attachment 44181 [details] [diff] [review]
proposed patch; take two

Comment 6

16 years ago
This will probably block the majority of popups/popunders right now, until the
advertisers get wise to it and/or we regain enough market share to make it worth
their while. Then, they will open popups on mouseover, on focus, etc. In the
meantime, I think this is a good idea. We'll need to make this controllable by
pref, because Netscape will probably want it off.

Also, we could potentially tie this into caps rather than hardcoding it into
window.open. Caps could call CheckForAbusePoint() and use that as another piece
of input to the security check for property access. That way, we or the users
could block other properties besides window.open from running in the "abuse
points" simply by adding a line to prefs. I can do this if people think there's
a value to it. Thoughts?

Comment 7

16 years ago
Created attachment 44444 [details] [diff] [review]
proposed patch; take three

Comment 8

16 years ago
The last patch was too conservative in checking for onload, it never saw chrome
documents load, so you couldn't open new windows from chrome, like say,
mailnews.  I'm running with this patch in my day to day opt build, with no
obvious problems yet.

It'd be nice to integrate this into caps, but I don't know caps well enough to
understand how it'd work.  This patch adds CheckForAbusePoint() as a protected
member of nsGlobalWindowImpl, how will caps call it?

Comment 9

16 years ago
I'm taking this over.
Assignee: rginda → mstoltz
Target Milestone: --- → mozilla0.9.4

Comment 10

16 years ago
Created attachment 47449 [details] [diff] [review]
Patch take 4 - updated & added pref

Comment 11

16 years ago
I think patch 4 is check-in-able. The question is, do we want it on by default
in Mozilla? Needs reviews and approval.
Priority: -- → P1
Whiteboard: patch
A couple of issues with the patch:

- Any reason to not combine the check for aEvent->message being NS_PAGE_LOAD or
NS_PAGE_UNLOAD into one place in stead of having them separated? (and yes,
please do keep the braces around the if statement)

- In GlobalWindowImpl::Open() the suggested changes are:

+  /* If we're in a commonly abused state (top level script, running a timeout,
+   * or onload/onunload), and the preference is enabled, block the window.open().
+   */
+  nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
+  if (prefs)
+  {
+      PRBool blockOpenOnLoad = PR_FALSE;
+      prefs->GetBoolPref("privacy.block_open_during_load", &blockOpenOnLoad);
+    if (blockOpenOnLoad && CheckForAbusePoint()) {
+#ifdef DEBUG
+        printf ("*** Blocking window.open.\n");
+        return NS_OK;
+    }
+  }

This gets the pref service even if it's not necessarily needed, I would suggest
this instead:
+  /* If we're in a commonly abused state (top level script, running a
+   * timeout, or onload/onunload), and the preference is enabled, block
+   * the window.open().
+   */
+  if (CheckForAbusePoint()) {
+    nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
+    if (prefs) {
+      PRBool blockOpenOnLoad = PR_FALSE;
+      prefs->GetBoolPref("privacy.block_open_during_load", &blockOpenOnLoad);
+      if (blockOpenOnLoad) {
+#ifdef DEBUG
+        printf ("*** Blocking window.open.\n");
+        *_retval = nsnull;
+        return NS_OK;
+    }
+  }

Especially note the setting of *_retval in the early return case, this is important.
And, a few nits on top of that:

- Both 4 and 2 space indentation used in GlobalWindowImpl::CheckForAbusePoint(),
use 2 space indentation in this file.

- Bad indentation in GlobalWindowImpl::Open(), but that's fixed in my suggested
change above.

- mIsLoadComplete could maybe be renamed to mIsDocumentLoaded to make the name
match closer with what it's actually used for.

sr=jst with the above changes.
shaver-nit: there's nothing privacy-related here, that I can think of.  Let's
not dilute the term any more than we have to (or have already: c.f. the form

Comment 14

16 years ago
Sure it's privacy-related. Pop-up ads are a violation of my privacy (in that
they are annoying, not that they steal personal info). Can you suggest another
term? It's not "security." 
"Privacy" in the internet context almost always refers to personal information:
none of the interesting legislation or lobbying is really concerned with
"annoying things on the internet", other than spam -- and there it's because an
email address is PII.  I think we'd do well to keep our "privacy" label applied
only to personal information.

(Aside: Why isn't it OK to say it's "securing" the browser against popups, but
it's OK to say that it's keeping you "private" against popups?  If this isn't
security, how are Window.alert controls in our configurable "security" policies
security?  I don't get it.)

Anyway, it's a site capability.  Why isn't this in with the rest of the
configurable-"security" stuff, like general access to Window.open, &c.?  We
could either have a different capability

    user_pref("capability.policy.default.Window.open_during_load", "noAccess")

or a different "security level" value
    user_pref("capability.policy.default.Window.open", "notDuringLoad")

.  I think either would fit better with our current
what-can-this-site-do-to-my-browser? story.

Comment 16

16 years ago
I would argue that this is a security issue, after all. Denial of Service
attacks fall in this category. Opening pop-up windows ad infinitum crashed a lot
of browsers. This also may leads to dataloss.

Comment 17

16 years ago
> If this isn't
> security, how are Window.alert controls in our configurable "security" policies
> security?  I don't get it.

They're not security. They're just a useful side effect of the security manager.
We've had this discussion, remember? I'd rather see "content control" stuff
handled seaprately from security, at least on the frontend.

> Anyway, it's a site capability.  Why isn't this in with the rest of the
> configurable-"security" stuff
Ideally it would be. In fact, I plan to integrate this with the configurable
policies stuff, but we're obviously out of time in this milestone and I wanted
to get a basic functionality into this milestone because I think our users will
really appreciate it.


Comment 18

16 years ago
Created attachment 47691 [details] [diff] [review]
Patch v5 with jst's suggestions.

Comment 19

16 years ago
I changed the pref name to "dom.disable_open_during_load" so we're not diluting
the meaning of 'privacy.' Eventually I will integrate this into the
"capability.policy" functionality but there's no time now.

Johnny, as to your first point, it looks like the check for NS_PAGE_UNLOAD needs
to come before the event handler is called, and NS_PAGE_LOAD afterwards.
Whiteboard: patch → fixinhand
Sweet.  Thanks, Mitch.  (I guess we'll just leave this bug open, and mutate the
summary a little bit when you check in?)

Comment 21

16 years ago
Mitch, yeah, doh, I should've realized...
a=brendan@mozilla.org on behalf of drivers, for 0.9.4 checkin.


Comment 24

16 years ago
The fix is checked in. Bug 64737 will track continuing efforts to generalize
this mechanism.
Last Resolved: 16 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 25

16 years ago
just for the record: 

What is the pref to disable popups on window close?

Comment 26

16 years ago
The pref is dom.disable_open_during_load. It's Boolean (1 or 0). I got this
fromm the text of the latest patch.


Comment 27

16 years ago
Actually, the value must be true or false, not 1 or 0. 

Comment 28

16 years ago
*** Bug 33448 has been marked as a duplicate of this bug. ***

Comment 29

16 years ago
Since as others have noted, once advertisers get wise to this they'll do
something else like opening a window on focus, why not just have an option to
ONLY allow window.open when clicked.  That's the only time we ought to want it,
anything else is just annoying and rife with abuse.  Is this fix general enough
that an enhancement request could be made to add that as another option?  Or
perhaps there is another similar bug already opened?  I looked but wasn't able
to find one.

Comment 30

16 years ago
In embedding builds, this patch seems to be a bit over zealous, and disables all
popup windows.

Steps to recreate:

Add the line
user_pref("dom.disable_open_during_load", true);
to prefs.js in you mfcembed profile directory

Launch mfcembed
go to www.collegeclub.com
click the "Log On" link

Expected results:
A popup window is created to let the user enter a username and password

Actual results:
No popup window is created.
Resolution: FIXED → ---

Comment 31

16 years ago
Slice, bug 64737. Jeff, embedding may be out of luck on this one but I'll take a

Comment 32

16 years ago
Why is this still open?

Re: Jeff's comment on embeded builds... not seeing that on linux 2001091305...
if it's still happening on embeded, file a seperate bug.

Comment 33

16 years ago
This seems to be working on windows embedding now, too.

Comment 34

16 years ago
Marking Fixed again based on comments. If this is still broken in embedding,
file a separate bug. I'm not sure that embedding wants this (at least not the
AOL embeddors; maybe others do).
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 35

16 years ago
Verified on 2001-09-20-Branch build on WinNT

Followed following steps and the pop up window is not opened
1. Added line user_pref("dom.disable_open_during_load", true); to prefs.js file.
2. Loaded www.collegeclub.com and clicked on the LogOn button
3. Nothing happens.


15 years ago
Blocks: 176958
You need to log in before you can comment on or make changes to this bug.