726024 - Some of the desktop bookmarks are put under "Mobile Bookmarks" after profile migration

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Camelia Urian]

Build ID XUL: Mozilla/5.0 (Android; Linux armv7l; rv:13.0a1) Gecko/20120209 Firefox/13.0a1 Fennec/13.0a1 
Device: Samsung Google Nexus S
OS: Android 4.0

Steps to reproduce:
1. On the XUL build Perform Sync with a large profile(have many bookmarks)
2. Verify that bookmarks where correctly synced to device.
3. Go to About:fennec
4. Check for updates- install update
5. Go to 

Expected Result:
1. Bookmarks are listed correctly according to Mobile or Desktop.

Actual Result:
1. Some of the desktop bookmarks are displayed under Mobile.

Comment 1

[:Margaret Leibovic]

Did you already have a "Mobile Bookmarks" folder on desktop from previously syncing with XUL fennec?

Comment 2

[Camelia Urian]

(In reply to Margaret Leibovic [:margaret] from comment #1)
> Did you already have a "Mobile Bookmarks" folder on desktop from previously
> syncing with XUL fennec?

Yes, I had a "Mobile Bookmarks" folder on desktop, but I had only 6 bookmarks in it. After sync on device those 6 bookmarks were displayed under "Mobile Bookmarks", but also many other desktop bookmarks were listed under "Mobile Bookmarks"

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Camelia Urian from comment #2)
> (In reply to Margaret Leibovic [:margaret] from comment #1)
> > Did you already have a "Mobile Bookmarks" folder on desktop from previously
> > syncing with XUL fennec?
> 
> Yes, I had a "Mobile Bookmarks" folder on desktop, but I had only 6
> bookmarks in it. After sync on device those 6 bookmarks were displayed under
> "Mobile Bookmarks", but also many other desktop bookmarks were listed under
> "Mobile Bookmarks"

I think the migration is adding all bookmarks to mobile. This is going to be fixed once migration starts using the ContentProvider directly, I guess? gcp?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Yes. There is no bug for that yet, so I'll use this one.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Use ContentProvider directly. Import extra fields for Sync.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 597500 [details] [diff] [review]
Patch 1. Use ContentProvider directly. Import extra fields for Sync.

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

Looking good, but worth talking to lucasr about how best to ensure that folders are added in the correct order…

::: mobile/android/base/ProfileMigrator.java
@@ +98,5 @@
> +        + "favicon.url AS f_url, favicon.guid AS f_guid "
> +        + "FROM ((moz_places AS places JOIN moz_bookmarks AS bookmarks ON "
> +        + "places.id = bookmarks.fk) LEFT OUTER JOIN moz_favicons AS favicon "
> +        + "ON places.favicon_id = favicon.id) "
> +        + "WHERE places.hidden <> 1";

I reformatted this to make it easier to read; you might want to drop it in the patch.

		private final String bookmarkQuery =
        "SELECT places.url             AS p_url,"        +
        "       places.title           AS p_title,"      +
        "       places.guid            AS p_guid,"       +
        "       bookmarks.parent       AS b_parent,"     +
        "       bookmarks.dateAdded    AS b_added,"      +
        "       bookmarks.lastModified AS b_modified,"   +
        "       bookmarks.position     AS b_position,"   +
        "       favicon.data           AS f_data,"       +
        "       favicon.mime_type      AS f_mime_type,"  +
        "       favicon.url            AS f_url,"        +
        "       favicon.guid           AS f_guid "       +
        "FROM ((moz_places         AS places "           +
        "       JOIN moz_bookmarks AS bookmarks "        +
        "       ON places.id = bookmarks.fk) "           +
        "      LEFT OUTER JOIN moz_favicons AS favicon " +
        "      ON places.favicon_id = favicon.id) "      +
        "WHERE places.hidden <> 1";

@@ +115,4 @@
>  
>      /*
>        The sort criterion here corresponds to the one used for the
>        Awesomebar results. It's an simplification of Frecency.

s/an/a

@@ +178,5 @@
> +                if (c != null)
> +                    c.close();
> +            }
> +            // 0 is the top-level root folder
> +            return 0;

Const named FIXED_ROOT_ID. (Reuse?)

@@ +265,5 @@
> +                cursor = mCr.query(getHistoryUri(),
> +                                  projection,
> +                                  History.URL + " = ?",
> +                                  new String[] { url },
> +                                  null);

Nit: indenting.

@@ +269,5 @@
> +                                  null);
> +
> +                if (cursor.moveToFirst()) {
> +                    ContentValues values = new ContentValues();
> +

We can reuse these ContentValues, no? Saves a lot of allocations…

@@ +270,5 @@
> +
> +                if (cursor.moveToFirst()) {
> +                    ContentValues values = new ContentValues();
> +
> +                    values.put(History.VISITS, cursor.getInt(1) + visits);

Lift that magic number.

@@ +271,5 @@
> +                if (cursor.moveToFirst()) {
> +                    ContentValues values = new ContentValues();
> +
> +                    values.put(History.VISITS, cursor.getInt(1) + visits);
> +                    values.put(History.DATE_LAST_VISITED, date);

What about History.URL?

@@ +290,5 @@
> +                    if (title != null) {
> +                        values.put(History.TITLE, title);
> +                    } else {
> +                        values.put(History.TITLE, url);
> +                    }

Seems like you could share most of both branches of this if/else, and just have visits and insert/update decided by the conditional…

@@ +303,5 @@
> +
> +        protected void addFavicon(String url, String faviconUrl, String faviconGuid,
> +                                  String mime, ByteBuffer data) {
> +            // Some GIFs can cause us to lock up completely
> +            // without exceptions or anything. Not cool.

File a bug, put the bug number in the comment…

@@ +307,5 @@
> +            // without exceptions or anything. Not cool.
> +            if (mime == null || mime.compareTo("image/gif") == 0) {
> +                return;
> +            }
> +            // Decode non-PNG images.

Seems like this could be a separate method. Hooray reuse!

@@ +430,5 @@
> +            values.put(Bookmarks.IS_DELETED, 0);
> +            if (mRerootMap.containsKey(parent)) {
> +                parent = mRerootMap.get(parent);
> +            }
> +            values.put(Bookmarks.PARENT, parent);

It seems like you're going to run into issues with Bug 723841 here…

@@ +472,5 @@
> +                    String faviconGuid = (String)resultRow[faviconGuidCol];
> +
> +                    addBookmark(url, title, guid, parent, added, modified, position);
> +                    addFavicon(url, faviconUrl, faviconGuid,
> +                               faviconMime, faviconDataBuff);

Wrap these in a `try` with logging. Best to migrate some, even in the case of some unexpected failure.

@@ +477,3 @@
>                  }
>              } catch (SQLiteBridgeException e) {
> +                Log.e(LOGTAG, "Failed to get bookmarks: " + e.getMessage());

Log.e(LOGTAG, "Failed to get bookmarks.", e);

@@ +513,5 @@
>              } catch (SQLiteBridgeException e) {
>                  if (db != null) {
>                      db.close();
>                  }
> +                Log.e(LOGTAG, "Error on places database:" + e.getMessage());

Log.e(LOGTAG, "Error on places database.", e);

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 597500 [details] [diff] [review]
Patch 1. Use ContentProvider directly. Import extra fields for Sync.

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

Looking generally good but I'd like to see answers to my questions and nits fixed before giving r+.

::: mobile/android/base/ProfileMigrator.java
@@ +110,5 @@
> +    private final String bookmarkPosition = "b_position";
> +    private final String faviconData      = "f_data";
> +    private final String faviconMime      = "f_mime_type";
> +    private final String faviconUrl       = "f_url";
> +    private final String faviconGuid      = "f_guid";

nits: we don't usually align assignments like this in Fennec. Also, we usually prefix private members with 'm'. It would be nice to keep some consistency at least for the member names here.

@@ +147,5 @@
>      public void launch() {
>          new PlacesTask().run();
>      }
>  
>      private class PlacesTask implements Runnable {

For a separate patch: maybe rename PlacesTask to PlacesRunnable? Task has a specific connotation in the Android world.

@@ +162,5 @@
> +        protected Uri getImagesUri() {
> +            return Images.CONTENT_URI;
> +        }
> +
> +        private long getFolderId(String folderName) {

folderName -> guid?

@@ +178,5 @@
> +                if (c != null)
> +                    c.close();
> +            }
> +            // 0 is the top-level root folder
> +            return 0;

This looks a bit suspicious. The top-level root folder should not contain non-special folders/bookmarks. I'd return the id of the UNFILED folder in case the root folder cannot be found for some reason.

What are the cases where the folder id cannot be found here? Is it a bug if that happens? Or is it an expected edge case?

@@ +194,5 @@
> +
> +                for (Object[] resultRow: queryResult) {
> +                    String name = (String)resultRow[rootCol];
> +                    long placesFolderId = Integer.parseInt((String)resultRow[folderCol]);
> +                    mRerootMap.put(new Long(placesFolderId), new Long(getFolderId(name)));

I think you don't need "new Long(placesFolderId)" here. You can simply use placesFolderId directly. Same for getFolderId(), you can use directly here.

@@ +430,5 @@
> +            values.put(Bookmarks.IS_DELETED, 0);
> +            if (mRerootMap.containsKey(parent)) {
> +                parent = mRerootMap.get(parent);
> +            }
> +            values.put(Bookmarks.PARENT, parent);

You'll have to make sure you're adding bookmarks in parent order because I'll be adding a foreign key constraint to the parent col.

@@ +444,5 @@
>          }
>  
>          protected void migrateBookmarks(SQLiteBridge db) {
>              try {
>                  ArrayList<Object[]> queryResult = db.query(bookmarkQuery);

Is this loading all bookmarks in memory?

@@ +461,5 @@
>                  for (Object[] resultRow: queryResult) {
>                      String url = (String)resultRow[urlCol];
>                      String title = (String)resultRow[titleCol];
> +                    String guid = (String)resultRow[guidCol];
> +                    long parent = Long.parseLong((String)resultRow[parentCol]);

How is the id from the places DB transferred to the new DB? I mean, the parent col has a reference to a bookmark folder id and you don't seem to be ensuring that the ids are the same on migrated bookmarks (i.e. you're relying on the autoincremented ids). Or am I misreading something here?

::: mobile/android/base/db/BrowserProvider.java.in
@@ +680,5 @@
> +                    values.put(Bookmarks.DATE_CREATED, now);
> +                }
> +                if (!values.containsKey(Bookmarks.DATE_MODIFIED)) {
> +                    values.put(Bookmarks.DATE_MODIFIED, now);
> +                }

I'd put this change in a separate patch.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Allow times to be specified on BrowserProvider inserts.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Use ContentProvider directly. Import extra fields for Sync.

Richard, I need confirmation one one thing: should bookmarks use the GUID from moz_places or the GUID from moz_bookmarks? The previous patch had it from places, but *bookmark folders* don't refer to any places entry, so I think that was a mistake and it should have been the bookmarks GUID instead.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 3. Rename Task into Runnable.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

>nits: we don't usually align assignments like this in Fennec. Also, we usually 
>prefix private members with 'm'. It would be nice to keep some consistency at 
>least for the member names here.

They're constants, not fields, so I renamed everything to kXXXX. AS for aligning assignments, why not do it when it makes sense and improves readability? Just look at the top of GeckoApp.java, for example. 

>For a separate patch: maybe rename PlacesTask to PlacesRunnable? Task has a 
>specific connotation in the Android world.

I know - it was an AsyncTask before. Fixed.

>This looks a bit suspicious. The top-level root folder should not contain 
>non-special folders/bookmarks. I'd return the id of the UNFILED folder in case the 
>root folder cannot be found for some reason.
>
>What are the cases where the folder id cannot be found here? Is it a bug if that 
>happens? Or is it an expected edge case?

As mentioned on IRC, the problem is that this code runs before UNFILED and friends even exist. Once you get your patches for that landed, this should be indicative of a bug.

>Is this loading all bookmarks in memory?

Yes. SQLiteBridge doesn't support cursors (or keeping state at all) yet. At 2kb for a bookmark with favicon, we'll OOM at about 30 000 bookmarks. Given that we can't import such an amount in a reasonable time, I think this'll do for now.

I addressed the other complaints and the comments from rnewman (where appropriate). The principal thing was to ensure bookmarks aren't added before their parents are, and that we remap all the folder ids.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 598210 [details] [diff] [review]
Patch 1. Allow times to be specified on BrowserProvider inserts.

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

::: mobile/android/base/db/BrowserProvider.java.in
@@ +686,4 @@
>                  long now = System.currentTimeMillis();
> +                if (!values.containsKey(Bookmarks.DATE_CREATED)) {
> +                    values.put(Bookmarks.DATE_CREATED, now);
> +                }

nit: add empty line here.

@@ +688,5 @@
> +                    values.put(Bookmarks.DATE_CREATED, now);
> +                }
> +                if (!values.containsKey(Bookmarks.DATE_MODIFIED)) {
> +                    values.put(Bookmarks.DATE_MODIFIED, now);
> +                }

nit: add empty line here.

Comment 13

[Richard Newman [:rnewman]]

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

> Richard, I need confirmation one one thing: should bookmarks use the GUID
> from moz_places or the GUID from moz_bookmarks? The previous patch had it
> from places, but *bookmark folders* don't refer to any places entry, so I
> think that was a mistake and it should have been the bookmarks GUID instead.

Correct.

The exact query we use for bookmarks is:

      "SELECT guid " +
      "FROM moz_bookmarks " +
      "WHERE id = :item_id"

The only time JS Sync bookmarks code touches moz_places is to retrieve the frecency value for a URL.

For history, JS Sync uses the moz_places GUID; all of its values come from moz_places, with the one exception of accesses to moz_historyvisits.

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 598211 [details] [diff] [review]
Patch 2. Use ContentProvider directly. Import extra fields for Sync.

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

::: mobile/android/base/ProfileMigrator.java
@@ +129,5 @@
> +    private final String kBookmarkPosition = "b_position";
> +    private final String kFaviconData      = "f_data";
> +    private final String kFaviconMime      = "f_mime_type";
> +    private final String kFaviconUrl       = "f_url";
> +    private final String kFaviconGuid      = "f_guid";

not: add empty line here.

::: mobile/android/base/db/BrowserContract.java.in
@@ +101,5 @@
>  
>      public static final class Bookmarks implements CommonColumns, URLColumns, ImageColumns, SyncColumns {
>          private Bookmarks() {}
>  
> +        public static final int ROOT_FOLDER_ID = 0;

Skip this as I'm doing exactly the same thing in bug 723841.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +335,5 @@
>              values.put(Bookmarks.IS_FOLDER, 1);
>              values.put(Bookmarks.POSITION, 0);
>  
>              // Set the parent to 0, which sync assumes is the root
> +            values.put(Bookmarks.PARENT, Bookmarks.ROOT_FOLDER_ID);

Skip this change as this code will be very different after I land my patch for bug 723841. I guess your patch depends on the migration to land first right?

Comment 15

[Gian-Carlo Pascutto [:gcp]]

>I guess your patch depends on the migration to land first right?

Not really. It will work but everything will be below ROOT_FOLDER_ID instead of places/etc.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab31ba136025
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aed1f777177
https://hg.mozilla.org/integration/mozilla-inbound/rev/d40608e2e594

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab31ba136025
https://hg.mozilla.org/mozilla-central/rev/2aed1f777177
https://hg.mozilla.org/mozilla-central/rev/d40608e2e594

Comment 18

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-03-01)
20120301031135
http://hg.mozilla.org/mozilla-central/rev/1c3b291d0830

--
Device: Samsung Galaxy S2
OS: Android 2.3.4