217636 - cookperm.txt index wrong, two image entries, no cookie entry

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Jeremy M. Dolan]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030824 Mozilla
Firebird/0.6.1+ genereates a cookperm.txt with this index/comment thing at the top:

%0>     image
%1>     image
%2>     popup

('>' are tabs)

Older builds correctly show:

%0>     cookie
%1>     image
%2>     popup

I don't think mozilla looks up the values of this index for functional purposes
and it is more a comment for user's reference. Setting severity trivial, unless
anyone knows that this is actually used for something.

Comment 1

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

The headers are used, and it is correct that the numbers can change. Some
extension could add a new type ("flash" for example) and that could end up
having index 1.

Comment 2

[Jeremy M. Dolan]

oh my, oh my.

Just noticed none of my cookie permissions work with this build. Had kind of
noticed something was awry yesterday, but just now figured it out while cleaning
up my cookies.

->major.

Comment 3

[Jeremy M. Dolan]

A little more info, this is a new profile, created with the build mentioned. I
was browsing around the new Firebird preference UI a bit, looking at the
exceptions dialog, etc, so I assume some part of that code duplicated the image
entry, at the expense of cookies.

When I would click 'remember' and remove a cookie from the "Show Cookies" panel,
and it didn't show in the exceptions panel, I figured that panel just wasn't
functional yet. It works properly now that I manually added cookie to cookperm.
I had NOT touched cookperm manually until that point. Somehow Firebird generated
that file like that.

Comment 4

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

all mine

Comment 5

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

Attachment: patch v1

This is what happened:
start with no cookperm.txt. Because the is no file, there are no types defined.
If you add say an image permission (by right clicking the image), that type
gets added with the first free index, 0. Now, restart mozilla. Now, there is a
file, and the type gets read. The other types are empty, and filled with
defaults. But the default for 1 is images. But 0 was already images too. Double
type. Problems.

This patchs adds the default types when there is no cookperm.txt.
If there is a file, it makes sure to add types that might be not in the file.
(In the correct order, to be able to read old style files)

Comment 6

[Jeremy M. Dolan]

> the part that defines the types

Oh good, I'm not the only one who doesn't know what to call that thing.

> // Old files, wothout

Typo.

Comment 7

[Darin Fisher]

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

looks good to me.  sr=darin

Comment 8

[[e-mail]]

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

>+      NS_ASSERTION(GetTypeIndex(buffer.get() + stringIndex, PR_FALSE), "Corrupt cookperm.txt file");

doesn't GetTypeIndex() return -1 for not found? in which case you want
NS_ASSERTION(GetTypeIndex(...) != -1)?

r=dwitte. wow, darin has time to give out sr's without even being asked
nowadays :)

hey, this will be your first landing with your shiny new cvs account, congrats
;)

Comment 9

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

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

Asking approval for 1.5.
This is a low risk patch that fixes that reading / creating cookperm.txt might
go wrong in some cases.

Comment 10

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to Mozilla 1.5

Comment 11

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

checked in.

Comment 12

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

The assertion is the wrong way around. Now it asserts when it shouldn't.

Comment 13

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

Attachment: fix assertion

Comment 14

[[e-mail]]

Comment on attachment 130689 [details] [diff] [review]
fix assertion

r+, no need for sr here, if dbaron's okay with that.

Comment 15

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

Comment on attachment 130689 [details] [diff] [review]
fix assertion

Assking approval for 1.5. This is a low risk change that only affects debug
builds.It fixes some false assertions being fired.

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 130689 [details] [diff] [review]
fix assertion

a=asa (on behalf of drivers) for checkin to Mozilla 1.5

Comment 17

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

assertion fix is in too.