174765 - Popup Blocking should support whitelists like Phoenix

Status: VERIFIED FIXED

(Core :: DOM: Events, enhancement)

Description

[Carl Schrader]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016

The current implementation of popup blocking works, sometimes but the method
being used by Phoenix is soo much better. 99.9% of the popups ever sent are
unwanted. Why show them to me and require that I right click and reject them?
Let me choose TO open them as needed. The other problem is that some sites will
send a popup that is all "flash". When you right click, you get the flash menu
with no option to block the popup. And finally, many times, I right click on the
popup, only to find that it is already on the reject list, yet still popping up.
Yes, I know, these should be filed as seperate bugs but I only included those to
support the point of this bug. If this bug is fixed, those become irrelevant.

Reproducible: Always

Steps to Reproduce:
1. Open site that has popups. (www.newsisfree.com)
2. Witness popup
3. Reject it
4. Restart Mozilla and go to the above site.
5. Witness popup anyhow

Actual Results:  
Popups all over the place.

Expected Results:  
Pop ups that I ask for only.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Dan, don't you already have a bug on a whitelist?

Comment 2

[Dan M]

yes yes, whitelists. Oddly there was no actual bug for this already. So now
there is. The feature is coming, it's just not a very visible effort. This bug's
as good a placeholder as any for it.

Comment 3

[Jason Bassford]

Also, for a way around the context menu problem with sites that use flash, and
others, see bug 171964.

Comment 4

[Daniel Veditz [:dveditz]]

->me, joki won't be working on this AFAIK.

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: trunk patch for whitelisting backend

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: 1.0-branch patch for whitelisting backend

Comment 7

[Dan M]

  It's probably not harmful to give the PopupWindowManager's opinion priority
over dom.disable_open_during_load, but I think it would simplify things to leave
the dom preference in charge. I believe the advantage of prioritizing PWM is it
allows you to fire the popupblocked event. But this overcomplicates permission
checking (the entire IsPopupBlocked function). An alternative I prefer is to
give the dom preference priority, silently failing to open the window if it
insists. Then the unfortunate temporary override of the popupmanager's global
default setting could be avoided.
  Really, I screwed up. The dom preference should have complete control.
privacy.popups.policy is redundant. Ideally, I'd like to get rid of the latter
before we ship something that uses it. So I know that isn't going to happen. But
we should just be using the dom preference. Someday when we're allowed we should
go back and fix that. But failing that, the temporary usurpation of the PWM's
global is still kind of icky.

  You've removed mCustomPermissions. I agree it's unnecessary but at the moment
the UI claims to support it. Wouldn't want to remove back end knowledge of the
preference without also updating the UI.

  Hmmm. In places you've bypassed nsIPermissionManager (going straight to the
Permission_* functions) and in places you're still using nsIPermissionManager.
What's up with that? I think you're right; bypassing nsIPM allows you to skip
the construction of a URI object. Can you clean it up and get rid of
mPermManager completely? That'd be preferable to using it only sometimes. (Wow.
I see the next level down, PERMISSION_*, _Add takes an nsIURI and _Remove takes
a string. Funny. But still it'd be nice.)

nsPopupWindowManager.cpp block @@ -232,19 +234,15 @@
+    mPolicy = (perm = ALLOW_POPUP) ? ALLOW_POPUP : DENY_POPUP;
                    ^^

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: updated trunk patch

Comment 9

[Daniel Veditz [:dveditz]]

Attachment: updated branch patch

These two patches should address Dan's comments except the removal the custom
settings from the pref UI.

Comment 10

[Dan M]

Comment on attachment 103668 [details] [diff] [review]
updated trunk patch

's fine, understanding that this is still a transient stage on the way toward
getting rid of the privacy.popups.policy preference so that
dom.disable_open_during_load will have sole control. But it looks like it'll
work to me.

However this patch will cause regressions; it'll break the current popup
blocking UI. That claims to support three states: all on, all off and
blacklist, and this will no longer be true. And the current UI doesn't set the
dom preference correctly for this model, so it basically won't block popups any
more. We should disable fancy popup blocking for the 1.2 release, leaving the
1.1 model in place. I'll attach a patch later.

  One thing, also please remove line 45 from nsPopupWindowManager.h (#include
"nsIPermissionManager.h").

Comment 11

[Dan M]

Comment on attachment 103669 [details] [diff] [review]
updated branch patch

r=danm, both

Comment 12

[Dan M]

Attachment: disable popup blocking

You'll need to do this for these other patches.

Comment 13

[shliang]

Attachment: revised branch patch

adds one more permission type to separate white lists and black lists

commmented out open unrequested windows checkbox in scripts pref panel

add help context id

Comment 14

[Dan M]

argh. I've been trying to make myself like the new branch patch, but I just
can't make it happen. I think we're kidding ourselves, checking in funky branch
things and promising they'll never make it to the trunk. Please don't ask me to
review this. I'm not allowed to play in this sandbox any more and I've reached a
point where I genuinely don't want to play. And I'm rescinding my review of the
the latest trunk patch, #3. On that I'd restore approval, if asked, but I want
to be clear that's contingent on checkin of the as-yet unreviewed disabling
patch, #5. Which should go in anyway.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 103753 [details] [diff] [review]
disable popup blocking

r=dveditz, this is an addition to the trunk patch (does not need to be done on
the branch)

Comment 16

[jag (Peter Annema)]

Comment on attachment 103753 [details] [diff] [review]
disable popup blocking

sr=jag

Comment 17

[Brendan Eich [:brendan]]

dveditz, jag: can you review the disabling patch at attachment 103753 [details] [diff] [review]?

/be

Comment 18

[Brendan Eich [:brendan]]

Duh, I misread attachment ids.

Ok, I'll approve the disabling patch.

/be

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 103753 [details] [diff] [review]
disable popup blocking

The front end changes are commented or early-returned out -- what's the plan
for restoring the context menu, or removing the code rather than leaving it
disabled or commented out?

a=[e-mail] for the trunk.

/be

Comment 20

[Dan M]

Brendan: I don't want to remove the feature, just disable it during this
turbulent phase. I expect it'll be reenabled after 1.2, though I couldn't say in
what form. Perhaps the commented out code could be simply uncommented then.
Don't know. In the meantime, it makes private patches to enable the feature a
little less subject to conflict.

Comment 21

[Dan M]

Comment on attachment 103668 [details] [diff] [review]
updated trunk patch

r=danm, trunk patch #3, now that the disabling patch #5 is good to go.

Comment 22

[Daniel Veditz [:dveditz]]

Attachment: updated trunk patch (including previous UI change)

This patch includes minor changes requested by jag and incorporates the already
reviewed UI patches (attachment 103753 [details] [diff] [review]) for review completeness.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 103933 [details] [diff] [review]
updated trunk patch (including previous UI change)

I hope I'm not out of line carrying-over danm's r=. The changes to the patch he
reviewed were all in nsGlobalWindow.cpp

Comment 24

[David Hyatt]

Please just yank all of the MOZ_PHOENIX code.  Your new impl can be used by 
Phoenix no problem.  We don't need to leave any of the #ifdefs in.  I assume 
the popup whitelist is just another integer value in morse's stuff (like the 
other lists)?  In theory once you yank everything and land this, I just have 
to change to the whitelist integer value instead of the blacklist integer 
value in Phoenix's prefs UI, right?

Comment 25

[David Hyatt]

So jag says ya use the same list and just have a boolean.  Does that mean all 
I have to change in Phoenix code is my call to popupmanager.add?  I pass 
in "false" (which means blacklist) and I should pass in "true" instead (to 
mean whitelist)?

Comment 26

[jag (Peter Annema)]

Comment on attachment 103933 [details] [diff] [review]
updated trunk patch (including previous UI change)

hyatt: yep, that sounds about right.

dveditz: so what hyatt said two comments ago, just remove the phoenix ifdef'ed
code.

>Index: extensions/cookie/nsPopupWindowManager.cpp
>===================================================================
>@@ -84,74 +77,71 @@
>                    nsIObserver);
> 
> nsresult
> nsPopupWindowManager::Init()
> {
>   mOS = do_GetService("@mozilla.org/observer-service;1");
>-  mPermManager = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>   nsCOMPtr<nsIPrefService> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>   if (prefs)
>-    prefs->GetBranch(sPopupPrefRoot, getter_AddRefs(mPopupPrefBranch));
>+    prefs->GetBranch("", getter_AddRefs(mPopupPrefBranch));

Why not "dom"? That way the pref service won't have to call us for changes to
prefs not in the dom branch. And if you keep sPopupPrefRoot and the
#define/leaf thing (I now understand why it was done that way) you can use
NS_LITERAL_STRING and make things a tiny bit faster.

>@@ -177,28 +167,35 @@
> {
>   NS_ENSURE_ARG_POINTER(aURI);
>   NS_ENSURE_ARG_POINTER(_retval);
> 
>   *_retval = mPolicy;
> 
>+  /* Permission Manager won't save values if there's no host: return default */

I think this comment can be taken out.

>@@ -229,25 +226,20 @@
> //*****************************************************************************
> NS_IMETHODIMP
> nsPopupWindowManager::Observe(nsISupports *aSubject, const char *aTopic,
>                               const PRUnichar *aData)
> {
>   if (nsCRT::strcmp(aTopic, sPrefChangedTopic) == 0 &&
>+      NS_ConvertASCIItoUCS2(sPopupDisablePref).Equals(aData)) {
>+    // refresh our local copy of the "disable popups" pref
>+    PRBool perm = PR_FALSE;

Why not permission like elsewhere?

sr=jag with those changes

Comment 27

[shliang]

Attachment: updated branch patch

undo the two types 

needs to be updated with dveditz's most recent changes (to be posted, according
to jag)

Comment 28

[Daniel Veditz [:dveditz]]

Attachment: final(?) trunk patch

The pref service only calls on the one pref we explicitly register for. Inside
nsPrefBranch it recombines the root and the leafname before calling
PREF_RegisterCallback to do the real work, so it really doesn't matter how we
split it up. Since we're only dealing with the one pref I think it's more
readable as a single string (and it's slightly faster since the pref service
doesn't have to chop the prefix off before calling Observe, but that doesn't
really matter).

I've made the other changes.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 103954 [details] [diff] [review]
final(?) trunk patch

carrying over reviews

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 103954 [details] [diff] [review]
final(?) trunk patch

>@@ -2891,24 +2871,10 @@
>     return PR_FALSE;
>   
>   if (!mIsDocumentLoaded || mRunningTimeout) {
>-    PRBool blockOpenOnLoad = PR_FALSE;
>-    prefs->GetBoolPref("dom.disable_open_during_load", &blockOpenOnLoad);
>-    if (blockOpenOnLoad) {        
>-#ifdef DEBUG
>-      printf ("*** Scripts executed during (un)load or as a result of "
>-              "setTimeout() are potential javascript abuse points.\n");
>-#endif
>-
>-#ifdef MOZ_PHOENIX
>-      // see the definition of the function for details.
>-      PRBool whitelisted = IsPopupWhitelisted(mDocument);
>-      if (!whitelisted)
>-        FirePopupBlockedEvent(mDocument);
>-      return !whitelisted;
>-#else
>-      return PR_TRUE;
>-#endif
>-    }
>+    PRBool blocked = IsPopupBlocked(mDocument);
>+    if (blocked)
>+      FirePopupBlockedEvent(mDocument);
>+    return blocked;
>   } else {

Nit: Get rid of the else after return here, unbrace and exdent the whole block.

>+++ extensions/cookie/nsPopupWindowManager.h	24 Oct 2002 07:35:47 -0000
>@@ -67,9 +67,7 @@
>   void     DeInitialize();
> 
>   PRUint32                       mPolicy;
>-  PRBool                         mCustomPermissions;
>   nsCOMPtr<nsIObserverService>   mOS;
>-  nsCOMPtr<nsIPermissionManager> mPermManager;
>   nsCOMPtr<nsIPrefBranch>        mPopupPrefBranch;
> };
> 

No need for the nsIPermissionManager.h include any longer, now that you've
removed the mPermManager member?

With these nits addressed, a=[e-mail] for 1.2final trunk checkin.

/be

Comment 31

[shliang]

Attachment: updated branch patch

Comment 32

[Daniel Veditz [:dveditz]]

Checked in the trunk back-end patch. I'm marking this fixed because we now
"support" a whitelist, even if it has be to manually entered. I created bug
176624 to cover adding the UI.

Sorry if this is inconvenient, but this bug is already pretty long.

Comment 33

[shliang]

Attachment: updated branch patch

Comment 34

[Daniel Veditz [:dveditz]]

Attachment: fix damage cause by reviewer nitpicks :-) (trunk)

various nitpicks about how we weren't using the permission manager APIs led to
its removal, but it turns out that's what owns the list and needs to stay. If
we try to duplicate the list owning in nsPopupWindowManager then we start
conflicting with the cookie and image blocking code which uses the permission
manager.

I didn't ever see any problems, I suppose because my extensive list of blocked
cookies ensured the permission manager service was already started. But QA saw
situations where the black or white lists wouldn't work until after you opened
the pref panel which uses the permission manager to enumerate the list.

Comment 35

[shliang]

Attachment: updated branch patch

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 104094 [details] [diff] [review]
updated branch patch

>Index: mozilla/extensions/cookie/nsPopupWindowManager.cpp
>+#include "nsIPermissionManager.h"

Don't need this here, already in nsPopupWindowManager.h

>+  if (mOS && mPopupPrefBranch) {

I'd restore the check for mPermManager here too -- if that fails
then we don't have a valid underlying list mechanism to work with.

>+nsPopupWindowManager::Add(nsIURI *aURI, PRBool aPermit)

Since Permission_AddHost might create a new list if one isn't found and someone
someday might ignore a failure from the Init() method, I think it'd be a little
safer to check mPermManager here and bail out if it's null. Otherwise our added
permission might stomp on a valid cookperm.txt file

>+nsPopupWindowManager::Remove(nsIURI *aURI)

DOn't need the same check here, if Remove finds an empty list it just says "OK,
I'm done!" so there's no danger to the cookperm.txt data

with these minor changes r=dveditz

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 104099 [details] [diff] [review]
updated branch patch

r=dveditz

Comment 38

[jag (Peter Annema)]

Comment on attachment 104097 [details] [diff] [review]
fix damage cause by reviewer nitpicks :-) (trunk)

sr=jag

We'll need to clean this up.

Comment 39

[jag (Peter Annema)]

Comment on attachment 104099 [details] [diff] [review]
updated branch patch

There are some small differences in the trunk and branch versions of
nsPopupWindowManager.cpp and nsIPopupWindowManager.idl, sr=jag once you've
fixed those.

Comment 40

[shliang]

Attachment: patch

Comment 41

[Brendan Eich [:brendan]]

>>Index: mozilla/extensions/cookie/nsPopupWindowManager.cpp
>>+#include "nsIPermissionManager.h"
>
>Don't need this here, already in nsPopupWindowManager.h

Stating direct dependencies instead of bootlegging them via nested includes is
generally good form.  Header files have idempotent-include guard #ifndefs, etc.

Hope my nit-picking wasn't to blame (I see the smiley, but still).  Code review
!= testing.  I asked danm about the cookie changes when reviewing attachment
103954 [details] [diff] [review], averring that I hadn't mentally executed all the paths or examined the
PERMISSION_/Permission_/permission_ (is that enough, already? ;-) layers being
used.  The only moral here, it seems to me, is to get the patch in and let the
community test it.  I'll let danm r= attachment 104097 [details] [diff] [review], then I'll sr=.

/be

Comment 42

[David Hyatt]

Just tested the current trunk with phoenix (I patched phoenix's front end code),
and it all works beautifully.  yay!

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 104117 [details] [diff] [review]
patch

r=dveditz

Comment 44

[Dan M]

Comment on attachment 104097 [details] [diff] [review]
fix damage cause by reviewer nitpicks :-) (trunk)

I hadn't investigated the mystery of permission list persistence. Now that I
have I wish I hadn't. Sorry I missed that. Dan, your patch will work so r=me.
However it seems to me that there is no owner of the permission list. All the
top- and mid-level entry points into this code seem to share ownership. So I'd
prefer to maintain the structure of equal nsPopupWindowManager and
nsPermissionManager siblings that you began and do this instead:

Index: extensions/cookie/nsPopupWindowManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/cookie/nsPopupWindowManager.cpp,v
retrieving revision 1.7
diff -u -r1.7 nsPopupWindowManager.cpp
--- extensions/cookie/nsPopupWindowManager.cpp	25 Oct 2002 04:46:19 -00001.7
+++ extensions/cookie/nsPopupWindowManager.cpp	25 Oct 2002 18:52:25 -0000
@@ -88,7 +88,8 @@
   if (prefs)
     prefs->GetBranch("", getter_AddRefs(mPopupPrefBranch));

-  if (mOS && mPermManager && mPopupPrefBranch) {
+  if (mOS && mPermManager && mPopupPrefBranch &&
+      NS_SUCCEEDED(PERMISSION_Read())) {
      // initialize our local copy of the pref
     Observe(NS_STATIC_CAST(nsIPopupWindowManager *, this),
	     sPrefChangedTopic, NS_LITERAL_STRING(POPUP_PREF).get());

Oh. I see you've already checked it in. Well, some extra overhead but no harm.

Comment 45

[shliang]

Comment on attachment 104117 [details] [diff] [review]
patch

carrying over sr=jag

Comment 46

[shliang]

i've incorporated danm's suggestion in the branch patch (comment 44)

Comment 47

[Daniel Veditz [:dveditz]]

I thought of just adding the read to the popup manager init--that solves the
symptoms we encountered. But to do it right you really have to be a profile
change listener, and if both managers are running they'd both be trying to do
the same thing. If you incorporate the permission manager anyway as your diff
shows (but I suspect that's unintentional) the read is just more overhead.

Safer to leave the list to the permission manager for now and clean up the next
layer down soon.  We really need to re-write all that list-walking code anyway,
it should use a hash table not nested for-loops over all the sites. Even a
binary search down the sorted list would be better than what's there now. (this
is more of an issue for cookies and image loads than the relatively infrequent
popups.)

Comment 48

[Dan M]

Right, comment 44 doesn't work because nsPopupWindowManager isn't listening for
profile changes. Sigh. Now I'm suspicious of the other managers that claim
ownership of this list. The list ownership and initialization model need some
serious hammering but for now, scratch comment 44 and go with Dan's funky
initialization scheme, which is the best we're going to do without a rewrite.

Comment 49

[chris hofmann]

Comment on attachment 104117 [details] [diff] [review]
patch

a=chofmann for 1.0.2

Comment 50

[Daniel Veditz [:dveditz]]

Attachment: Fire event only if really blocked

The popup blocked event is not being fired in quite the right place. If
window.open specifies a window name AND that window exists then we load the
content into it even if CheckForAbusePoint() return true. In this fairly
uncommon case the user still gets a popup-blocked notification (beep, ui flash,
whatever) although the window.open did succeed.

Doing it the right place actually simplifies the code a little.

Comment 51

[Daniel Veditz [:dveditz]]

The branch patch (attachment 104117 [details] [diff] [review]) is still good to go, attachment 104214 [details] [diff] [review]
fixes an extra popup-blocked notification in a minor edge case. The blocking or
not of popups works perfectly fine, it's just this extra event announcing a
block that didn't actually happen.

I'll pursue getting it in, but it shouldn't hold up the bulk of the implementation.

Comment 52

[jag (Peter Annema)]

Comment on attachment 104214 [details] [diff] [review]
Fire event only if really blocked

sr=jag

Comment 53

[Dan M]

Comment on attachment 104214 [details] [diff] [review]
Fire event only if really blocked

better this way anyway. r=danm

Comment 54

[Sven Grull]

Could it be that this checkins caused the "original" popup-manager to disappear?
In the trunk (2002102508, win2000) there is no entry under tools and also the
prefs for popup-blocking are not there where they used to be.

Comment 55

[Dan M]

Yes. Popup blocking was intentionally disabled by the fifth patch (attachment
103753 [details] [diff] [review]), checked in after incorporation into the ninth patch (attachment
103954 [details] [diff] [review]). Comment 10 superficially explains. See also reopened bug 166442 comment
104 - 106.

Comment 56

[Daniel Veditz [:dveditz]]

Fixed on branch. Fixed on trunk as far as "support" goes. other bugs deal with
the UI.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 104214 [details] [diff] [review]
Fire event only if really blocked

a=roc+moz for trunk

Comment 58

[Daniel Veditz [:dveditz]]

Attachment 104214 [details] [diff] checked into the trunk

Comment 59

[Vladimir Ermakov]

Verifying on build 2002-10-30-..-1.0 on Win2k and 2002-10-30-..-1.0 on OSX

Comment 60

[Boris Zbarsky [:bzbarsky]]

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

Comment 61

[Vladimir Ermakov]

There is no popup blocker in trunk builds... dont know what to do about
verifying with this.

Comment 62

[sairuh (rarely reading bugmail)]

vladimir: yes, it's there. :) check Edit - Preferences - Privacy & Security -
Popup Windows.

Comment 63

[Rakesh Mishra]

Ah, had an old trunk build. Verifying.

Comment 64

[Jaakko Järviluoma]

Just wanted to know if the whitelist is coming or not?? Please?

Comment 65

[Simon Spiegel]

It's here and working fine. Just download 1.3b.