Closed Bug 1309614 Opened 8 years ago Closed 8 years ago

Fix startup in case of migration so that places initialization doesn't race with importing data from other browsers

Categories

(Firefox :: Migration, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file)

In the beta 50 experiment, a lot of users passively disabled the "undo" functionality for automatic migration by creating/modifying bookmarks. The number of users that hit this case was much higher than expected.

After investigating some of the code at issue, Marco and I somewhat suspect that at least some of these users are hitting a case where the initial creation of some default bookmark structures triggers our observer so we "prematurely" disable automatic migration. We should fix the startup sequence and/or the observers so that this doesn't happen.
Comment on attachment 8800731 [details]
Bug 1309614 - finish initializing places before we import stuff,

https://reviewboard.mozilla.org/r/85602/#review85104

::: browser/components/migration/MigrationUtils.jsm:343
(Diff revision 1)
> +        Services.obs.removeObserver(onPlacesInited, TOPIC_PLACES_DEFAULTS_FINISHED);
> +        doMigrate();
> +      };
> -        let onImportComplete = function() {
> +      let onImportComplete = function() {
> +        Services.obs.addObserver(onPlacesInited, TOPIC_PLACES_DEFAULTS_FINISHED, false);
> -          browserGlue.observe(null, TOPIC_DID_IMPORT_BOOKMARKS, "");
> +        browserGlue.observe(null, TOPIC_DID_IMPORT_BOOKMARKS, "");

I'm not sure I understand..

We send initial-migration-will-import-default-bookmarks so nsBrowserglue does NOT invoke _initPlaces.

Then we wait for places-browser-init-complete... but this is fired by _initPlaces once it's done.

when we get (never?) places-browser-init-complete we send initial-migration-did-import-default-bookmarks that calls _initPlaces...

Something's wrong or am I misreading this?
From what I see looks like now we always skip initing Places if there is a migration...

::: browser/components/nsBrowserGlue.js:1610
(Diff revision 1)
> +        try {
> -        yield this._distributionCustomizer.applyBookmarks();
> +          yield this._distributionCustomizer.applyBookmarks();
> -        yield this.ensurePlacesDefaultQueriesInitialized();
> +          yield this.ensurePlacesDefaultQueriesInitialized();
> +        } catch (e) {
> +          Cu.reportError(e);
> +        }

I agree this is not critical, but could you please explain why we need to try/catch it now?
If it's to be sure we always notify finishing, you coud probably do that using the fact we are in a task, so you could 

Task.spawn(...).catch(ex => ...).then(notify)

It actually seems to make sense.
Attachment #8800731 - Flags: review?(mak77)
Comment on attachment 8800731 [details]
Bug 1309614 - finish initializing places before we import stuff,

https://reviewboard.mozilla.org/r/85602/#review85118

::: browser/components/migration/MigrationUtils.jsm:343
(Diff revision 1)
> +        Services.obs.removeObserver(onPlacesInited, TOPIC_PLACES_DEFAULTS_FINISHED);
> +        doMigrate();
> +      };
> -        let onImportComplete = function() {
> +      let onImportComplete = function() {
> +        Services.obs.addObserver(onPlacesInited, TOPIC_PLACES_DEFAULTS_FINISHED, false);
> -          browserGlue.observe(null, TOPIC_DID_IMPORT_BOOKMARKS, "");
> +        browserGlue.observe(null, TOPIC_DID_IMPORT_BOOKMARKS, "");

We send `initial-migration-will-import-default-bookmarks` here, then we call `importFromURL` to get default bookmarks, then we send `initial-migration-did-import-default-bookmarks` (so TOPIC_DID vs TOPIC_WILL import bookmarks) which triggers `_initPlaces` in nsBrowserGlue, so we wait for that to have finished, and then we continue with the migration.

Would comments help? Or renaming the topic constants, or reorganizing the code somehow, maybe using Task w/ yields instead of the callback-y code?

::: browser/components/nsBrowserGlue.js:1610
(Diff revision 1)
> +        try {
> -        yield this._distributionCustomizer.applyBookmarks();
> +          yield this._distributionCustomizer.applyBookmarks();
> -        yield this.ensurePlacesDefaultQueriesInitialized();
> +          yield this.ensurePlacesDefaultQueriesInitialized();
> +        } catch (e) {
> +          Cu.reportError(e);
> +        }

It's in a try catch a little further down (in the else block) when we run the same code, so I thought we should be consistent.

A .catch would also work. The difference would be how much of the remaining code would still run. I'll "and and" and do both, I guess. Leaving the issue open for now.
Comment on attachment 8800731 [details]
Bug 1309614 - finish initializing places before we import stuff,

https://reviewboard.mozilla.org/r/85602/#review85118

> We send `initial-migration-will-import-default-bookmarks` here, then we call `importFromURL` to get default bookmarks, then we send `initial-migration-did-import-default-bookmarks` (so TOPIC_DID vs TOPIC_WILL import bookmarks) which triggers `_initPlaces` in nsBrowserGlue, so we wait for that to have finished, and then we continue with the migration.
> 
> Would comments help? Or renaming the topic constants, or reorganizing the code somehow, maybe using Task w/ yields instead of the callback-y code?

ah sorry, I missed that onImportComplete is invoked by importFromURL... maybe using a task would help clarifying the order?
Comment on attachment 8800731 [details]
Bug 1309614 - finish initializing places before we import stuff,

https://reviewboard.mozilla.org/r/85602/#review85612

::: browser/components/nsBrowserGlue.js:1696
(Diff revision 2)
>            }
>          }
>          this._idleService.addIdleObserver(this, this._bookmarksBackupIdleTime);
>        }
>  
>        Services.obs.notifyObservers(null, "places-browser-init-complete", "");

I still think sending this notification should be moved to a .then after the .catch, so we are sure it's always fired, since stuff begins to depend on it to proceed.
Attachment #8800731 - Flags: review?(mak77) → review+
Comment on attachment 8800731 [details]
Bug 1309614 - finish initializing places before we import stuff,

https://reviewboard.mozilla.org/r/85602/#review85612

> I still think sending this notification should be moved to a .then after the .catch, so we are sure it's always fired, since stuff begins to depend on it to proceed.

Good idea, done!
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fe3994d56a50
finish initializing places before we import stuff, r=mak
https://hg.mozilla.org/mozilla-central/rev/fe3994d56a50
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment on attachment 8800731 [details]
Bug 1309614 - finish initializing places before we import stuff,

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: places initialization in a new profile will race with automigration. This could cause its own problems, but the main concern is that it will sometimes lead to the "undo" functionality of automigration to be disabled prematurely. We would like to avoid that in order to get more data about why/when/how often users don't want the data we automatically migrated from other browsers.
[Describe test coverage new/current, TreeHerder]: this is on the startup codepath. Unfortunately there isn't a whole lot of testing of the case where we do migrate data, but we will definitely be automatically testing the other (more common case) extensively.
[Risks and why]: low-ish - the change is relatively straightforward, and fixes indeterminacy which is usually a source of bugs. Aurora is getting on a bit, but we'll still have the full beta cycle and some weeks on aurora to shake out issues with this patch. Note especially also that the beta cycle for 51 will be particularly long, lasting over 2 months.
[String/UUID change made/needed]: nope.
Attachment #8800731 - Flags: approval-mozilla-aurora?
(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 8800731 [details]
> Bug 1309614 - finish initializing places before we import stuff,
> 
> Approval Request Comment
> [Feature/regressing bug #]: n/a

Oops. So this is related to the automatic migration feature which we'd like to test again in beta 51 (we already did some testing on beta 50). We likely won't ship with 51, but because of the nature of startup migration (it only happens when you've got a fresh Firefox install with no previous Firefox data on your machine) testing only on nightly/aurora won't give us the data we need to make decisions about release.
Comment on attachment 8800731 [details]
Bug 1309614 - finish initializing places before we import stuff,

Fix a startup issue related to automigration. Take it in 51 aurora.

Hi Florin,
Can this verification be added in tests of bug 1279240?
Flags: needinfo?(florin.mezei)
Attachment #8800731 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Hi Gijs,
Could you please detail how the auto migration feature works? 
I have tried to reproduce this issue on a Windows 10 x64 machine with Firefox Nightly 52.0a1 (id: 20161026030210). I only get the "auto-import from the default browser" screen and if I "skip" this step (by hitting "cancel" or closing the window, I suppose) the browser is opened and nothing else happens. Please note that I have Chrome as my default web browser set in Windows default programs.
How do I trigger the auto-migration process?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Alexandru Simonca, QA (:asimonca) from comment #15)
> Hi Gijs,
> Could you please detail how the auto migration feature works? 
> I have tried to reproduce this issue on a Windows 10 x64 machine with
> Firefox Nightly 52.0a1 (id: 20161026030210). I only get the "auto-import
> from the default browser" screen

Where does it say "auto"? This is the normal import wizard, I think.

> and if I "skip" this step (by hitting
> "cancel" or closing the window, I suppose) the browser is opened and nothing
> else happens. Please note that I have Chrome as my default web browser set
> in Windows default programs.
> How do I trigger the auto-migration process?

You would need a build with the pref set.

Here's some for Aurora:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7cf32229912e61dc6706c6f206619716552d248

Nightly:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a404f40259478c45d642da3deaf926e5725a47a7
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(alexandru.simonca)
(In reply to :Gijs Kruitbosch from comment #16)

> Where does it say "auto"? This is the normal import wizard, I think.

"In the beta 50 experiment, a lot of users passively disabled the "undo" functionality for automatic migration by creating/modifying bookmarks."

I can trigger the "normal import wizard" by deleting all profiles on the machine, but if I hit "Cancel" in that screen the browser opens and nothing else happens and if I do import data from other browsers the data is imported with no issues and, again nothing else happens. 
 
I am trying to trigger that message that prompts the user to "Keep" or "Don't Keep" the auto-imported data from other browsers so I can modify a bookmark and see if I can still "undo" the import or not. 

https://bug1248077.bmoattachments.org/attachment.cgi?id=8746784
Flags: needinfo?(alexandru.simonca) → needinfo?(gijskruitbosch+bugs)
(In reply to Alexandru Simonca, QA (:asimonca) from comment #17)
> (In reply to :Gijs Kruitbosch from comment #16)
> 
> > Where does it say "auto"? This is the normal import wizard, I think.
> 
> "In the beta 50 experiment, a lot of users passively disabled the "undo"
> functionality for automatic migration by creating/modifying bookmarks."
> 
> I can trigger the "normal import wizard" by deleting all profiles on the
> machine, but if I hit "Cancel" in that screen the browser opens and nothing
> else happens and if I do import data from other browsers the data is
> imported with no issues and, again nothing else happens. 

Right, we only offer undo if we imported *automatically*. In that case you would not see the wizard.

If you're trying to see issues with beta builds, you will need to use older beta builds - the automigration code was preffed off again already.

> I am trying to trigger that message that prompts the user to "Keep" or
> "Don't Keep" the auto-imported data from other browsers so I can modify a
> bookmark and see if I can still "undo" the import or not. 

So as noted, you would need a special build, and you would need to make sure Firefox is not the default browser when you run the initial build, so we automatically (without the wizard) import data from another browser (so you would also need to make sure that the default browser had some measure of data available on it).

But what you're describing isn't what this bug is about. It's expected that if you modify a bookmark, we disable the undo functionality. What we're trying to address is that this case seemed to be triggered by so many people, that we suspected this wasn't a manual bookmarks change those users were making, but somehow the "oh, a bookmark changed" observers were triggered without user interaction, by changes the rest of Firefox was making (automatically).

I don't know how easy it is to reproduce and verify this manually, tbh, and I'm not sure if trying to do so would be productive - unless you can reproduce problems with e.g. Firefox 50b5 where we don't offer you the undo option after we import things automatically, and telemetry indicates this is because you modified a bookmark (but you didn't do any such thing).
Flags: needinfo?(gijskruitbosch+bugs)
Marking this issue with qe-verify - based on Comment #18
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: