Currently image blocking is accomplished by the nsIImgManager::CheckForPermission() method (http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsIImgManager.idl#35 ). This causes extra insertion points for callbacks to verify image loads, because contentpolicy callbacks are already being made.

I'm applying a patch that: 1. makes formatting consistent in nsImgManager.cpp 2. removes the CheckForPermission method from the manager. 3. makes the manager impl and register nsIContentPolicy.

r=pavlov ... i expect there are a few places that still call CheckForPermission though.

the only callsite for the checkForPermission() method which my patch removes is in mozilla/modules/libimg/src/if.cpp::il_PermitLoad(). my patch works if I just comment that call out, and pavlov has talked about removing if.cpp altogether. pavlov, can il_PermitLoad go away, when I walk it up the call chain, IL_GetImage() uses it, which is used by gfx/src/nsImageRequest.cpp (http://lxr.mozilla.org/seamonkey/source/gfx/src/nsImageRequest.cpp#258 ). Because my patch works (blocks images) w/ that callsite commented out, it's not clear to me how it's used. another benefit to using this model is that image blocking can leverage the multiple image loading enable/disable callsites that content policies are currently using.

The last time I investigated it (which was a while ago), I discovered that the reason image blocking got broken is because images were being loaded from somewhere else and no longer going through the code in if.cpp::il_PermitLoad(). So it seems to me that the fix is to put in the necessary checking at the new place that images are being loaded from. It's not at all clear to me from this patch that that was actually done. Has anyone applied this patch and tried to use image manager to block the image on mozilla.org?

morse wrote: "The last time I investigated it (which was a while ago), I discovered that the reason image blocking got broken is because images were being loaded from somewhere else and no longer going through the code in if.cpp::il_PermitLoad()." right, that's what I'm trying to confirm w/ pavlov. "So it seems to me that the fix is to put in the necessary checking at the new place that images are being loaded from. It's not at all clear to me from this patch that that was actually done." That's the whole point of this patch... it doesn't have to touch any blocking callsites. my patch to http://bugzilla.mozilla.org/show_bug.cgi?id=81260 already implemented image blocking using nsIContentPolicy, so the callsites are all covered. The idea is that imgManager blocking can just leverage this code, no need to dupe effort and add risk w/ more callsites (the 73848 doesn't cover the needed callsites anyway)" "Has anyone applied this patch and tried to use image manager to block the image on mozilla.org?" See my previous posting here. my test case indeed happened to be moz.org's image (which I toggled on and off). For an understanding of the nsIContentPolicy blocking model, see http://www.hungry.com/~shaver/Content_control_and_capabilities_in_Mozilla.html

pavlov is irradicating if.cpp sometime this week and on IRC suggested I just comment out the CheckPermissions() call. patch forthcoming.

patch to remove old callsite in if.cpp. Index: if.cpp =================================================================== RCS file: /cvsroot/mozilla/modules/libimg/src/if.cpp,v retrieving revision 3.73 diff -u -r3.73 if.cpp --- if.cpp 2001/05/15 02:55:51 3.73 +++ if.cpp 2001/06/05 19:30:51 @@ -1813,7 +1813,7 @@ return PR_TRUE; } PRBool permission; - rv = imgmanager->CheckForPermission(host, firstHost, &permission); + //rv = imgmanager->CheckForPermission(host, firstHost, &permission); Recycle(host); Recycle(firstHost);

r=pavlov

Will the image-manager dialog (tasks->privacy->image-manager->view-sites) still show the list of the blocked sites, or is there a new UI that goes with the content policy?

the UI is independent. all of the backend and UI is the same. I verified on windows and linux that the view-sites content is still valid.

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

Even though the change to if.cpp is transient, I'd prefer to see some comments around it, indicating why the call to CheckForPermission was removed. Other than that, sr=vidur. For future reference, it would be good to have a #define for the content policy category name.

Copying the keywords from dup bug 73848 and removing the blocking of it.

need to checkin 82000 before this patch. 82000 is already r/sr'd and a'd for the trunk.

Just tried to build with the patch on Linux and got this error: nsModuleFactory.cpp c++ -o nsModuleFactory.o -c -DOSTYPE=\"Linux2.2\" -DOSARCH=\"Linux\" -DOJI -I../../dist/include -I../../dist/include -I/moz/mozilla/dist/include/nspr -I/usr/X11R6/include -fPIC -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wbad-function-cast -Wcast-align -Woverloaded-virtual -Wsynth -pedantic -Wno-long-long -pipe -pthread -DNDEBUG -DTRIMMED -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../config-defs.h -Wp,-MD,.deps/nsModuleFactory.pp nsModuleFactory.cpp nsModuleFactory.cpp: In function `nsresult UnregisterContentPolicy(nsIComponentManager *, nsIFile *, const char *, const nsModuleComponentInfo *)': nsModuleFactory.cpp:73: no matching function for call to `nsDerivedSafe<nsICategoryManager>::DeleteCategoryEntry (const char[15], const char[26], int)' ../../dist/include/nsICategoryManager.h:78: candidates are: nsresult nsICategoryManager::DeleteCategoryEntry(const char *, const char *, int, char **) nsModuleFactory.cpp:74: warning: control reaches end of non-void function `UnregisterContentPolicy(nsIComponentManager *, nsIFile *, const char *, const nsModuleComponentInfo *)' Is this because of bug 82000?

yes, you need 82000, or just add back the last (bogus) param for deletecategoryentry in your tree.

a=blizzard on behalf of drivers for 0.9.1 and the trunk assuming you fix what vidur suggested

fix is in. this is *not* on the 0.9.1 branch.

valeski: Do you need someone to check this in on the branch for you? It has a= for that, and I thought that was the entire point... Gerv

I didn't notice the 0.9.1 approval in here. re-opening and applying to 0.9.1 branch as well. I'll revert the one line api change in 82000 for the content policy patch so I don't have to drag 82000 into the branch.

I see a serious problem with the patch on Linux (not sure if it's really caused by this checkin): 1. Goto Image Blocking in prefs 2. Select "originating server only" 3. Click OK 4. Visit any page with ads on it 5. Crash 6. Restart Motzilla 7. Goto Image Blocking in prefs 8. Crash That's not what we want in 0.9.1. Workaround: Set user_pref("network.image.imageBehavior", 0); by hand to make Mozilla usable again.

adding clarence to cc list

At first, attach 37274 appeared to fix my crash problems. After some further playing around, it appears to have just moved the crash to IMAGE_CheckForPermission().

same patch w/ more bulletproofing coming up.

+ if (NS_FAILED(rv) || !doc) return rv; This kind of error checking will return NS_OK even if doc is NULL. Is that what you want here? Otherwise, r=sfraser

yes, NS_OK return was intented there. we don't want to kill anything if the doc isn't avail (valid state).

sr=rpotts. looks good to me.

fix is in. built on mac,win,lin.

in

Sorrry to spam but will this be checked into 0.9.1?

currently this sits on the trunk, not the 0.9.1 branch. after the fallout from yesterday, I'm reluctant to put this on the branch w/ out at least 24 hours of shakeout on the trunk. I'm leaving town tomorrow, so I wouldn't want to checkin on the branch and run anyway. after bulletproofing yesterday, I have a high degree of confidence that this code is branchworthy, but it wouldn't be right for me to checkin and leave town. someone feel free to drive this into the branch though.

Another problem with accepting images from the originating server only: Internal images that are needed for scrollbars are blocked. I see only a small part of the scrollbar (the middle part). This is with 2001-06-06-04-trunk on Win NT, Modern theme only (not tried on Linux yet).

the patch I just attatched checks the content location's scheme as well. now we're checking both the baseURI's scheme, and the URI of whatever it is we're trying to load. If either of those is not http, we let the load continue. this is particularly baffling to me though. I don't understand how one URI could be chrome (I'm assuming it is in the scrollbar image case), and another be http. pavlov?

the document could have a chrome:// baseurl (like a xul window) and a http url image in it, so you'd have two different schemes. r=pavlov

See bug 84403 for the scrollbar problem.

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

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

Image blocking still fails on certain URIs. I'm guessing it's the ones with port numbers on them. On http://www.voodooextreme.com, a number of images from http:adserver.ugo.com:80, and these fail to block correctly.