Probing migration status can launch multiple migrations

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

14 Branch
Firefox 14
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=725150#c28

rnewman pointed out that we can unintentionally launch multiple migrations. Unfortunately I already landed the code by then, so let's fix that issue here.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 743054
Blocks: 743098
Depends on: 725150
(Assignee)

Comment 2

6 years ago
Created attachment 613381 [details] [diff] [review]
Patch 1. Fix double launch on status probe

Regarding the remarks in bug 725150:
https://bugzilla.mozilla.org/show_bug.cgi?id=725150#c28

>? Seems like you want s/hasMigrationFinished/isHistoryMigrated, right?
>And do you really intend for both to be migrated when you check one? (That is, for 
>launch() to affect both?)

Profile Migration just keeps working as it always has: it migrates bookmarks first and then part of history. If called again (ENSURE_HISTORY_MIGRATED), it will migrate more history until the old places database is exhausted. It's still supposed to do a good job even if the user never bothers with Sync. Because of this hasMigrationFinished implies isHistoryMigrated, and it's very much intentional that launching it will affect both bookmarks and history, if appropriate.

I changed the (hasMigrationFinished/iHistoryMigrated) anyway in this patch to make it more symmetric and perhaps more future-proof.
Assignee: nobody → gpascutto
Attachment #613381 - Flags: review?(lucasr.at.mozilla)
(In reply to Gian-Carlo Pascutto (:gcp) from comment #2)
> Because of this hasMigrationFinished implies isHistoryMigrated, and
> it's very much intentional that launching it will affect both bookmarks and
> history, if appropriate.

I see.

Just wanted to make sure that you'd be getting the expected behavior when we call isBookmarksMigrated, get "no", then come right back and check history a few milliseconds later!
(Assignee)

Comment 4

6 years ago
>Just wanted to make sure that you'd be getting the expected behavior when we call 
>isBookmarksMigrated, get "no", then come right back and check history a few 
>milliseconds later!

In normal operation, I don't think it is possible for Sync to call and get a "no" to isBookmarksMigrated, because that runs at the first Fennec start. It would require Sync to be set up first (and create the profile?) and start the initial migration before Fennec ever had a chance to run.
Even if it did so, it would only happen once, so I think we're good.
Comment on attachment 613381 [details] [diff] [review]
Patch 1. Fix double launch on status probe

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

Nice.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1466,3 @@
>  
> +                if (needBookmarks || needHistory) {
> +                    migrator.launch();

nit: add empty line here.

@@ +1466,5 @@
>  
> +                if (needBookmarks || needHistory) {
> +                    migrator.launch();
> +                    needBookmarks = wantBookmarks && !migrator.isBookmarksMigrated();
> +                    needHistory = wantHistory && !migrator.isHistoryMigrated();

nit: add empty line here.
Attachment #613381 - Flags: review?(lucasr.at.mozilla) → review+
blocking-fennec1.0: --- → beta+
(Assignee)

Comment 7

6 years ago
This code is bugged: it doesn't respect the projection that is passed it when it's setting the results in the output cursor. I'm going to backout.
(Assignee)

Comment 9

6 years ago
Created attachment 613956 [details] [diff] [review]
Patch 1. v2 Fix double launch on status probe
Attachment #613381 - Attachment is obsolete: true
Attachment #613956 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 613956 [details] [diff] [review]
Patch 1. v2 Fix double launch on status probe

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

Nice.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1495,2 @@
>  
> +                boolean needBookmarks = wantBookmarks && !migrator.isBookmarksMigrated();

Wondering: shouldn't this be "areBookmarksMigrated() for correctness? :-) Also, needsBookmarks maybe?
Attachment #613956 - Flags: review?(lucasr.at.mozilla) → review+

Comment 11

6 years ago
https://hg.mozilla.org/mozilla-central/rev/3d047ef4fe29
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14

Comment 12

6 years ago
Backed out: https://hg.mozilla.org/mozilla-central/rev/966e23109ac2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8facffe7ff
Status: REOPENED → NEW
Target Milestone: Firefox 14 → ---
(Setting inbound flags.)
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/aa8facffe7ff
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.