397238 - Cookie import code should not need to be triplicated

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

Both Firefox and SeaMonkey's importers duplicate the cookie import code.

Comment 1

[[e-mail]]

Attachment: Export import

Comment 2

[[e-mail]]

Comment on attachment 282002 [details] [diff] [review]
Export import

nice idea... my only concern is, now you're exposing the import functionality and people can call it at any time, we need to make sure we stomp any existing cookies first. something like:

if (mCookieCount != 0)
  RemoveAll();

at the top of ImportCookies()? Also, please add a comment in the idl to that effect. Might want to add an NS_ENSURE_ARG(aCookieFile) at the top of ImportCookies() too.

can you test the non-empty case, to make sure things work as expected?

Comment 3

[[e-mail]]

one more thing:

+  // we're done importing - delete the old cookie file
+  return cookieFile->Remove(PR_FALSE);

the previous code ignored the rv of Remove(), which i think is correct - you should do the same here.

Comment 4

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

hm... wouldn't it be nice to allow importing cookies in addition to what we currently have, instead of replacing it?

Comment 5

[[e-mail]]

(In reply to comment #2)
>(From update of attachment 282002 [details] [diff] [review])
>nice idea... my only concern is, now you're exposing the import functionality
>and people can call it at any time, we need to make sure we stomp any existing
>cookies first. something like:
Actually, that was my whole idea; the migrators just copy the cookie file if it isn't there (so they may in fact be broken on trunk, because if the sqlite DB already exists then the cookie service will ignore the copied cookie file) but manually import cookies if the cookie file exists.

>Might want to add an NS_ENSURE_ARG(aCookieFile) at the top of ImportCookies() too.
Good point.

>can you test the non-empty case, to make sure things work as expected?
Sorry, I don't understand this bit.

(In reply to comment #3)
>>+  return cookieFile->Remove(PR_FALSE);
>the previous code ignored the rv of Remove(), which i think is correct
My callers ignore my return value anyway ;-)

Comment 6

[[e-mail]]

(In reply to comment #5)

Sorry for the delay in replying here!

> Actually, that was my whole idea; the migrators just copy the cookie file if it
> isn't there (so they may in fact be broken on trunk, because if the sqlite DB
> already exists then the cookie service will ignore the copied cookie file) but
> manually import cookies if the cookie file exists.

Are you saying they call nsICookieManager2::Add() to manually import cookies? If so, this will safely add cookies to the existing db. Your patch as-is will not do this, however... nsCookieService::ImportCookies() will call through to AddCookieToList() which is a low-level method that, well, directly adds cookies to the list without checks. In this context, it's designed to be called on an empty db. This will break things horribly if the db isn't empty.

Is there a reason you want to be able to import cookies to a db that isn't empty? Can you give some use cases? It means you're going to be mixing new cookies with old cookies, which might cause problems... and how do we decide which takes precedence? There's no creation date info stored in cookies.txt. My idea of import is that it's usually done once on first installation, but maybe I'm wrong?

> >can you test the non-empty case, to make sure things work as expected?
> Sorry, I don't understand this bit.

Can you test the import functionality when importing to an existing database with some cookies in it? To test the case with duplicated cookies, etc.

> (In reply to comment #3)
> >>+  return cookieFile->Remove(PR_FALSE);
> >the previous code ignored the rv of Remove(), which i think is correct
> My callers ignore my return value anyway ;-)

Sure, but future callers might not, and a failure here should rightly be ignored.

Comment 7

[[e-mail]]

(In reply to comment #6)
>Is there a reason you want to be able to import cookies to a db that isn't
>empty? Can you give some use cases? It means you're going to be mixing new
>cookies with old cookies, which might cause problems... and how do we decide
>which takes precedence? There's no creation date info stored in cookies.txt. My
>idea of import is that it's usually done once on first installation, but maybe
>I'm wrong?
The migration code allows you to merge a migration source into an existing profile. I didn't develop the migration backend. I assumed there was a good reason at the time, but as a suite, SeaMonkey now has its own reason, that is, importing from (say) both Thunderbird and Firefox into the same profile.

The existing migration code does not do this correctly; it assumes that if no cookies.txt file exists then this is because the profile has no cookies. There seem to be several ways that might resolve the problem:
1. Make the inital cookie import not delete cookies.txt (note that this fails if you create a profile, set a cookie, then try to migrate cookies).
2. Always import and delete cookies.txt if it exists (which fixes the migrator).
3. Expose an import method on the cookie service which uses the same internal parser but flagged to add the cookies safely instead of quickly.
4. Fix the migrator to always parse cookies.txt manually.

Do you have a preference, or another idea?

Comment 8

[[e-mail]]

(In reply to comment #7)
> importing from (say) both Thunderbird and Firefox into the same profile.

ok; that's a good reason... although, thunderbird shouldn't really have any cookies. ;)

> 2. Always import and delete cookies.txt if it exists (which fixes the
> migrator).
> 3. Expose an import method on the cookie service which uses the same internal
> parser but flagged to add the cookies safely instead of quickly.
> 4. Fix the migrator to always parse cookies.txt manually.

2) will break if the deletion of cookies.txt fails for some reason (eg it's write-protected or somesuch), so it seems fragile. 4) is safest but results in triplication. 3) seems like the best idea to me... and the precedence of which cookies overwrite others will be determined by the order of imports.

to do this, i don't think we need to pass in a flag to ImportCookies()... just check if the db isn't empty at the beginning of the function (mCookieCount != 0), and if so, use AddInternal() instead of AddCookieToList(). see Add() for an example of how to use AddInternal() in this instance, and note that we already have a PR_Now() value to pass in. i'm pretty sure this will work.

this will need some thorough testing w/ multiple imports and importing into an existing db, to make sure stuff doesn't break.

Comment 9

[[e-mail]]

(In reply to comment #8)
>(In reply to comment #7)
>>importing from (say) both Thunderbird and Firefox into the same profile.
>ok; that's a good reason... although, thunderbird shouldn't really have any
>cookies. ;)
Doesn't their RSS allow cookies? (Even if it doesn't, we don't want to force people to migrate Firefox first, just so that there definitely won't be any cookies yet.)

>to do this, i don't think we need to pass in a flag to ImportCookies()... just
>check if the db isn't empty at the beginning of the function (mCookieCount !=
>0), and if so, use AddInternal() instead of AddCookieToList(). see Add() for an
>example of how to use AddInternal() in this instance, and note that we already
>have a PR_Now() value to pass in. i'm pretty sure this will work.
I can do that, but I think I'll have to remove the Matrix comment; AddInternal doesn't return a status so I'll have to hold the cookie in an nsRefPtr.

Comment 10

[[e-mail]]

Attachment: Addressed review comments

I manually invoked some cookie imports to verify that
a) Completely new cookies were added
   (Tested by importing bugzilla cookies to a profile that had none)
b) Updated cookies replaced existing cookies
   (Tested by observing the effect of importing a bugzilla buglist cookie)
c) Otherwise existing cookies continued to exist
   (Tested by importing from a cookie file with no bugzilla cookies)

Comment 11

[[e-mail]]

Comment on attachment 283835 [details] [diff] [review]
Addressed review comments

>Index: public/nsICookieManager2.idl

>+  /**
>+   * Import an old-style cookie file
>+   *
>+   * @param aCookieFile the file to import, usually cookies.txt
>+   */
>+  void importCookies(in nsIFile aCookieFile);

can you elaborate on the additive behavior of this method in the comment? something like "imported cookies will be added to the existing database. if the database contains any cookies the same as those being imported, they will be replaced. the same rules for cookie equality and replacement are applied as are used when setting cookies - i.e. host, name, and path equality."

r=dwitte with that. thanks for the patch and testing!

Comment 12

[[e-mail]]

Comment on attachment 283835 [details] [diff] [review]
Addressed review comments

This patch makes it easier to fix the potential dataloss issue I mentioned in comment #5.

Comment 13

[[e-mail]]

Fix checked in.