343937 - Default "unblock popup" behavior should not include adding site to Exceptions List

Status: RESOLVED FIXED

(Camino Graveyard :: Annoyance Blocking, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I filed bug 331194 to make the unblock button show the popup rather than blindly add it to the exceptions list (whitelist), but along the way we ended up compromising on showing the popup and adding it to the whitelist all at once.

I still don't think that it's a good idea to whitelist a site until after the popup has been shown.

CNN.com is a great example of why this is a bad idea.  CNN.com launches multiple popups and, considering that CNN is a "respectable" organization (maybe they're not in the AOL era, but Metro Atlantan here and all), one might believe that these are true site-related popups and tell Camino to "unblock" CNN.com.  Camino would dutifully show the evil ads.  When bug 336020 is fixed, we'll actually whitelist the "real" site showing the popup, an ad-server, instead of CNN.com.

This becomes very evil pretty quickly, because we'd be whitelisting an ad-server that 1) the user doesn't really want whitelisted 2) the user doesn't know he's whitelisting 3) without having shown the user the popup first to allow the user to make an informed decision and 4) with no easy way for the user to discover the actual site spawning the popups so as to remove the site from the exceptions list when he discovers he's getting popups not only from this site but all over the web.

I believe we need separate "Show" and "Whitelist" (we can keep the "Unblock" name, I guess) buttons in any UI, and they should do just that, not some combination (and, better yet, some more descriptive UI as to which popup(s) we'll show and which sites(s) will be whitelisted).  We also need to figure out how to do this cleanly in the UI, which I realize will be difficult.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per pink at one of our meetings, "Unblock" should be renamed "Show" when it shows the pop-up.

Comment 2

[Håkan Waara]

Smokey and I discussed this on IRC. Here's an idea:

1. Have a popup-menu (as previously discussed) in the popup bar. Something like this:


 Show all blocked popups
 -------------------------
 Show popup from x.foo.com
 Show popup from y.foo.org
 (etc)

Now, most users are probably gonna use "Show all popups" if they encounter trouble. And we don't want that to whitelist every popup, so here's how to solve that:

2. For a shown popup, we need some way for the user to confirm that it was an OK popup. We can have a little cute bar in the shown popup, where there's just a button to "Always show this popup". That would be how to permanently whitelist a specific popup.

It's a bit more clever than Firefox's bloated UI, and provides the user a way to permanently block/show certain popups in a natural way (try showing it, and then saying "that's OK").

What do you guys think about it? As a final note, the confirmation bar would have too look good, so some Hicks loving would be needed. ;)

Comment 3

[Håkan Waara]

Taking. I'm almost done with a revamp of the popup blocker code based on comment 2.

Comment 4

[Håkan Waara]

I'm not so sure that blocking individual popups is easy to fix. 

When unblocking popups, we actually unblock based on the requesting site's URI, so either you allow all popups from a site, or none.

Comment 5

[Håkan Waara]

So, I'm not sure how to go ahead with this bug, if it's not possible to block some, but not all, popups from a webpage. 

The popup-unblock verification strategy in comment 2 point #2 doesn't really work then...

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Well, in the cnn.com case, evil popups are coming from ads.cnn.com.

Could the bar in the popups track the requesting site, so that it said something like:

Unblock this and all other pop-ups from [requesting site, e.g. ads.cnn.com]?

And clicking on Unblock would whitelist the requesting site and close the bar on all other popups that were opened from the same requesting site?

(The case I'm really thinking about here is a site where a page throws its own (used-by-site) pop-up but also has an iframe from another server on the page, which throws evil popups.  So you have 3 popups, 2 from site-i'm-visiting and 1 from ad-server, and you want popups from the site itself...)


On an entirely different note, I still think that we need a way to show the urls of the actual popups themselves before showing, but the UI for that will also be hard....

Comment 7

[Samuel Sidler (old account; do not CC)]

Not blocking 1.1a1, but we'd definitely take a patch.

Comment 8

[Stuart Morgan]

One UI approach worth kicking around is to handle it like we do cookies. The bar would say something vaguely like:

This site wants to show a popup    Deny   Allow
|_| Always do this for this site


Deny hides the bar (the current 'x' button).
Allow shows the popup.
Deny + Always adds the site to a blacklist where even the bar wouldn't show for popups from that site.
Allow + Always whitelists the site.

Comment 9

[Stuart Morgan]

(That doesn't address the part of this bug that's about showing the user the popup before letting them decide, obviously; it's just a take on the issue of show vs. whitelist.)

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

When we blacklist a site (Deny button + Always checkbox), are we going to blacklist the pop-up's host (just as Gecko whitelists the pop-up's host) or the host of the site showing the pop-up?

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

If we believe cl's hostperm.1 documentation http://kb.mozillazine.org/Hostperm.1#popup we should be able to blacklist a host by setting the policy to 2, and then we could(?) then query hostperm.1 and not show the bar when the policy is set to 2.  I.e., no separate list for the blacklist required.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Last comment for now, I promise....

Fx has no UI for setting that hostperm.1 policy to 2 (only for 1/allow), but when you do so manually, the Fx popup exceptions list shows it as "Block".  Fx, however, still shows the inline bar (or the status bar icon) when a site set to 2 tries showing popups.  IMO, that's a bug we should avoid ;)

Comment 13

[Stuart Morgan]

Attachment: wacky mockup

The trick to adding a checkbox is that if you add a second line for it, it starts to look pretty weird. This is an attempt to give that UI without making it too cluttered; it's a bit unusual, but it (or something like it) just might work.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Not blocking a2.

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

At the very least, we need to fix the substance of my original comment (axe configure and have "show" and "whitelist" functioning buttons), even if we can't completely fix the UI like we want to.

Comment 16

[Stuart Morgan]

I'll try to have a bunch of mocks ready for tomorrow's meeting.

Comment 17

[Stuart Morgan]

Attachment: new mock

The mockup we agreed on in the meeting (with sample tooltip) for b1

Comment 18

[Stuart Morgan]

We'll need to adjust the prefs UI to deal with blacklisting, so it will need to look more like the cookie UI.

Comment 19

[Stuart Morgan]

It looks to me like what we mean by blacklisting and what core means by blacklist are different, and core doesn't support what we mean (the source of comment 12). The core describes the current policy as blacklist by default, with a whitelist. That implies a two-state system: popups are either allowed without asking, or blocked and the user gets UI. What we want is a three-state system such as exists for cookies, where things can be silently allowed, silently dropped, or cause a prompt.

I'll confirm this next week, and if I'm right see if any core people are willing to entertain making popup hostperm mean the same thing cookie hostperm means. If they aren't we'll need to implement our own blacklist (which won't be too hard, just a bit ugly).

Comment 20

[Stuart Morgan]

Attachment: fix

This implements the Allow Once/Always/Never bar. We don't need to have core support for the blacklist *or* implement our own, as it turns out; we can just ask the permission manager whether there is a blacklist entry for that site and not show our UI if there is, so that's what this patch does.

This also fixes bug 369197, since this UI doesn't really work well with a source-domain-based black/whitelist, as discussed in the meeting.

Comment 21

[Stuart Morgan]

Attachment: corresponding nib

This also needs the following new strings entries:

"PopupDisplayRequest" = "%@ wants to show a pop-up";
"GenericHostString" = "This site";

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We still need comment 18, too, right (making this the 3rd open bug on that nib)?

Comment 23

[Stuart Morgan]

Right. Sorry, I meant to file that as a spin-off when I posted the patch. Now filed as bug 370084.

Comment 24

[Nick Kreeger]

Comment on attachment 254743 [details] [diff] [review]
fix

> +- (void)whitelistPopupsFromURL:(NSString*)inURL
>  {
> -  nsCOMPtr<nsIPermissionManager> pm (do_GetService(NS_PERMISSIONMANAGER_CONTRACTID));
> -  pm->Add(URL, "popup", nsIPermissionManager::ALLOW_ACTION);
> +  nsCOMPtr<nsIURI> uri;
> +  NS_NewURI(getter_AddRefs(uri), [inURL UTF8String]);
> +  nsCOMPtr<nsIPermissionManager> pm(do_GetService(NS_PERMISSIONMANAGER_CONTRACTID));
> +  if (pm && uri)
> +    pm->Add(uri, "popup", nsIPermissionManager::ALLOW_ACTION);
> +}

> +- (void)blacklistPopupsFromURL:(NSString*)inURL
> +{
> +  nsCOMPtr<nsIURI> uri;
> +  NS_NewURI(getter_AddRefs(uri), [inURL UTF8String]);
> +  nsCOMPtr<nsIPermissionManager> pm(do_GetService(NS_PERMISSIONMANAGER_CONTRACTID));
> +  if (pm && uri)
> +    pm->Add(uri, "popup", nsIPermissionManager::DENY_ACTION);

nsresult rv;
nsCOMPtr<nsIPermissionManager> pm = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);

if (NS_SUCCEEDED(rv) && pm && uri)

> +- (BOOL)popusAreBlacklistedForURL:(NSString*)inURL
> +{
> +  nsCOMPtr<nsIURI> uri;
> +  NS_NewURI(getter_AddRefs(uri), [inURL UTF8String]);
> +  nsCOMPtr<nsIPermissionManager> pm(do_GetService(NS_PERMISSIONMANAGER_CONTRACTID));
> +  if (pm && uri) {

It might be safe to nsresult and check to your service request:

> +- (void)showPopupsWhitelistingSource:(BOOL)shouldWhitelist
>  {
>    NS_ASSERTION([self popupsBlocked], "no popups to unblock!");
>    if ([self popupsBlocked]) {
> -    nsCOMPtr<nsIArray> blockedSites = do_QueryInterface(mBlockedSites);
> -    [mDelegate unblockAllPopupSites:blockedSites];
> +    nsCOMPtr<nsIArray> blockedSites = do_QueryInterface(mBlockedPopups);
> +    [mDelegate showBlockedPopus:blockedSites whitelistingSource:shouldWhitelist];
>      [self removeBlockedPopupViewAndDisplay];
>    }
>  }
 
It would be nice to not have to call |popupsBlocked| twice, perhaps add:

#if DEBUG
  else 
    NSLog(@"no popups to unblock!");
#endif


Nits:
> -  if (!mBlockedSites)
> -    CallCreateInstance(NS_ARRAY_CONTRACTID, &mBlockedSites);
> -  if (mBlockedSites) {
> -    mBlockedSites->AppendElement((nsISupports*)eventData, PR_FALSE);
> +  if (!mBlockedPopups)
> +    CallCreateInstance(NS_ARRAY_CONTRACTID, &mBlockedPopups);
> +  if (mBlockedPopups) {
> +    mBlockedPopups->AppendElement((nsISupports*)eventData, PR_FALSE);

Can you add a comment to |AppendElement| to notify that |eventData| is being owned by the |nsIMutableArray| (because of the PR_FALSE).

Otherwise, code looks good to me. r=me with changes.

Comment 25

[Sean Murphy]

Comment on attachment 254743 [details] [diff] [review]
fix

...
patching file src/browser/BrowserWrapper.h
Hunk #2 FAILED at 138.
Hunk #3 FAILED at 198.
2 out of 3 hunks FAILED -- saving rejects to file src/browser/BrowserWrapper.h.rej
patching file src/browser/BrowserWrapper.mm
Hunk #1 FAILED at 63.
Hunk #2 FAILED at 116.
Hunk #3 FAILED at 196.
Hunk #4 succeeded at 344 (offset -5 lines).
Hunk #5 succeeded at 541 (offset -15 lines).
Hunk #6 succeeded at 705 (offset -41 lines).
Hunk #7 succeeded at 1049 (offset -61 lines).
3 out of 7 hunks FAILED -- saving rejects to file src/browser/BrowserWrapper.mm.rej

The patch would not apply for me using up-to-date trunk revisions of BrowserWrapper.h and BrowserWrapper.mm.

compiler warning: `BrowserWrapper' may not respond to `-getCurrentURI'
(BrowserWapper.mm: lines 722, 1101, 1133 and BrowserWindowController.mm: line 1923)

Rev 1.45 of BrowserWrapper.h changed the method name of |getCurrentURI| to just |currentURI| (your fix as part of Bug 368934).  I'm guessing you were coding this patch on a different tree or something, since it still contains references the old |getCurrentURI| method in BW.h.

+- (void)showBlockedPopus:(nsIArray*)blockedSites whitelistingSource:(BOOL)shouldWhitelist;
+- (BOOL)popusAreBlacklistedForURL:(NSString*)inURL;
..
+  if ([self popusAreBlacklistedForURL:[self getCurrentURI]])
..
+    [mDelegate showBlockedPopus:blockedSites whitelistingSource:shouldWhitelist];

"Popups" spelled wrong here as "popus".

(In reply to comment #24)
> > +- (void)showPopupsWhitelistingSource:(BOOL)shouldWhitelist
> >  {
> >    NS_ASSERTION([self popupsBlocked], "no popups to unblock!");
> >    if ([self popupsBlocked]) {
> > -    nsCOMPtr<nsIArray> blockedSites = do_QueryInterface(mBlockedSites);
> > -    [mDelegate unblockAllPopupSites:blockedSites];
> > +    nsCOMPtr<nsIArray> blockedSites = do_QueryInterface(mBlockedPopups);
> > +    [mDelegate showBlockedPopus:blockedSites whitelistingSource:shouldWhitelist];
> >      [self removeBlockedPopupViewAndDisplay];
> >    }
> >  }
>
> It would be nice to not have to call |popupsBlocked| twice, perhaps add:
> 
> #if DEBUG
>   else 
>     NSLog(@"no popups to unblock!");
> #endif

NS_ASSERTION() is declared in nsDebug.h and the debug versions of each macro declared in that file are already wrapped inside an #ifdef DEBUG.  Each macro reference is essentially replaced with a blank line in release builds:

(From nsDebug.h:)
..
#else
/**
 * The non-debug version of these macros do not evaluate the
 * expression or the message arguments to the macro.
 */
..
#define NS_ASSERTION(expr, str)        PR_BEGIN_MACRO /* nothing */ PR_END_MACRO

I modified around the patch and was able to apply it to my source tree for testing.  All functionality works as expected.  So, with the method name corrections plus a patch update to properly integrate with the most recent trunk revisions, r=me.

Comment 26

[Stuart Morgan]

(In reply to comment #25)
> I'm guessing you were coding this patch on a different tree or something, since
> it still contains references the old |getCurrentURI| method in BW.h.

I was coding this to the current tree at the time I made the patch; it predates the landing of the currentURI change.

Comment 27

[Stuart Morgan]

Attachment: v2

(In reply to comment #24)
> > +  nsCOMPtr<nsIPermissionManager> pm(do_GetService(NS_PERMISSIONMANAGER_CONTRACTID));
> > +  if (pm && uri)
> > +    pm->Add(uri, "popup", nsIPermissionManager::DENY_ACTION);
> 
> nsresult rv;
> nsCOMPtr<nsIPermissionManager> pm =
> do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
> 
> if (NS_SUCCEEDED(rv) && pm && uri)

That adds clutter for no reason. The implementation of do_GetService explicitly nulls out the return value if the status that would be returned in the two-param version isn't a success value, so there's no reason to use the longer call unless you actually care what the error was.

> Can you add a comment to |AppendElement| to notify that |eventData| is being
> owned by the |nsIMutableArray| (because of the PR_FALSE).

Since the default in Cocoa-land is that collections own their contents, I don't think it needs to be called out; we only need to comment behavior that wouldn't be expected.

(In reply to comment #25)
> "Popups" spelled wrong here as "popus"

Good catch; I managed to do that in two methods. Fortunately neither was a method used by the nib, so that doesn't need a respin.

Comment 28

[Stuart Morgan]

Attachment: v3-trunk

Patch as checked in on trunk (I forgot the getCurrentURI->currentURI change in the last patch).

Comment 29

[Stuart Morgan]

Attachment: v3-branch

Branch version as checked in (popup handling code is slightly different on branch).

Comment 30

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH.

Comment 31

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Stuart, was there supposed to be a .strings checkin with this?  I see "PopupDisplayRequest" instead of "Camino has blocked a pop-up on this website" or whatever your replacement text might have been.

Comment 32

[Stuart Morgan]

Sorry about that. Strings checked in.

Comment 33

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #21)
> This also needs the following new strings entries:
> 
> "PopupDisplayRequest" = "%@ wants to show a pop-up";
> "GenericHostString" = "This site";

Stuart, was leaving out the period intentional?  I was working through the docs on this tonight and went to block some pop-ups and saw that.  It's a full sentence, and IMO it looks weird without the period.

Comment 34

[Stuart Morgan]

IIRC the old sentence didn't have a period, and I just left it the way it was. I can't remember if I tried it with one or not.