712240 - Safari migrator does not import cookies

Status: RESOLVED WONTFIX

(Firefox :: Migration, defect, P3)

Description

[Asaf Romano (gone)]

Rewriting the Safari profile migrator in JS, I found out the the cookies in my Cookies.plist file we're trying to import are quite old. They're the cookies I had back when I used Snow Leopard.

The new cookies is named cookies.binarycookies, and its format isn't documented. This is not a change in Mac Safari itself, but an OS-level change. The new format is used even if you run old versions of Safari on 10.7 (with old WebKit framework etc.).

While there's an a objective-c api for reading these cookies, we probably can't and shouldn't use it. Can't, because I'm pretty sure ctypes cannot handle obj-c apis at this point.  Shouldn't, because the new migrator I'm writing doesn't use native mac apis at all. I wrote our own plist readers so the migrator can be adopted to support the the Windows version for Safari as well.  Speaking of Windows Safari, it also uses the new cookies format, starting with version 5.1, apparently.

I'm not likely to fix this as part of the initial rewrite. This bug will cover that later.

Comment 1

[Asaf Romano (gone)]

Support for the previous format (Cookies.plist) was dropped in the new migrator.

We can fix this on Mac by using Cocoa apis. Fixing this on windows would involve some reverse engineering we don't want to do.

Comment 2

[Asaf Romano (gone)]

Some notes:

1. I've a working solution for quite a while. It uses the ObjC API as noted above (namely NSHTTPCookie). Patch to be posted once I decide which unwanted xpcom api for this is most wanted, and once I figure out if it's worth spawning a thread.

 About that:
   - Initially, I used nsISimpleEnumerator, as we now do for ie-history, but this solution doesn't play nice with ObjC's fast enumeration ( http://developer.apple.com/library/ios/#documentation/cocoa/conceptual/objectivec/Chapters/ocFastEnumeration.html ), which requires us to do the iteration in the form of a simple for...in loop, with no enumerator object exposed. I'm kinda-sorta surprised there's no general-purpose xpcom interface with one doYourJob method, taking no arguments... I could use nsIObserver, or a new interface.
   - Spawning a thread: so, given that all cookies have to be read synchronously, and, on the other hand, that cookies have to be stored through the main thread, it might be a good idea to read the cookies in a background thread.  However, it may be too complex to manage.

2. Windows #1: Over the internet the file structure has been already described in detail, and there are at least two Windows apps which take advantage of this (both are not open source, so I'm speculating). I'm not saying we should that too, but, if Safari ever gains more market share on Windows, we should re-evaluate the decision I made here. This comment also applies to next note.

3. Windows #2: Alternatively, Safari's CFNetwork.dll could be used on windows, which, while not exactly documented, has open-sourced consumers (CookieJarWin.cpp at apple).

Comment 3

[Asaf Romano (gone)]

Attachment: patch that works nicely, but the interface isn't final

No idea how I forgot to post this after all, *ugh*.

It Just Works, as they say, but my Cocoa-foo isn't great, thus asking Steven for feedback ahead of finalizing the interface and requesting review from Marco.

Steven: The feedback request is only for the code in the new file, nsSafariMigratorCocoaUtils.mm. Thanks much!

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 652189 [details] [diff] [review]
patch that works nicely, but the interface isn't final

This basically looks fine to me.

> +  for (NSHTTPCookie* cookie in cookies) {

But this (new style) syntax might not work in gcc on OS X 10.6.X.  Let me check it out.

Do you mind if I take a week or so to get to it?

Comment 5

[Asaf Romano (gone)]

Sure thing!

I don't have any 10.6 build box to test the syntax issue by myself, but i'll let you know If I figured that somehow.

Comment 6

[Asaf Romano (gone)]

Steven?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Sorry for the delay :-(

> +  for (NSHTTPCookie* cookie in cookies) {

Now I realize it's much easier to rewrite this part of your patch using syntax that I know works properly in all of our build configurations.  Something like this:

NSEnumerator *enum = [cookies objectEnumerator];
while (NSHTTPCookie* cookie = [enum nextObject]) {

Comment 8

[Asaf Romano (gone)]

NP.

So, just that?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 652189 [details] [diff] [review]
patch that works nicely, but the interface isn't final

> So, just that?

Yes.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 14

[Ania]

This is expected behavior. Moving forward, we will drop supporting cookies import from all browsers.