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.

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

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.

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

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

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; ^^

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

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 on attachment 103669 [details] [diff] [review] updated branch patch r=danm, both

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

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

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 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 on attachment 103753 [details] [diff] [review] disable popup blocking sr=jag

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

Duh, I misread attachment ids. Ok, I'll approve the disabling patch. /be

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=brendan@mozilla.org for the trunk. /be

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 on attachment 103668 [details] [diff] [review] updated trunk patch r=danm, trunk patch #3, now that the disabling patch #5 is good to go.

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

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

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?

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

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

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 on attachment 103954 [details] [diff] [review] final(?) trunk patch carrying over reviews

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=brendan@mozilla.org for 1.2final trunk checkin. /be

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.

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 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 on attachment 104099 [details] [diff] [review] updated branch patch r=dveditz

Comment on attachment 104097 [details] [diff] [review] fix damage cause by reviewer nitpicks :-) (trunk) sr=jag We'll need to clean this up.

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.

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

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

Comment on attachment 104117 [details] [diff] [review] patch r=dveditz

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 on attachment 104117 [details] [diff] [review] patch carrying over sr=jag

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

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

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 on attachment 104117 [details] [diff] [review] patch a=chofmann for 1.0.2

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.

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 on attachment 104214 [details] [diff] [review] Fire event only if really blocked sr=jag

Comment on attachment 104214 [details] [diff] [review] Fire event only if really blocked better this way anyway. r=danm

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.

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.

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

Comment on attachment 104214 [details] [diff] [review] Fire event only if really blocked a=roc+moz for trunk

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

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

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

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

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

Ah, had an old trunk build. Verifying.

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

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