Closed Bug 742815 Opened 8 years ago Closed 8 years ago

Probing migration status can launch multiple migrations

Categories

(Firefox for Android :: General, defect)

14 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: gcp, Assigned: gcp)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Duplicate of this bug: 743054
Blocks: 743098
Depends on: 725150
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!
>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+
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.
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+
https://hg.mozilla.org/mozilla-central/rev/3d047ef4fe29
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Backed out: https://hg.mozilla.org/mozilla-central/rev/966e23109ac2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.