1062984 - [System] Browser bookmarks not migrated to homescreen icons on OTA from 2.0 to 2.1

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Brogan Zumwalt [Inactive]]

Attachment: Logcat

Description:
When user has existing browser bookmarks on 2.0 then performs an OTA update to 2.1, the bookmarks are not migrated to homescreen icons after update. Any existing homescreen icons migrate correctly, but no new homescreen icons are created for existing browser bookmarks.
   
Repro Steps:
1) Update a Flame device to 2.0 BuildID: 20140822000206
2) Change update channel and url on device to appropriate 2.1 build (20140904000203)
3) Ensure that Browser bookmarks exist, if not, create one or more bookmarks
4) Perform OTA update
  
Actual:
Existing browser bookmarks are not migrated to homescreen icons after 2.0 to 2.1 OTA.
  
Expected: 
Browser bookmarks become homescreen icons after 2.0 to 2.1 OTA.
  
Environmental Variables:
Device: Flame 2.1 (319mb)
BuildID: 20140904000203
Gaia: a47ecb6368c015dd72148acde26413fd90ba3136
Gecko: 757931d0149e
Version: 34.0a2 (2.1)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Repro frequency: 5/5, 100%
See attached: Youtube video clip & logcat

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

From logcat:

---

09-04 09:57:54.826: E/GeckoConsole(2512): Content JS LOG at app://system.gaiamobile.org/js/migrators/browser_migrator.js:49 in onUpgradeNeeded: Browser db upgrade needed, migrating so doesn't matter.

09-04 09:57:54.836: E/GeckoConsole(2512): [JavaScript Error: "NotFoundError: The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened." {file: "app://system.gaiamobile.org/js/migrators/browser_migrator.js" line: 86}]

---

This means something went wrong with the database copy step in the profile. Will investigate.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

[Blocking Requested - why for this release]:

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

[Blocking Requested - why for this release]: Bookmark migration not working.

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Already nominated 2.1 by Kyle.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok. Looks like we're resolving the wrong path to the IDB file on the device. It's in /data/local/, but we look in the profile directory. 

This passed my local tests because the scripts I wrote just copied a DB into the profile directory without checking that's where it was supposed to be. Oops. 

Just need to figure out how to resolve that correctly based on the platform.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Fix directory service query for IDB storage directory on phone

Tested this on phone by creating 2.0 profile, then hardcoding upgrade code paths to always fire. Bookmark appeared on homescreen.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8484688 [details] [diff] [review]
Patch 1 (v1) - Fix directory service query for IDB storage directory on phone

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

I would rather do the js equivalent of http://mxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManager.cpp#969
That will be more robust when we move indexedDB in the profile directory on device.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Make directory service query for bookmark migration more resilient

Removed platform ifdefs, now check profile directory if indexedDBPDir doesn't exist.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8486089 [details] [diff] [review]
Patch 1 (v2) - Make directory service query for bookmark migration more resilient

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

Maybe that would be easier to read by refactoring the "check id this file exists either in ProfD or indexedDBPDir" ?

::: b2g/components/B2GAppMigrator.js
@@ +48,5 @@
> +
> +    // On the phone, the browser db will only be in the old IDB
> +    // directory, since it only existed up until v2.0. On desktop, it
> +    // will exist in the profile directory.
> +    let browserDBFile = FileUtils.getFile(kIDBDirType,

getFile will create the subdirectories if they are not there, which is not desirable. Can you just use a nsIFile ?

@@ +62,5 @@
> +                                        ["storage",
> +                                         "persistent",
> +                                         browserAppStorageDirName,
> +                                         "idb",
> +                                         browserDBFileName], true);

and here too.

@@ +94,5 @@
> +        return;
> +      }
> +    }
> +
> +    

nit: whitespace!

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Make directory service query for bookmark migration more resilient

Make sure bookmark migration doesn't create directories along the way.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Blocking on gaia fix to migration.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8488301 [details] [diff] [review]
Patch 1 (v3) - Make directory service query for bookmark migration more resilient

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

::: b2g/components/B2GAppMigrator.js
@@ +98,5 @@
> +        return;
> +      }
> +    }
> +
> +    

nit: whitespace.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/56d1ead5277f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56d1ead5277f

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8488301 [details] [diff] [review]
Patch 1 (v3) - Make directory service query for bookmark migration more resilient

Approval Request Comment
[Feature/regressing bug #]: Bug 938171
[User impact if declined]: Bookmarks won't migrate, causes data loss
[Describe test coverage new/current, TBPL]: Hand tested by QA
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 16

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad25b160ac30

Comment 17

[Tony Chung [:tchung]]

brogan, can you verify this is fixed on 2.1, and mark it off when done?  thanks!

Comment 18

[Jason Smith [:jsmith]]

Comment on attachment 8488301 [details] [diff] [review]
Patch 1 (v3) - Make directory service query for bookmark migration more resilient

Mentioned offline - this needs post build verification, so verifyme is needed here.

Comment 19

[Jason Smith [:jsmith]]

Peter is working on getting a QA contact to look into this, so I'm adding needinfo to him to set the QA contact to declare who's testing this.

Comment 20

[Peter Bylenga [:PBylenga]]

Josh Schmitt will work on verifying this, setting flags.

Comment 21

[Josh Schmitt [Joshs]]

Issue still reproduces, this bug has been re-written as bug 1069064

Comment 22

[Brogan Zumwalt [Inactive]]

This issue still reproduces on Flame 2.2. Existing bug 1077715

Actual Results: Existing browser bookmarks are not migrated to homescreen icons after 2.0 to 2.2 OTA. 


This issue is verified fixed on Flame 2.1

Actual Results: Browser bookmarks become homescreen icons after 2.0 to 2.1 OTA.

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141022001201
Gaia: 3d9cc667f4e929861a9a77c41096bbf5a9c1bde0
Gecko: 928b18f7d8ff
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0