Safari bookmarks migration is broken

VERIFIED FIXED in Firefox 40

Status

()

Firefox
Migration
--
major
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({regression})

unspecified
Firefox 40
All
Mac OS X
regression
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox39 unaffected, firefox40 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

This is a regression from bug 1123309, the proxy doesn't catch .has() calls.
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
Created attachment 8594004 [details] [diff] [review]
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

3 years ago
Lovely. I take it you checked Chrome as well by now and that still works?
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.

Updated

3 years ago
Iteration: --- → 40.2 - 27 Apr
Blocks: 1094876
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...)
Flags: needinfo?(gijskruitbosch+bugs)
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.
and users bookmarks might also be a problem for 3...
Created attachment 8594124 [details] [diff] [review]
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

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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 :)
Attachment #8594124 - Flags: review?(gijskruitbosch+bugs)

Comment 10

3 years ago
Comment on attachment 8594124 [details] [diff] [review]
patch v2s

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

Nice!
Attachment #8594124 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8594004 - Attachment is obsolete: true

Comment 11

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/0ac453f0a1f6
https://hg.mozilla.org/mozilla-central/rev/0ac453f0a1f6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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.
we didn't implement cookies import from safari, yet.
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?
the code doing the actual import is the same in both cases.
Thank you, marking as verified in this case.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.