Closed Bug 215461 Opened 21 years ago Closed 21 years ago

Block images seems to not work.

Categories

(Core :: Graphics: Image Blocking, defect)

Product:
Core
Component:
Graphics: Image Blocking
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: spatz, Assigned: mvl)

References

(
URL
)

Details

(Keywords: fixed1.5, regression)

Attachments

(2 files)

Cookperm.txt at the point where no image or cookie permissions can be saved
4.73 KB, text/plain
Details
ignore bogus lines
1.28 KB, patch
dwitte
: review+
darin.moz
: superreview+
asa
: approval1.5+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030804
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030804

Right click and select "Block images from this server".  New page comes up and
an image from server that was just selected loads.

Reproducible: Always

Steps to Reproduce:
1.Chose to block images.
2.New image loads from same server.
3.

Actual Results:  
Images keep loading.

Expected Results:  
Images should be blocked.
Any page? You'll have to be more specific. The URL you gave doesn't show
anything for me.
Sorry about that.  It was a quick copy and past of the curent image I was trying
to block.  to answer your question, yeah it appears to be any page.  I like to
block the ad images from Slashdot, they still all show up.  This is the url for
one I just tried to block: http://ads.osdn.com/ ( with the ad stuff snipped.)  I
then reloaded the page and a completely different image loaded but still from
ads.osdn.

This happens with with every image I have tried to block.
I have cleared my cache and it still doesn't work.
works for me.
Is there an entry for ads.osdn.com in the image manager after you tried to block
the image? What are you image blocking prefs? In about:config, what is the value
for imageblocker.enabled?
There is nothing listed in the image manager, and I have blocked a lot of images
in the past 2 days.

imageblocker.enabled is set to TRUE.


what does your cookperm.txt (from your profile) contain?
It's a 60k file so I am not posting it all here ( unless you need to see it all)
but for the ads.osdn.com I have this:

ads.osdn.com	1F	4F	6F
Confirming on a Linux CVS build from 2003-08-16 (today). Image blocking seems to
be completely broken: The list in Tools->Manage Image Permissions is empty,
although I had blocked quite a few servers.

Setting OS=All and requesting 1.5b blocker, as this is a regression of one of
the features that get praised ist the reviews most.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.5b?
Keywords: regression
OS: Windows 98 → All
This works for me with both old and new profile on Seamonkey build 20030820 on
Linux.
pref | privacy and security | images  -- do not load images 
makes it happen for me on winxp using builds from last week and today -> 20030820

if I run browser buster I don't see any images, but I do run across lots of
flash content that is shown as images...  

can someone that is seeing the problem try and narrow the exact steps to make it
happen?  thx...
spatz, oliver: can you guys try creating a new mozilla profile just for kicks?  thx!
Steve, can you post the first few lines of your cookperm.txt? 10 to 15 will do.
With a new profile, I dont't have this problem. I'll try if deleting
cookperm.txt in the old profile makes any difference.
OK, deleting cookperm.txt fixed the problem for me.
Deleting cookperm.txt fixed the problem only temporarily - every now and then,
all image permissions are gone again. Argh.
Oliver, can you attach one of the cookperm.txt files you have after it stops
working?
I'll do so the next time I see the problem.
maybe for final but doesn't look like this is gonna make beta.
Flags: blocking1.5b?
Flags: blocking1.5b-
Flags: blocking1.5?
This could be related to bug 217636, so maybe it is fixed now.
I saw this bug again after using Mozilla 1.4 one time before switching back to
a CVS build. File permissions are ok.
Thanks for that file. It looks the problem is that older version of mozilla
don't understand the additions to the header of cookperm.txt. Thats allright,
but the format I came up with made it that older version mass it up. Now they
write out a bad version.
The solution could be to change the new version. That must be done in such a way
that new version can read the current format too.
Assignee: security-bugs → mvl
Flags: blocking1.5? → blocking1.5+
1.4 writes out the cookperm.txt file from memory. So everything it skips or
doesn't understand correctly when reading the file isn't saved again. Or it is
saved in a wrong format.

The two solutions I see are
- move the definitions to some other place, like prefs. That separates them from 
  the permissions itself, which I don't really like.
- if illegal lines saved with 1.4 are read, ignore them, and set them to the
  defaults. That will break when there are other types besides cookie, image and
  popup. Those permissions will be lost when running 1.4. But nothing uses other 
  types at the moment. So if some extension goes to use that, you shouldn't use
  your profile with 1.4 anymore.

dwitte, darin, any thoughts?
Flags: blocking1.5+ → blocking1.5?
hmm, midair flag change. Sorry.
Flags: blocking1.5? → blocking1.5+
I have noticed this too. It happens when I read my Yahoo webmail. I have some ad
servers blocked. On the first load, it shows the ad, but if I hit the reload
button the ad goes away and the blocking seems to work like it used to. I am
most puzzled.

Not that I can get the ad picture's context menu and it actually says 'unblock
images from this server,' thereby also confirming it isn't a flash animation.

I am using a 1.4 build (I lack admin rights to install an update - sorry), so I
don't know if this is current and useful info, but I figured it might help.
Never mind the comment above. Further investigation showed my problem is not
related to this bug. Sorry about that.
Wow, this sucks. I would have thought someone would have tested how older builds
handle the new cookperm format before checking it in.

I thought of two other schemes to get around this, but after further pondering,
I've come to the conclusion Michiel already listed the best (of the worst) two
solutions. Well, I would even throw "relnote" in there as one of the top three
too. It involves neither bloat nor permanently destroying cookperm.txt as the
central permission storer.
This patch does the second option. I'm not sure if this is the right way to go,
but i think it is the less worst option I can think of.
It keeps the data together in one file, and the loss of data will only occur if
someone uses a extension for 1.5 or later, and then goes back to 1.4. I don't
think that will happen a lot. (The data isn't really lost, only mozilla can't
access it anymore). There won't be a problem with the cookie, image and popup
blocking lists anymore.

there is actually another option, and that is to create a new file, and move
all the data in there. That will prevent the loss of data, but older version
can't access the permission list anymore. And it also is too much code this
late before a release.
Attachment #130923 - Flags: superreview?(darin)
Attachment #130923 - Flags: review?(dwitte)
Comment on attachment 130923 [details] [diff] [review]
ignore bogus lines

>Index: extensions/cookie/nsPermissionManager.cpp

>+      // Older versions of mozilla can't parse the permission sting lines.
>+      // They will put them backe like '%0 0F' instead of '%0 cookie'
>+      // Ignore those lines, and revert to the defaults later.
>+      // XXX This means that when the user has additional types besides the
>+      // default, those will be lost after using and old version with the
>+      // new profile, and then going back to a new version.
>+      if (!PL_strcmp(buffer.get() + stringIndex, "0F"))
>+        continue;

nit: please fix typos in the comment, and use |strcmp| instead of |PL_strcmp|.
Attachment #130923 - Flags: superreview?(darin) → superreview+
+      // They will put them backe like '%0 0F' instead of '%0 cookie'
extra 'e'                        ^

Just a note, you mentioned using a new file name. This would fix bug 106663
(weird/wrong that filename for image blocking is cookperm). I recently commented
there that siteperm.txt would be a good replacement name, since the file is now
responsible for setting any number of permissions on domains.

That would be my prefered solution, but dwitte@stanford.edu wasn't too fond of
the idea of breaking backwards compat.
uber-nit:

+      // default, those will be lost after using and old version with the
                                                  ^^^

this should be 'an'.

Comment on attachment 130923 [details] [diff] [review]
ignore bogus lines

wellll....

the thing i don't like here is this: say our user goes from 1.5 to 1.4, and the
header lines for his extension gets lost by 1.4. but the permission data for
random sites is still there, orphaned - some future extension could be mapped
to that (if he goes back to 1.5), and then it'd pick up a whole bunch of random
things the user never intended.

since the most this could amount to is a privacy issue, and it's unlikely at
that, i'm not too worried. however, i'd like to make sure darin is aware of
this possibility... darin, can you comment on that?

r=dwitte with spelling mistakes fixed, and provided darin says it's fine. (i'll
make sure you fix those spelling nits too :)
Attachment #130923 - Flags: review?(dwitte) → review+
can we do something so that this problem doesn't happen when the next permission
extension comes online during another future milestone.  do we maybe need a new
file format?  or maybe at least a new file location with migration code?  is it
time for something like that?
Renaming the file would take more time then left for 1.5.

If a new file is created, and the old one deleted, older versions won't have
anything left. On the positive side, it won't mess with newer files.
We also could leave the old file in place, but it would not be updated anymore.
Old permissions in old mozilla's. And keeping the old file up-to-date would be
bloated imho.
the file is small.  keeping the old one as is, and only making changes to the
newer one seems reasonable to me.  moreover, i doubt users will be very
surprised to find that changes to permissions made in 1.5 do not show up when
they go back and run 1.4.  this same thing was done for the mail filters file. 
old versions use a completely different file.  we could do the same here.  i.e.,
when loading permissions, load the new file first, and only load the old file if
the new file isn't found (only a couple extra lines of code).  when writing
permissions, write to the new file location.  that would be a small enough patch
i think, and it would certainly be something that we could do for 1.5.  if the
drawback is possible corruption of the users permissions as a result of
switching back and forth between 1.5 and netscape 7.1, which i'm sure many users
will do, then i think this is something we should try to do for 1.5 final.
If we move to another file, we could also move to another file format. The
current format isn't ideal. (not only the headers, but everything) It seems like
a waste to not look at it when changing files anyway.

It would be nice if it didn't destroy lines it doesn't understand, so problems
like this won't occur later.
It also would be cool if we could reuse some other file parser, and not write
yet another. But maybe this is overkill :)
mvl: good point about wanting to change the file format.  so, how do you suggest
we solve this problem?
on irc:

<darin> ok, then maybe we shouldn't worry so much
<darin> but we should fix the file format for 1.6 at least

so darin's happy with doing this patch for 1.5, and then playing with file
formats in 1.6. that way, we can rev the format to hearts' content in 1.6 :)

let's drop text-mode and go binary, baby! no more snprintf!
Comment on attachment 130923 [details] [diff] [review]
ignore bogus lines

Requesting approval for 1.5.
This is fairly low risk. It will improve reading cookperm.txt files that old
versions create. It makes it so that cookie, image and popup permssions are not
lost.
Attachment #130923 - Flags: approval1.5?
Comment on attachment 130923 [details] [diff] [review]
ignore bogus lines

a=asa (on behalf of drivers) for checkin to Mozilla 1.5
Attachment #130923 - Flags: approval1.5? → approval1.5+
checked in to the trunk.
keeping open to check in to the 1.5 branch.
also in the 1.5 branch.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Keywords: fixed1.5
*** Bug 219587 has been marked as a duplicate of this bug. ***
Verified FIXED.  This works for me now with build 2004-07-25-09.  I successfully
blocked all ads coming from *.osdn.com servers on Windows XP.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: