738263 - Default bookmarks import on migration is temporarily broken

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Due to bug 482911 we had to temporarily break default bookmarks import on initial migration, Mano will fix this here or in bug 737381.

This should be fixed before we merge FF14 to Aurora.

Comment 1

[Alex Keybl [:akeybl]]

(In reply to Marco Bonardo [:mak] from comment #0)
> This should be fixed before we merge FF14 to Aurora.

For Mano's sake, the uplift will occur on 4/24.

Comment 2

[Marco Bonardo [:mak]]

Attachment: part 1: explicitly break this.

This doesn't fix the bug, but as it is now we may overwrite migrated bookmarks, so better breaking it by will.

Comment 3

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/346f2964dc92

Comment 4

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/346f2964dc92

Comment 5

[Asaf Romano (gone)]

Fixed by bug 748569.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
Updating status flag to 'fixed' as per comment 5

Comment 7

[Paul Silaghi, QA [:pauly]]

I can't find any builds to see the broken import before this fix. Could you please direct me to one ?

Comment 8

[Marco Bonardo [:mak]]

any nightly build from 2012-04-04 to 2012-05-29 should have it broken, we are talking about default bookmarks, not generic import.  Like the Mozilla Firefox folder in Bookmarks Menu.

Comment 9

[Paul Silaghi, QA [:pauly]]

I see all the default bookmarks (Most Visited, Getting Started, Mozilla Firefox folder, Recently Bookmarked, Recent Tags) on new profiles on Nightly 2012-04-06, Nightly 2012-05-13, Nightly 2012-05-28. Am I missing something here ?

Comment 10

[Marco Bonardo [:mak]]

(In reply to Paul Silaghi [QA] from comment #9)
> I see all the default bookmarks

Is this when importing bookmarks from another browser? We broke them "on initial migration", so when a new profile is created and bookmarks are imported from a third party browser.

Comment 11

[Paul Silaghi, QA [:pauly]]

Thanks Marco. I can see the issue now. But the problem is I can also see it on the latest nightly 16.0a1 (2012-06-18).

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Paul, can you list the STR exactly? Perhaps in a separate bug - we should certainly investigate.

Comment 13

[Paul Silaghi, QA [:pauly]]

I'll post the STR here, because in fact it's the same issue.
STR:
1. Remove profiles folder
2. Start Firefox
3. Select Chrome from the migration wizard
4. Press Finish

Expected results: 
Firefox should start having all the default bookmarks (Most Visited, Getting Started, Mozilla Firefox folder, Recently Bookmarked, Recent Tags)

Actual results:
Default bookmarks (Most Visited, Getting Started, Mozilla Firefox folder, Recently Bookmarked, Recent Tags) are missing

Tested on Nightly 2012-06-21 on Win 7 64-bit.

Comment 14

[Asaf Romano (gone)]

Wow, yes, something is completely broken.

Comment 15

[Asaf Romano (gone)]

Bloody hell.

The reason this worked for me with my patch is because I didn't actually remove the profiles Firefox data folder. Instead, I launched firefox with --migration *and creted a new profile*. Apparently, profile manager->create a new profile->migration (during a single start-up!) is somehow different then the "natural" use case. Investigating now.

Comment 16

[Asaf Romano (gone)]

Er, wait, I cannot reproduce this at all with my own build, only with the nightly.

Comment 17

[Asaf Romano (gone)]

And comment 15 is wrong. In the nightly build I can reproduce this with --migration just as well as in the natural scenario.

Wondering what could be different...

Comment 18

[Asaf Romano (gone)]

In my own build, the default profile has a bookmarks.html when it's created (to which "BMarks" point). This file doesn't exist in the default profile created by the nightly build.

Comment 19

[Asaf Romano (gone)]

So...

I'm not sure how this bookmarks.html sneaks his way into new profiles in my homemade build, but it seems to me that I should not rely on it. I /think/ I should use "resource:///defaults/profile/bookmarks.html" (in the nightly case, this points to a file in the omnijar, and it is indeed there).

Comment 20

[Asaf Romano (gone)]

Attachment: Do so

Comment 21

[Asaf Romano (gone)]

Comment on attachment 635763 [details] [diff] [review]
Do so

Marco is away.

Comment 22

[Asaf Romano (gone)]

Pushed to try so I can test this on something close enough to the nightly builds. The builds will be available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cf487c1033f9

Comment 23

[Asaf Romano (gone)]

Indeed I cannot reproduce this bug in the try-build.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 635763 [details] [diff] [review]
Do so

[Triage Comment]
We'll want this on beta - let's get this on Aurora now, get an extra verification there just to be extra sure before approving for beta.

Comment 25

[Asaf Romano (gone)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c6c014006601
http://hg.mozilla.org/releases/mozilla-aurora/rev/a8b465235ab3

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6c014006601

Comment 27

[Asaf Romano (gone)]

Paul, could you test this again with a recent aurora build?

Comment 28

[Paul Silaghi, QA [:pauly]]

(In reply to Mano from comment #27)
> Paul, could you test this again with a recent aurora build?

Verified fixed on FF 15.0a2 (2012-06-24) on Win 7, Ubuntu 12.04 and Mac OS X 10.6.

Comment 29

[Asaf Romano (gone)]

Comment on attachment 635763 [details] [diff] [review]
Do so

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new migration system.
User impact if declined: default-bookmarks won't be included for users performing initial migration
Testing completed (on m-c, etc.): There are any tests for migration. Paul verified that the fix works though
Risk to taking this patch (and alternatives if risky): No risk. The patch only touches the use case involved. Worst case scenario - the bug won't be fixed completely for some users.
String or UUID changes made by this patch: none

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 635763 [details] [diff] [review]
Do so

[Triage Comment]
Low risk, and in support of the new migration feature. Approved!

Comment 31

[Asaf Romano (gone)]

http://hg.mozilla.org/releases/mozilla-beta/rev/5f1a74045fce

Comment 32

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 14b10 on Win 7, Ubuntu 12.04 and Mac OS X 10.6.