1155705 - Safari bookmarks migration is broken

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

This is a regression from bug 1123309, the proxy doesn't catch .has() calls.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1

I don't have a clean Safari profile at hand, so I'm going through Try before asking for review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dea79221e98f

Comment 2

[:Gijs (he/him)]

Lovely. I take it you checked Chrome as well by now and that still works?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I was doing bookmarks import testing for bug 1094876, Chrome seems to work, but I'm not done yet.

Though I suspect a default OS X profile doesn't have bookmarks.plist, thus my automated test might not work and we'd have to revert to manual testing... Let's see.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Well it's even worse, we don't detect Safari at all on tinderbox, cause likely the whole /Library/Safari folder doesn't exist.

We have 3 choices:
1. forget about the test, keep testing manually as we did until now (and we know nobody is doing that)
2. Manually create a Safari folder and copy a standard Bookmarks.plist into it. This will allow to test our internal code, but if Safari changes location or format for the file, we won't know. (fwiw, we can do the same for Chrome)
3. Always skip the test but leave it in-tree so every now and then someone can run it manually on OS X (by experience nobody will ever do...)

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The problem with 2. is that on a developer machine we'd break the user's bookmarks... so we'd need to tell the migrator where to find our fake Bookmarks.plist.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

and users bookmarks might also be a problem for 3...

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2s

So, we might do something like this, it is 2, with a fake Library folder. It is expandable to support more tests (on cookies, reading list and so on) but it's not detecting changes in Safari itself... Should be better than nothing.
Let's see if this would work:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee6c7119efda

Comment 8

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #5)
> The problem with 2. is that on a developer machine we'd break the user's
> bookmarks... so we'd need to tell the migrator where to find our fake
> Bookmarks.plist.

I'm confused, surely we could check if the file is there already beforehand? Becomes a bit difficult to then verify expectations though, I guess, and if we move the file out of the way, use our copy, and then move it back, that opens us up to race conditions, which doesn't sound attractive either.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Marco Bonardo [::mak] from comment #5)
> > The problem with 2. is that on a developer machine we'd break the user's
> > bookmarks... so we'd need to tell the migrator where to find our fake
> > Bookmarks.plist.
> 
> I'm confused, surely we could check if the file is there already beforehand?

Yes, but the test relies on certain position of bookmarks (or certain cookies being set and so on) and it would be very hard to know whether the system file is ok, and skipping automatically could end up skipping every time if tinderboxes grow a file the test doesn't like, so we would still not figure failures.

I think the patch in comment 7, so far, is the best bet... While it won't figure failures due to Safari/Chrome internal changes, at least it will ensure our code runs. 

> Becomes a bit difficult to then verify expectations though, I guess, and if
> we move the file out of the way, use our copy, and then move it back, that
> opens us up to race conditions, which doesn't sound attractive either.

I would not really want to play with users files :)

Comment 10

[:Gijs (he/him)]

Comment on attachment 8594124 [details] [diff] [review]
patch v2s

Review of attachment 8594124 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0ac453f0a1f6

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ac453f0a1f6

Comment 13

[Petruta Horea [:phorea], Desktop QA]

Verified under Mac OS X 10.9.5 with "Import Data from Another Browser" option from Library window:
 Nightly 40.0a1 2015-04-17 and Nightly 38.0a1 2015-01-17 have the same behavior, Bookmarks and History are imported, Cookies data are not.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we didn't implement cookies import from safari, yet.

Comment 15

[Petruta Horea [:phorea], Desktop QA]

Thanks, Marco.

There are some wrong builds id's in comment 13 above, I'm writing again for clarity. 

Verified under Mac OS X 10.9.5 with "Import Data from Another Browser" option from Library window:
 - Nightly 40.0a1 2015-04-28 and Nightly 38.0a1 2015-01-19 have the same behavior, Bookmarks and History are imported, Cookies data are not.
 - Nightly 40.0a1 2015-04-17 (when this bug was filled) doesn't import anything from Safari (Bookmarks, History and Cookies)

Should I make the same verification with the import start-up option that appears when no profiles are created?

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the code doing the actual import is the same in both cases.

Comment 17

[Petruta Horea [:phorea], Desktop QA]

Thank you, marking as verified in this case.