742815 - Probing migration status can launch multiple migrations

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Attachment: 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.

Comment 3

[Richard Newman [:rnewman]]

(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!

Comment 4

[Gian-Carlo Pascutto [:gcp]]

>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 5

[Lucas Rocha (:lucasr)]

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.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d047ef4fe29

Comment 7

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/966e23109ac2

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v2 Fix double launch on status probe

Comment 10

[Lucas Rocha (:lucasr)]

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?

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/3d047ef4fe29

Comment 12

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/966e23109ac2

Comment 13

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8facffe7ff

Comment 14

[Richard Newman [:rnewman]]

(Setting inbound flags.)

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/aa8facffe7ff