240070 - Add nsIContentPolicy <> nsIPermissionManager general glue

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

We have nsIContentPolicy, that allows to block all kinds of content. We have
nsIPermissionManager to store all kinds of permissions for different types.
Those two should be actually used.
Currently, there is an image blocker, that is a nsIContentPolicy implemenation,
and uses nsIPermissionManager for the lookup of permissions. Why not extend it
to all types content policy knows about? Then you could block plugins,
javascript etc from random sites. Or only allow it from certain sites.

This should probably live in /extensions/permissions or so. nsIPermissionManager
can be moved there too. It should _not_ live in /extensions/cookie where the
current image manager lives.

I'm not proposing to add UI at the moment. Just hand editing hostperm.1

One could argue that this should be a mozdev extension or so, but i think the
code bloat shouldn't be that big, as most code already is there in the form of
the image manager. There wouldn't be UI bloat, as i don't plan on adding it :)
And it would be a nice feature to be able to block flash from irritating ad sites.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

I was going to wait for bug 191839, but there isn't much movement there. So i
might as well attach what i have.
It is largely copied from nsImgManager. It doesn't do checks for mailnews. I
think that should be a seperate thing.

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 146876 [details] [diff] [review]
patch v1

I will fix the license boilerplates.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated to new api

Patch is now updated to the new nsIContentPolicy api.
Also, it includes a seperate file for mailnews blocking. I'm not sure what to
do with the prefs. The old pref is only about blocking images. For now, i'm not
using it, and created a new one. This new pref blocks all remote content, not
only images.

Comment 4

[Darin Fisher]

ben goodger and i spoke a while back about the possibility of adding richer site
preferences... this looks like a good push in that direction.  i'll try to
review the patch this month!  sorry for the huge review delays :(

Comment 5

[[e-mail]]

Attachment: review response to above patch

my review comments for the above patch.

Comment 6

[[e-mail]]

Comment on attachment 148569 [details] [diff] [review]
updated to new api

cancelling review request, since you probably want to post an updated patch
before you ask for sr=darin ;)

Comment 7

[Richard Klein]

Any chance of having a generic DOM Event fire for all blocked content.  My
thought is having statusbar icons for each type of blocked content, including
popups and xpinstalls.  Then we could allow the user to toggle permission for
either the blocked host or just one specific blocked uri.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated

Updated patch to review comments.
I think the best way to go is to get this code checked in, but not build by
default. Then we can test it some more, iron out issues, and after that enable
it and make it replace nsImgManager. When we do that, there should be some
pref-migrating code, or we should just ise those prefs.

Comment 9

[[e-mail]]

Comment on attachment 153935 [details] [diff] [review]
updated

nice, r=dwitte... although, before we land i think we should have some idea of
how we want to split extensions/cookie up, in case we don't end up moving the
residents to extensions/permissions. i will file a bug and propose some ideas,
so we can get a quick handle on it. i don't want to hold up this patch :)

anyway, one comment:

>diff -rNU8 foo/nsContentBlocker.cpp permissions/nsContentBlocker.cpp
>+  // Ony support NUMBER_OF_TYPES content types. that all there is at the
>+  // moment, but you never know...
>+  if (aContentType > NUMBER_OF_TYPES)
>+    return NS_OK;

in my previous review i said:

you need to change this check... like so:

// aContentType is from 1..8, while the array index for kTypeString is 0..7, so
// we need to decrement and bounds-check it.
if (--aContentType >= NUMBER_OF_TYPES)
  return NS_OK;

i think this still applies, right?

Comment 10

[[e-mail]]

filed bug 252554 about moving permmgr backend/consumers.

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #9)

> i think this still applies, right?

No, because i wanted aContentTyp to mean the same thing everywhere, so here it
is still 1..8. Look down, where i commented about -1, to make it 0..7 (in
TestPermission)

Comment 12

[[e-mail]]

ah, brilliant, i missed that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

mvl, I'm not really doing srs on code I don't know, remember?

I can try to get to this, but it will take me a while (no less than a week or
two, possibly a lot longer).

Comment 14

[Darin Fisher]

Comment on attachment 153935 [details] [diff] [review]
updated

>+++ permissions/nsContentBlocker.cpp	2004-07-21 14:35:36.000000000 +0200

>+// Possible behavior pref values
>+// Those map to the nsIPermissionManager values where possible
>+#define BEHAVIOR_ACCEPT 1
>+#define BEHAVIOR_REJECT 2
>+#define BEHAVIOR_NOFOREIGN 3

should these be:

#define BEHAVIOR_ACCEPT nsIPermissionManager::ALLOW_ACTION

etc.?


>+nsContentBlocker::~nsContentBlocker()
>+{
>+}

nit: maybe no need to write this destructor since it is empty?


>+  // Keep a reference to the branch, otherwise it might go away (since it's a 
>+  // custom non-default branch), and then we wouldn't get notified anymore!
>+  mPrefBranchInternal = do_QueryInterface(prefBranch, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);

this sounds like a bug in the pref system.  please be sure to file
a bug on it.


>+  // we only want to check http, https, ftp
>+  PRBool needToCheck = PR_TRUE;
>+  rv = aContentLocation->SchemeIs("ftp", &needToCheck);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!needToCheck) {
>+    rv = aContentLocation->SchemeIs("http", &needToCheck);
>+    NS_ENSURE_SUCCESS(rv,rv);
>+
>+    if (!needToCheck) {
>+      rv = aContentLocation->SchemeIs("https", &needToCheck);
>+      NS_ENSURE_SUCCESS(rv,rv);
>+    }
>+  }
>+
>+  // for chrome:// and resources and others, no need to check.
>+  if (!needToCheck)
>+    return NS_OK;

i wonder if this might be better:

  nsCAutoString scheme;
  aContentLocation->GetScheme(scheme);
  if (!scheme.EqualsLiteral("ftp") &&
      !scheme.EqualsLiteral("http") &&
      !scheme.EqualsLiteral("https"))
    return NS_OK;

it should be less code, and writing the scheme to the stack should
be extremely cheap.


>+nsContentBlocker::TestPermission(nsIURI *aCurrentURI
...
>+  nsresult rv = mPermissionManager->TestPermission(aCurrentURI, kTypeString[aContentType - 1], &permission);

wrap this long line to 80 chars?


>+    const nsACString &tail = Substring(currentHost, dot, currentHost.Length() - dot);

as an optimization, you should use |const nsCSubstring &| here instead.


>+    const nsACString &firstTail = Substring(firstHost, firstHost.Length() - tail.Length(), tail.Length());

same thing here, and please wrap long lines.


>+  NS_ASSERTION(!nsCRT::strcmp(NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, aTopic),

nit: just use |strcmp|


>+++ permissions/nsContentBlocker.h	2004-06-22 19:38:14.000000000 +0200

>+  virtual ~nsContentBlocker();

dtor can be non-virtual.


>+protected:

since nothing subclasses this class, maybe these methods should
be marked private.  the destructor could also be put in the
private section.  if someone needs to subclass this guy, then
they could always make things protected as needed.




>+++ permissions/nsMailnewsContentBlocker.cpp	2004-06-06 21:47:23.000000000 +0200

>+nsMailnewsContentBlocker::~nsMailnewsContentBlocker()
>+{
>+}

nit: no need to write this dtor.

>+nsMailnewsContentBlocker::Observe(nsISupports     *aSubject,
...
>+  NS_ASSERTION(!nsCRT::strcmp(NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, aTopic),

just use strcmp


>+  virtual ~nsMailnewsContentBlocker();

no need for virtual dtor.  make it private.


>+protected:

make them private.


overall, this patch looks really good.	sr=darin

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in (with updates)
Not yet enabled in default build. Leaving open to fix that.

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

I enabled this for a while, to see Tp impact.
from btek, it went up maybe 2 msec, not more. Hard to say due to noise. That
would be about 0.3%
I think we can get that back by disabling the current image blocker.

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: enable by default

This patch enables the permissions module for seamonkey and firefox by default.

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 168150 [details] [diff] [review]
enable by default

asking for review/approval on this patch for seamonkey and firefox.

Comment 19

[Darin Fisher]

Comment on attachment 168150 [details] [diff] [review]
enable by default

so are there plans to add UI for this to firefox?  or is that going to be done
as an extension?

i'm fine enabling this by default since it is not much code, and since it can
be used to replace the image manager code.

sr=darin

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: fix a bit of logic, migrate prefs, update seamonkey ui, enable for seamonkey

Updated patch. This does what was needed to make it work for seamonkey,
including migrating prefs and updating the UI.
If this is in, we can stop building the image manager.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: enable for firefox

Patch to enable for firefox. Depends on the seamonkey patch

Comment 22

[Darin Fisher]

Comment on attachment 168562 [details] [diff] [review]
fix a bit of logic, migrate prefs, update seamonkey ui, enable for seamonkey

looks good to me.  sr=darin

Comment 23

[[e-mail]]

Comment on attachment 168562 [details] [diff] [review]
fix a bit of logic, migrate prefs, update seamonkey ui, enable for seamonkey

>Index: extensions/permissions/nsContentBlocker.cpp
>===================================================================
>+  PRInt32 oldPref;
>+  oldPrefBranch->GetIntPref("network.image.imageBehavior", &oldPref);
>+  if (oldPref) {
>+    PRInt32 newPref;
>+    switch (oldPref) {
>+      case 0:

i don't think you want this |if| wrapper, given the switch/case?

yay, don't forget to kill imgmgr after you check in! ;)

r=dwitte

Comment 24

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #23)
> i don't think you want this |if| wrapper, given the switch/case?

No, because i don't want to always set the new pref. I removed the |case 0|,
left it as default, just in case.

Comment 25

[Michiel van Leeuwen (email: mvl+moz@)]

patches checked in. This is now enabled for seamonkey and firefox. jay!

Comment 26

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

Any chance that Camino missed out on these changes (bug 299356) and/or this is
why hostperm.1 stopped blocking images in Camino (bug 281000)?