752444 - Mobile and default bookmarks are missing after migration

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Device: Samsung Captivate
OS: Android 2.2

Steps to reproduce:
1. Install the latest Nightly XUL build
2. Open Fennec
3. Go to cnn.com and bookmark the page
4. Close Fennec
5. Upgrade to the latest Native Nightly build
6. Open Fennec and go to AwesomeBar > Bookmarks

Expected result:
After step 6, besides all default bookmarks (about:home, about:firefox, AMO and Support links), there is listed the bookmarked page at step 3 as well. Desktop Bookmarks folder should not be displayed after migration since Sync was not involved at all.

Actual result:
After step 6:
- all default bookmarks are missing except AMO
- Desktop Bookmarks directory was created, even if Sync was not involved. All sub-directories are empty.
- the page bookmarked at step 3, is not listed in Bookmarks tab. However, if I load the page from Fennec History, the bookmark star is yellow colored, so the page is still bookmarked.

Comment 1

[Adrian Tamas (:AdrianT)]

On HTC Desire (Android 2.2)  Nightly 15.0a1 XUL 2012-05-06 / Nightly 15.0a1 Native 2012-05-06, I get the same behavior if I don't have sync set up. However if sync is set up before migration all mobile bookmarks are gone including the AMO page. The Desktop bookmarks are correctly displayed.

Comment 2

[Cristian Nicolae (:xti)]

Attachment: screenshot

Comment 3

[Lucas Rocha (:lucasr)]

Christian, logcat output would be useful here btw.

Comment 4

[Cristian Nicolae (:xti)]

Attachment: captivate_log

Comment 5

[:Margaret Leibovic]

FWIW, I can reproduce this as well.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

I think the problem is this: Profile Migration checks moz_bookmarks_roots. It then remaps those places root folders to the root folders in LocalDB, and drops everything that doesn't have a known root folder. 

However, the "Mobile" folder is not listed in moz_bookmarks_roots. This is perhaps a bug in XUL Fennec?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

The Desktop Bookmarks folder is displayed because the other root folders are migrated, and LocalBrowserDB checks for their presence to present the Desktop Bookmarks subfolder.

Because these are present in the old places db, they will end up existing in every XUL profile, even if there were no "real" bookmarks added below them.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

QAwanted to retest on a couple devices and see how wide the scope of this is.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

It's dataloss for every user who has mobile bookmarks. I'll patch the missing Mobile root in Profile Migrator. 

Not sure where to best fix the Desktop Bookmarks issue, doesn't look so easy in Migrator, maybe better in LocalBrowserDB or even on the UI side. But that's not dataloss so much less impact.

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

OK that is what we thought on triage. We just wanted to make sure this was as bad as the bug read.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Taking, though I'll split the cosmetic part (empty Desktop Bookmarks still showing) to a separate bug.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Split the display issue in Bug 753534.

Comment 13

[:Margaret Leibovic]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #9)

> Not sure where to best fix the Desktop Bookmarks issue, doesn't look so easy
> in Migrator, maybe better in LocalBrowserDB or even on the UI side. But
> that's not dataloss so much less impact.

Instead of checking if there are bookmarks with parents other than the mobile/root folders, we could explicitly check if there are bookmarks with the toolbar/menu/unsorted parents, since that's actually what we're using in the UI. That would just involve editing this query:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#351

Comment 14

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #13)
> (In reply to Gian-Carlo Pascutto (:gcp) from comment #9)
> 
> > Not sure where to best fix the Desktop Bookmarks issue, doesn't look so easy
> > in Migrator, maybe better in LocalBrowserDB or even on the UI side. But
> > that's not dataloss so much less impact.
> 
> Instead of checking if there are bookmarks with parents other than the
> mobile/root folders, we could explicitly check if there are bookmarks with
> the toolbar/menu/unsorted parents, since that's actually what we're using in
> the UI. That would just involve editing this query:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/
> LocalBrowserDB.java#351

Actually, it's not completely straightforward, since we'd need to get the IDs for the toolbar/menu/unsorted folders to do that, but we're already doing that for the mobile bookmarks folder, so it's not impossible.

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: XUL to Native Logcat on Samsung Galaxy S II

Wierd.  This might be a separate bug.

STR:
1) have native nightly installed w/ Sync setup
2) uninstall any nightly
3) install XUL nightly 5/8/2012
4) go to ilm.com and bookmark
5) quit
6) adb install -r fennec-15.0a1.en-US.nightly_05_08_2012.apk 
7) look in awesome page for ilm.com bookmark

Expected: ilm.com bookmark in the bookmark category
Actual: Top sites and History shows www.ilm.com bookmarked; Bookmarks does not show www.ilm.com in any folder

It tried to Sync my desktop w/ nothing... I see a Desktop Bookmarks folder instead of my default bookmarks

Within my log I found this:
05-10 06:56:03.140: I/dalvikvm(30473): Could not find method android.database.sqlite.SQLiteDatabase.enableWriteAheadLogging, referenced from method org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.onOpen
05-10 06:56:03.140: W/dalvikvm(30473): VFY: unable to resolve virtual method 277: Landroid/database/sqlite/SQLiteDatabase;.enableWriteAheadLogging ()Z
05-10 06:56:03.140: D/dalvikvm(30473): VFY: replacing opcode 0x6e at 0x0033

As well as this :
05-10 06:56:03.605: V/Telemetry(30473): Sending telemetry: {"value":11,"name":"BROWSERPROVIDER_XUL_IMPORT_BOOKMARKS"}
05-10 06:56:03.605: I/ProfileMigrator(30473): Flushing 1 DB operations

~~~~~
1) is Sync uncoupling itself completely?
2) should migration create folders for Desktop Bookmarks if Sync isn't attached?

Comment 16

[Gian-Carlo Pascutto [:gcp]]

>1) is Sync uncoupling itself completely?

Your Sync migration failed because of bug 753175.

>2) should migration create folders for Desktop Bookmarks if Sync isn't attached?

Migration just migrates what's in the profile, and the profile has them. Displaying those empty folders or not is discussed in bug 753534 :)

Comment 17

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #16)
> >1) is Sync uncoupling itself completely?
> 
> Your Sync migration failed because of bug 753175.

The problem is that I didn't use sync at all in the migration.

> >2) should migration create folders for Desktop Bookmarks if Sync isn't attached?
> 
> Migration just migrates what's in the profile, and the profile has them.
> Displaying those empty folders or not is discussed in bug 753534 :)

When it has a fresh profile and has no sync installed?

Comment 18

[Gian-Carlo Pascutto [:gcp]]

>The problem is that I didn't use sync at all in the migration.

My mistake. The bug I mentioned stops Sync preferences from getting read, which is indistinguishable from not having any :P

>When it has a fresh profile and has no sync installed?

The display of the Desktop Bookmarks folder is triggered by the existence of some other folders, which are always present in XUL Fennec DB's and get migrated.

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Ok, thanks for the explanation.  So it was only confusing because the end user didn't see the same structure that they saw in XUL; so in a sense it's more or less a minor UI bug.

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Fix and add missing Mobile bookmarks root.

[Approval Request Comment]
User impact if declined: Loss of all mobile bookmarks if Sync is not used.

Comment 21

[JP Rosevear [:jpr]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #20)
> Created attachment 622588 [details] [diff] [review]
> Patch 1. Fix and add missing Mobile bookmarks root.
> 
> [Approval Request Comment]
> User impact if declined: Loss of all mobile bookmarks if Sync is not used.

Including user created ones, correct?  We should update the bug summary if that is the case and consider this for a beta respin.

Comment 22

[JP Rosevear [:jpr]]

Flagged to release-drivers for possible beta 1 respin.

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 622588 [details] [diff] [review]
Patch 1. Fix and add missing Mobile bookmarks root.

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

Looks good.

::: mobile/android/base/ProfileMigrator.java
@@ +819,5 @@
>                  }
>                  cursor.close();
> +
> +                // XUL Fennec doesn't mark the Mobile folder as a root,
> +                // so fix that up here.

Maybe a file a bug for that? Sounds like something that needs fixing on desktop Firefox.

Comment 24

[Gian-Carlo Pascutto [:gcp]]

>Maybe a file a bug for that? Sounds like something that needs fixing on desktop 
>Firefox.

Desktop Places DBs don't have a Mobile root folder. (That's one of the main reasons why this slipped by - I did a lot of my testing with huge desktop profiles)

Maybe it's worth fixing in XUL Fennec.

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Filed Bug 753789 for the root cause.

Comment 26

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1df2b3ab08cf

Comment 27

[JP Rosevear [:jpr]]

We'll need this in a beta 1 respin.

Comment 28

[JP Rosevear [:jpr]]

QA, please smoke test this.

Comment 29

[JP Rosevear [:jpr]]

Comment on attachment 622588 [details] [diff] [review]
Patch 1. Fix and add missing Mobile bookmarks root.

For beta 1 respin.

Comment 30

[:Margaret Leibovic]

Pushed to aurora tip and beta 1 branch:
https://hg.mozilla.org/releases/mozilla-aurora/rev/bfeacc6e275b
https://hg.mozilla.org/releases/mozilla-aurora/rev/14ce1b841676

Comment 31

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/1df2b3ab08cf

Comment 32

[Cristian Nicolae (:xti)]

This bug is not reproducible any more on the latest Nightly and Native Beta build 3: http://s13.postimage.org/ay6hih8j9/device_2012_05_11_142027.png

The only noticeable thing is that the keywords are not migrated on Native ( Bug 754224 ). Also AMO/mobile seems that's still present for Native ( Bug 754236 ). Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-11), Firefox 14.b3
Samsung Galaxy Nexus
Android: 4.0.2