219752 - create a new file to store nsPermissionManager's data in

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

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

My recent changes to nsPermissionManager created some compatibility problems.
See bug 215461
I think it is time to create a new fileformat. It should get a new name.
cookperm.txt isn't really logical anymore, now it contains permissions for more
types.

Things I would like in that format:
- older versions of mozilla shouldn't kill things newer versions added
- reuse an existing parser, to reduce copied code
- be human readable

Comment 1

[[e-mail]]

a few random thoughts:

1) we need a versioning system, so older mozillas ignore newer versions, and
newer mozillas can (try to) import data
2) we should consider adding an extra field (metadata) with the permission - we
may or may not want to do this. it would be useful, for instance, to be able to
specify "this site can set lifetime-limited cookies", and then specify that
lifetime as an integer that is stored alongside the permission value itself.
perhaps we would want different fundamental types - integer, bool, string; much
like prefs.

Comment 2

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

my random thoughts:

* Could using nsIPersistentProperties work? (other than that is isn't
persistent. save isn't implemented). A permission could look like:
some.domain.com.cookie=accept. We can just store lines that the current version
doesn't understand, and write them out later.
some.domain.com.cookie.maxlifetime=10 could be used for the metadata.
This format takes more memory though.

* Versioning is nice, but having code to read all kinds of old version files is
bloat. Not that i expect changes that much :)

Comment 3

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

The best way to make sure the version system would work, is just to add the
version to the file name. So hostperm1.txt for the first version.

Comment 4

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

Attachment: patch v1

This patch creates a new file, named "hostperm.1". The format is:

host \t cookie \t 1 \t www.mozilla.org

The first part is an identifier. The patch only tries to parse lines with
identiefiers it understands. At the moment, that is only "host". In the future,
someone might add code to make it understand "regex" for example. 
If it doesn't understand it, it ignores it. But when writing, it first reads
lines with unknown identifiers from the file, and writes them right back. after
that, it writes the "host" lines.
So, if in some future version you can add "regex" lines, they won't get lost on
using an old version.

The patch also loads the old cookperm.txt if there is no new hostperm.1 file.

I didn't use an existing parser, because i couldn't find any that would do the
job. And with nsStringArray.ParseString, the parser isn't that big :)

Metadata could be added later, by using a "host-meta" identifier for example.

Comment 5

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

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

Requsting review on the patch, but take a look at comment 4 before you read the
patch. It tries to explain what the patch does.

Comment 6

[[e-mail]]

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

>Index: extensions/cookie/nsPermissionManager.cpp
>===================================================================
>+static const char kMatchTypeHost[] = "host";
>+
>+

one too many newlines eh?

> nsresult
> nsPermissionManager::Read()
> {
>   nsCOMPtr<nsIInputStream> fileInputStream;
>   rv = >NS_NewLocalFileInputStream(getter_AddRefs(fileInputStream), >mPermissionsFile);
>+  if (NS_FAILED(rv)) {
>+    // Try finding the old-style file

i wonder if we want to just check for NS_ERROR_FILE_NOT_FOUND here, rather than
a blanket NS_FAILED(rv). i'll defer to darin on this one.

>   /* format is:
>+   * matchtype  \t type \t permission \t host

you have an extra space after |matchtype|

>+   * Only host is supported for matchtype

can you put |host| in quotes... |"host"| ?

>+   * type corresponds to what Add() wants as type

|type is a string that identifies the type of permission (e.g. "cookie")|

>+   * permission is an integer, what AddInternal wants

|permission is an integer between 1 and 15|

>   nsASingleFragmentCString::char_iterator iter;

now that this variable is only used in the compatibility-code block, can you
shift its declaration to inside that?

>+      // Split the line at tabs
>+      lineArray.ParseString(PromiseFlatCString(buffer).get(), "\t");

you don't need the PromiseFlat here.

>+      if (lineArray[0]->Equals(kMatchTypeHost)) {
>+        if (lineArray.Count() != 4)
>+          continue;

if (lineArray[0]->Equals(NS_LITERAL_CSTRING(kMatchTypeHost)) &&
lineArray.Count() == 4) {

>+        nsCAutoString typeString;
>+        nsCAutoString host;
>+        lineArray.CStringAt(1, typeString);
>+        lineArray.CStringAt(3, host);
>+        PRUint32 type = GetTypeIndex(typeString.get(), PR_TRUE);

hmm, do this instead:

PRUint32 type = GetTypeIndex(lineArray[1]->get(), PR_TRUE);

>+        rv = AddInternal(host, type, permission, PR_FALSE);

rv = AddInternal(*lineArray[3], type, permission, PR_FALSE);

and then kill typeString and host variables.

>@@ -737,89 +785,107 @@ nsPermissionManager::Write()
>+  // Start with reading the old file, and remember any data that
>+  // wasn't parsed, to put it right back into the new file.

hmm, i wonder if we really need to do this. if we add a new type (e.g.
"regex"), we could always bump the version number of the file... it would avoid
this complexity, at the cost of old mozillae not reading any permissions at
all.

on this note, do we want to be deleting older versions once we've converted
them? e.g. if we read in a cookperm.txt and write out a hostperm.1, maybe we
should delete the old file... it might lead to user confusion if they switch
back to an older mozilla, and see a bunch of older permissions lurking about.
it also wouldn't be good to have a bunch of old permission files lurking in
everyone's profile, by the time we get to hostperm.4...

we also need a "cruft policy", so that e.g. each release, we remove
compatibility code to read permission files from three releases ago. so we
could remove the cookperm.txt-related code in 1.9?

>+  nsCStringArray rememberList;
>+  nsCOMPtr<nsIInputStream> fileInputStream;
>+  rv = NS_NewLocalFileInputStream(getter_AddRefs(fileInputStream), mPermissionsFile);
>+  if (NS_SUCCEEDED(rv)) {
>+    nsCOMPtr<nsILineInputStream> lineInputStream = do_QueryInterface(fileInputStream, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);

we don't want to abort writing if we couldn't get the unparsed strings... how
about this?

NS_NewLocalFileInputStream(getter_AddRefs(fileInputStream), mPermissionsFile);
nsCOMPtr<nsILineInputStream> lineInputStream =
do_QueryInterface(fileInputStream);
if (lineInputStream) {
  ...

>+      if ((typeLength = buffer.FindChar(kTab))) {
>+        const nsASingleFragmentCString &matchType = Substring(buffer, 0, typeLength);
>+        if (matchType.Equals(kMatchTypeHost))
>+          continue;
>+      }

since we know kMatchTypeHost doesn't contain tabs, how about just...

if (StringBeginsWith(buffer, NS_LITERAL_CSTRING(kMatchTypeHost)))
  continue;

(and then you could fold it into the |continue| statement above it)

>+  /* format is:
>+   * matchtype  \t type \t permission \t host \t

extra space again

>+  nsCAutoString line;
>+  for (PRInt32 i = 0 ; i < rememberList.Count() ; ++i) {
>+    rememberList.CStringAt(i, line);
>+    bufferedOutputStream->Write(PromiseFlatCString(line).get(), 
>+                                line.Length(), &rv);
>+    bufferedOutputStream->Write(&kNew, 1, &rv);
>   }

for (PRInt32 i = 0; i < rememberList.Count(); ++i) {
  bufferedOutputStream->Write(rememberList[i]->get(),
			      rememberList[i]->Length(),
			      &rv);
  bufferedOutputStream->Write(&kNew, 1, &rv);
}


>   // Make a copy of the pointer, so we can increase it without loosing
>   // the original pointer

losing, not loosing.

>-  for (i = 0; i < mHostCount; ++i) {
>+  for (PRUint32 i = 0; i < mHostCount; ++i) {

i think some compilers (with broken scoping) will complain that you redeclare i
here. beware of that when you check in.

>+      if (permission && mTypeArray[type]) {
>+        // Write matchtype
>+        // only host is possible at the moment.

s/host/"host"/

>+        char permissionString[5];
>+        PRUint32 len = PR_snprintf(permissionString, sizeof(permissionString) - 1, "%d", permission);

a real nitpick here, but... s/%d/%u/ ?

r=me with those addressed, but i think we need to decide on the three points i
raised. what are your thoughts mvl/darin?

Comment 7

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

>>@@ -737,89 +785,107 @@ nsPermissionManager::Write()
>>+  // Start with reading the old file, and remember any data that
>>+  // wasn't parsed, to put it right back into the new file.
>
>hmm, i wonder if we really need to do this. if we add a new type (e.g.
>"regex"), we could always bump the version number of the file... it would avoid
>this complexity, at the cost of old mozillae not reading any permissions at
>all.

Imo, the nice thing of the patch is that you can add new types without much
effort. No reading old files, no cruft poilicy, no hostperm.56, just add it. So
hopefully, it will take a while before we need hostperm.2.
Also, older mozillas just only see the types they know, and ignore the rest,
wothout deleting them. I think this is just cleaner to use, although the code is
a little bit more messy. (But being able to read n old version will be messy too
:) )

Comment 8

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

Attachment: patch v2

Updated the patch to dwittes review comments, except for what i defendend in
the previous comment.

Comment 9

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

Comment on attachment 137812 [details] [diff] [review]
patch v2

Caarying forward dwittes r=, trying to get sr.
It would be good to land this as soon as possible, last changes here led to
some regressions....

Comment 10

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

>hmm, i wonder if we really need to do this. if we add a new type (e.g.
>"regex"), we could always bump the version number of the file... it would avoid
>this complexity, at the cost of old mozillae not reading any permissions at
>all.

Maybe we could go the "simple" road, and just create a new file for every type.
So we have perm.host for now. When we add regexes, we create perm.regex. Older
mozillas won't even look at the file, so the data want be lost. There also is no
need for the code to preserve unknown data. So it will be less bloaty, without
conflicts when new features are added. Also, less older files to worry about how
and when to delete.

Comment 11

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

Attachment: only store host based perms

attached a patch that only knows about host based permissions. If we add
regexes later, we need to decide if we just add a file (regexperm.1?) or bump
the current file.
This approach removes the code to keep all the unknown lines writing. If you
want to add stuff, just create a new file, or bump the version :)

Comment 12

[Darin Fisher]

Comment on attachment 137812 [details] [diff] [review]
patch v2

>Index: extensions/cookie/nsPermissionManager.cpp

>+    // Try finding the old-style file
>+    const char oldPermissionsFileName[] = "cookperm.txt";

declare static?


>+        if (!PL_strcmp(buffer.get() + stringIndex, "0F"))
>+          continue;

nit: use raw |strcmp| instead.


>+        CopyUCS2toASCII(bufferUnicode, buffer);

please use LossyCopyUTF16toASCII instead b/c it makes it clear
that you understand that this could be a lossy operation.  a
comment might be good to explain why this is actually ok.


sr=darin (sorry it took so long for me to get to this patch)

Comment 13

[Darin Fisher]

>on this note, do we want to be deleting older versions once we've converted
>them?

i don't think we want to do to much of this.  i know the profile could end up
with a bunch of old files, but seriously... i think there are folks out there
that switch between multiple versions of mozilla (at least, think about mozilla
and netscape 7).  we shouldn't prevent older versions from working as they did
prior to running a newer mozilla.

Comment 14

[[e-mail]]

ok, i vote for the former patch (patch v2), for greater backwards compatibility.
however, i'd like to see a small optimization for the vastly common case: store
a boolean flag that says whether unknown lines were encountered during ::Read(),
and then only re-parse the file in ::Write() if that boolean is true. saves
reading in the whole file again in ::Write(), for the common case of no unknown
lines.

Comment 15

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

Checked in patch v2, updated to darins and dwittes comments.

Comment 16

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

adding relnote keyword, because the release notes have a list of file names in
the profile, and cookperm.txt changed to hostperm.1
This should be updated for new releases.

Comment 17

[Gervase Markham [:gerv]]

Er... I know I'm a bit late to the party, but is it a bit late to change the
name of this file? "hostperm.1" is really obscure. Having read the bug, I can
see the reasoning - but I'm afraid I don't buy it. No other Mozilla files (or
any files I know of) use file extensions as a versioning scheme. 

Extensions are there to denote a filetype - the correct one in this case would
seem to be ".txt", as it's a textual file.

Gerv

Comment 18

[Darin Fisher]

(In reply to comment #17)
> see the reasoning - but I'm afraid I don't buy it. No other Mozilla files (or
> any files I know of) use file extensions as a versioning scheme. 
> 
> Extensions are there to denote a filetype - the correct one in this case would
> seem to be ".txt", as it's a textual file.

This file isn't for end-users, so why does the file extension matter?

Comment 19

[Peter Weilbacher]

I am an "end-user" of Mozilla and try to help other end-users in newsgroups. And
I and others have written tens of posts explaining other people where to find
the cookies and other permission in the profile. So it definitely makes sense to
give the file a name that describes what's inside!

Comment 20

[Darin Fisher]

(In reply to comment #19)
> I am an "end-user" of Mozilla and try to help other end-users in newsgroups. And
> I and others have written tens of posts explaining other people where to find
> the cookies and other permission in the profile. So it definitely makes sense to
> give the file a name that describes what's inside!

this presumes that we want to support end-users directly modifying these files.
 that isn't a goal AFAIK.  if it is, then i agree that the filename should
change.  assumption: advanced users that are willing to muck around with obscure
files in their profile won't really be hindered by file extensions.  if we just
think about what typical end-users care about, i think we can conclude fairly
easily that they won't care what we name this file.  they'll care a lot more
that we provide a good UI to control user permission settings.

Comment 21

[Gervase Markham [:gerv]]

> that isn't a goal AFAIK.

It may not be a goal, but it's totally inevitable. No-one thought that end users
would have to have anything to do with xul.mfl, which is why it was thought it
could have a different name on each platform - but they do. And the name causes
grief every time.

> assumption: advanced users that are willing to muck around with obscure
> files in their profile won't really be hindered by file extensions.

One part is convenience. What application is associated with .1 files on your
system? Or anyone else's?

Gerv

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

>One part is convenience. What application is associated with .1 files on your
>system? Or anyone else's?

my system does not determine file types by whatever happens to follow the last
dot in the name, so this is not an issue ;)

Comment 23

[Darin Fisher]

(In reply to comment #21)
> > assumption: advanced users that are willing to muck around with obscure
> > files in their profile won't really be hindered by file extensions.
> 
> One part is convenience. What application is associated with .1 files on your
> system? Or anyone else's?

no application is associated with .1 files on my WIN32 partition, and that's a
good thing IMO.  (right click, "open with" works for me in a pinch... or, vi
from my cygwin terminal -- i did say advanced users! ;-) )

how about this: what application is associated with .db on your system?  answer:
likely none.  why?  because, it's a binary file not meant to be edited by hand.
 it is just a matter of fact that hostperm.1 is a text file.  i would even like
to see it have default readonly permissions on supported platforms to help
discourage random editing in that file.

also, i don't think anyone is double-clicking .mfl files to edit them.  maybe
they need to locate the .mfl file in order to know to delete it, but that's a
whole different story. 

bottom line: i don't think we need to design this to be ideal or convenient for
users that want to hand edit hostperm.1

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

What's the point of the extension then? 

Calling the file "Site Permissions" (no extension) won't cause the file to be
auto-opened by anything, and will make Mac users or at least system designers
cheer. (Indeed, why are any of our data files named so obscurely. "Site
Passwords", "Download History", "Form History", "Browsing History", "User
Preferences", "Window States" etc are all self documenting file names that save
documentation folks on help sites some work - fact is, people do go editing
these files, whether we want them to or not. Quality should permeate all aspects
of the app. This is a nit, but the quality of an application is measured more
than in its total of features, it's in the details. 

Comment 25

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

> What's the point of the extension then? 

Future proof versioning. You can't use headers, because that will fail when you
switch to xml for example.
We want versioning to not have to come up with new filenames when the file
format is changed. Not that i expect that to happen any time soon, but you never
know. And why not think about it while you have the opportunity?

And on the filename: I just tried to make the old filename match up to reality
somewhat more, and add version info. I wouldn't have cared to give it an other
name (like "Site Permissions V1" or so), if someone mentioned it before. Now, it
is more a pain, because there must be backward compatibility code. And there
already is too much of that :(