109360 - Image Blocking should block server of image, not server of page

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Brett Denny]

Load a page with banner ads... I noticed this with www.wunderground.com and
www.smartmoney.com but I'm sure it would work on most any page.   Right click on
an ad on the page and "Block Images from this Server"

Expected Result:  You would think (or at least I do) that the server of the
image (whaterver the SRC of the image is) that you clicked on would be blocked.

Actual Result:  Go to Preferences|Privacy & Security|Images|View Image
Permissions and the server that is blocked is for the content of the page you
are viewing, not the server of the image itself.

Why would I want to block all of the page's content, it is the ad I want to get
rid of.  Or am I missing the concept of image blocking?

Comment 1

[Stuart Parmenter]

->morse

Comment 2

[Stephen P. Morse]

Can you be more specific in your example.  Which image did you click on and who 
is the server of that image?

Comment 3

[André Dahlqvist]

[Build-ID: 2001-11-10-15 on Linux]

I've also seen this on many pages lately.

Steps to reproduce:

1) Go to http://www.dn.se
2) Right-click on the top banner and choose "Block images from this server".
3) Go to Tasks->Privacy & Security->Image Manager->View sites that can/cannot..."

Result: The site www.dn.se is listed as blocked.
Expected result: The site that holds the image (ads.adhostcenter.com in this
case) should be blocked.

Comment 4

[Stephen P. Morse]

OK, I can reproduc it.  Confirming.

Comment 5

[André Dahlqvist]

By the way, this happens on every URL that I have tried so far, so I'm pretty
sure that this feature is broken all together.

Comment 6

[André Dahlqvist]

I've narrowed it down:

2001-10-16-08   OK
2001-10-17-08   BROKEN

I'm pretty sure that [e-mail] checkin for bug 97409 broke this. Here's
his checkin was made 10/16/2001 13:37 with the comment:

"bug 97409, add unblocking images to context menu"

I'm guessing this part of the patch fucked it up:

-        var imgmanager =
-          Components.classes["@mozilla.org/imgmanager;1"]
-            .getService().QueryInterface(Components.interfaces.nsIImgManager);
-        imgmanager.block(gContextMenu.imageURL);
+        var permissionmanager =
+          Components.classes["@mozilla.org/permissionmanager;1"]
+            .getService(Components.interfaces.nsIPermissionManager);
+        if (!permissionmanager) {
+          return;
+        }
+        permissionmanager.add(window._content.location, false, IMAGEPERMISSION);
+      },

Therefore I'm adding morse to CC.

Comment 7

[Stephen P. Morse]

You are right.  Don't know what I was thinking when I made that change.  
Attaching revised patch that gets the url from gContextMenu.imageURL which is 
the way it was originally.

Comment 8

[Stephen P. Morse]

Attachment: get url from gContextMenu.imageURL

Comment 9

[Stephen P. Morse]

dahlqvist, please review.
cc'ing alecf for sr.

Comment 10

[André Dahlqvist]

This was the fix I was thinking of. I also applied the patch and it works as
expected again.

(From update of attachment 57412 [details] [diff] [review])
r=andre

Comment 11

[Alec Flett]

Comment on attachment 57412 [details] [diff] [review]
get url from gContextMenu.imageURL

sr=alecf

Comment 12

[André Dahlqvist]

Any chance of this making it into 0.9.6?

Comment 13

[Stephen P. Morse]

I'll be checking this into the trunk just as soon as it opens today.  I don't 
think it's that important that we want to get drivers approval to put this in on 
the branch, but if you think it's important, then feel free to do so.

Comment 14

[Stephen P. Morse]

Fix checked in.

Comment 15

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

a=roc+moz for checkin to 0.9.6

Comment 16

[Stephen P. Morse]

Fix is now checked in on 0.9.6 branch as well.

Comment 17

[Stephen P. Morse]

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