862127 - profile migrators do all sorts of main thread I/O

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Mostly a tracking bug - fixing this may require adjusting the APIs. I suppose bug 718280 could help. Since they're all JS now, it should be relatively easy to switch to OS.File, assuming an async API?

Comment 1

[Matthew N. [:MattN]]

I'd like to recommend that we create automated tests (bug 700898 with a WIP in bug 706017) before making significant changes to migrator code.

Comment 2

[Marco Bonardo [:mak]]

For file access, sure, we could move to OS.File.
other parts may be synchronous just cause there's no async API usable, in case of bookmarks for example.
Btw, did this come out of measurements or from reports of migrators being slow?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

It came out of me doing MXR searches for main thread I/O :)

I was mostly referring to the I/O from reading/copying from the "other" profiles, as opposed to our APIs for saving the migrated data.

Comment 4

[Asaf Romano (gone)]

Already working on this silently for some time now, so taking.

Comment 5

[Florian Quèze [:florian]]

I think you worked on migrators not too long ago, do you know if this bug is still relevant?

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Florian Quèze [:florian] from comment #5)
> I think you worked on migrators not too long ago, do you know if this bug is
> still relevant?

Yes, definitely. See e.g. this query: https://dxr.mozilla.org/mozilla-central/search?q=netutil%20path%3Amigration&=mozilla-central . Really, the migration code needs rewriting to use async functions and await() and async OMT IO.

However... this is only used when migrating data from other browsers (or importing/exporting JSON/HTML bookmark files), which most users don't do (even when we show them that crappy wizard that suggests they do on first run). So not sure how much of a win it'd be to fix this.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to :Gijs from comment #6)
> (In reply to Florian Quèze [:florian] from comment #5)
> > I think you worked on migrators not too long ago, do you know if this bug is
> > still relevant?
> 
> Yes, definitely. See e.g. this query:
> https://dxr.mozilla.org/mozilla-central/
> search?q=netutil%20path%3Amigration&=mozilla-central . Really, the migration
> code needs rewriting to use async functions and await() and async OMT IO.
> 
> However... this is only used when migrating data from other browsers (or
> importing/exporting JSON/HTML bookmark files), which most users don't do
> (even when we show them that crappy wizard that suggests they do on first
> run). So not sure how much of a win it'd be to fix this.

Also, I believe Doug is working on this stuff (bug 1421703), so he might be interested in this bug. :-)

Comment 8

[Alex Thayer [:alexical] (she/her)]

Yeah, all of this is on the table for the purposes of opening up automigration (or similar techniques that we'd want to try that don't use a wizard). However the main thread IO for reading from the other browsers is, as far as I can observe, not the cause of most of our migration jank. Right now we still have a lot of work to do to reduce the main thread time spent processing all the data we bring in with that IO (notifying observers, encrypting logins, etc). But after that the main thread IO is probably next on the chopping block.

Comment 9

[Florian Quèze [:florian]]

(In reply to Doug Thayer [:dthayer] from comment #8)

> However the main thread IO for reading from the other browsers is,
> as far as I can observe, not the cause of most of our migration jank.

I wonder what you are observing to reach this conclusion. Main thread IO causes pauses for unbound amounts of time, sometimes minutes. Looking at a profile captured on a fast developer computer with an SSD never shows this.

Comment 10

[Alex Thayer [:alexical] (she/her)]

(In reply to Florian Quèze [:florian] from comment #9)
> I wonder what you are observing to reach this conclusion. Main thread IO
> causes pauses for unbound amounts of time, sometimes minutes. Looking at a
> profile captured on a fast developer computer with an SSD never shows this.

Sorry, I hadn't looked closely enough at this. We're focused on Chrome imports right now, and all that remains as far as I can see of main thread IO for that is for the sourceHomePageURL (I'm not sure this is even relevant for Chrome anymore? This doesn't seem to map to any setting I can find).

The other browsers still have quite a bit of main thread IO left over and you're right that that should be a priority if we want to get automigration off the ground for these browsers.

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Doug Thayer [:dthayer] from comment #10)
> (In reply to Florian Quèze [:florian] from comment #9)
> > I wonder what you are observing to reach this conclusion. Main thread IO
> > causes pauses for unbound amounts of time, sometimes minutes. Looking at a
> > profile captured on a fast developer computer with an SSD never shows this.
> 
> Sorry, I hadn't looked closely enough at this. We're focused on Chrome
> imports right now, and all that remains as far as I can see of main thread
> IO for that is for the sourceHomePageURL (I'm not sure this is even relevant
> for Chrome anymore? This doesn't seem to map to any setting I can find).

From a quick look, I'm fairly sure we still use main-thread IO for reading the list of profiles (which we generally need to do before (auto)import), and then a bunch of nsIFile.exists() and isReadable() calls (which are technically main-thread IO, I think?) in getResources() and the things it calls (to determine if Chrome has any importable resources), right?

As for sourceHomePageURL, I think it's offered in this pane of the wizard: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js#274 . It's possible it's dead for Chrome if the import is broken for some reason, I'm not sure without trying.

Really, we should change getResources(), the `migrate()` function on it, and `get sourceProfiles()` to be async/await-based, which will then mean we can pretty much drop-in-replace some of it with OS.File, which will be relatively straightforward. As a nice bonus, doing that will help with de-XPCOM-taminating the code.

But as you rightly point out, whether this needs to be prioritized right now depends on the impact it's having. I don't know if, in the real (non-developer-machine-with-SSD) world, file.exists() is cheap enough that it's reasonably fast, or not.

Comment 12

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #11)

> I don't know if, in the real
> (non-developer-machine-with-SSD) world, file.exists() is cheap enough that
> it's reasonably fast, or not.

nsIFile.exists() uses stat, and it's main thread I/O, similar to a blocking read call in terms of cost: it's cheap most of the time and every once in a while blocks for more than 100ms (and seconds when extremely unlucky).

Comment 13

[Florian Quèze [:florian]]

Also, there's a common bad pattern that does:
  if (file.exists())
    file.read();

This does main thread I/O twice, when:
  try {
    file.read();
  } catch(e) {}
would produce the same result for about half the cost.

But I haven't looked at the import code to check if this is the reason why .exist is being used.

Comment 14

[Marco Bonardo [:mak]]

(In reply to :Gijs from comment #11)
> > Sorry, I hadn't looked closely enough at this. We're focused on Chrome
> > imports right now, and all that remains as far as I can see of main thread
> > IO for that is for the sourceHomePageURL (I'm not sure this is even relevant
> > for Chrome anymore? This doesn't seem to map to any setting I can find).

IIRC in the past we discussed about completely removing the homepage migration, because in a lot of cases the homepage stays uncustomized and is very specific for the source browser and we feel like our homepage (even more with Activity Stream) can be more useful for the average user (provided any user can still customize it).

Comment 15

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 862127 - Make migration interfaces more async

In order to clean up sync IO within our profile migrators, we
need to have async interfaces for those parts which are currently
doing sync IO. This converts the sync interfaces and adjusts most
of the call sites (migration.js call site changes are addressed
in a separate patch to break it out a bit).

Review commit: https://reviewboard.mozilla.org/r/212592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212592/

Comment 16

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 862127 - Convert migration.js to use async migration interfaces

migration.js is a special case where we generally need blocking
calls in order for the wizard to work correctly. Accordingly we
block waiting for the new async interfaces. With automigration
and potential new UIs that are in the works for migration, the
asynchronicity of these interfaces will be more relevant, but
here it's not really important enough to make big changes to the
way the UI is implemented.

Review commit: https://reviewboard.mozilla.org/r/212594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212594/

Comment 17

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 862127 - Remove all sync IO from Chrome migration

Now that the interfaces are all async, we can simply replace all
the sync IO in the Chrome migrator with the equivalent async IO.
Other browsers will be addressed in separate patches.

Review commit: https://reviewboard.mozilla.org/r/212596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212596/

Comment 18

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 862127 - Fix migrator tests to use async interfaces

Review commit: https://reviewboard.mozilla.org/r/212598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212598/

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8942330 [details]
Bug 862127 - Make migration interfaces more async

https://reviewboard.mozilla.org/r/212592/#review218614

Comment 20

[:Gijs (out for now; he/him)]

Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

https://reviewboard.mozilla.org/r/212594/#review218616

::: browser/components/migration/content/migration.js:46
(Diff revision 1)
>        this._source = args[1];
>        this._migrator = args[2] instanceof kIMig ? args[2] : null;
>        this._autoMigrate = args[3].QueryInterface(kIPStartup);
>        this._skipImportSourcePage = args[4];
>        if (this._migrator && args[5]) {
> -        let sourceProfiles = this._migrator.sourceProfiles;
> +        let sourceProfiles = MigrationUtils.spinResolve(this._migrator.getSourceProfiles());

I'm pretty nervous about this, mostly because I think in theory this can race with people clicking e.g. the forward/next button, and I bet things go pearshaped pretty quickly in that case (ie if we try to move forward without the first on`<whatever>` having run to completion.

Can you check what happens if you make getSourceProfiles() never resolve, and if it's possible to move forward or if the migrator stays stuck on the previous page or... something?

Comment 21

[:Gijs (out for now; he/him)]

Comment on attachment 8942330 [details]
Bug 862127 - Make migration interfaces more async

https://reviewboard.mozilla.org/r/212592/#review218618

::: browser/components/migration/nsIBrowserProfileMigrator.idl:58
(Diff revision 1)
> -   * Whether or not there is any data that can be imported from this
> +   * Get whether or not there is any data that can be imported from this
>     * browser (i.e. whether or not it is installed, and there exists
>     * a user profile)
> +   * @return a promise that resolves with a boolean.
>     */
> -  readonly attribute boolean          sourceExists;
> +  jsval getSourceExists();

Nit: 'getFooExists' is kinda awkward. Maybe `isSourceAvailable` ?

Comment 22

[:Gijs (out for now; he/him)]

Comment on attachment 8942332 [details]
Bug 862127 - Remove all sync IO from Chrome migration

https://reviewboard.mozilla.org/r/212596/#review218622

r=me with the below resolved one way or another (esp. the homepage URL thing)

::: browser/components/migration/ChromeProfileMigrator.js:137
(Diff revision 1)
>  ChromeProfileMigrator.prototype.getLastUsedDate =
> -  function Chrome_getLastUsedDate() {
> -    let datePromises = this.sourceProfiles.map(profile => {
> -      let basePath = OS.Path.join(this._chromeUserDataFolder.path, profile.id);
> -      let fileDatePromises = ["Bookmarks", "History", "Cookies"].map(leafName => {
> +  async function Chrome_getLastUsedDate() {
> +    let sourceProfiles = await this.getSourceProfiles();
> +    let chromeUserDataPath = await this._getChromeUserDataPathIfExists();
> +    if (!chromeUserDataPath) {
> +      throw new Error("Can't get last used date without User Data folder.");

Maybe just return `new Date(0)` ? Looks like this can't currently throw, so I'm a bit hesitant to make it so.

::: browser/components/migration/ChromeProfileMigrator.js
(Diff revision 1)
>        return resources && resources.length > 0;
> -    }, this);
> +    }, this).map(({profile}) => profile);
>      return this.__sourceProfiles;
>    };
>  
> -Object.defineProperty(ChromeProfileMigrator.prototype, "sourceHomePageURL", {

I know that it's annoying to have to update this, too, but I think it'd be better. You can presumably use the same pattern you've used for the other things in the first set of patches.

Alternatively, please talk to UX and/or Product about removing this. Michael Verdi and/or Peter Dolanjski are probably the right people to talk to. I agree with Marco that this is probably low-value, but we shouldn't just remove it in a tech/perf/cleanup patch without talking to folks.

::: browser/components/migration/ChromeProfileMigrator.js:327
(Diff revision 1)
> -  cookiesFile.append("Cookies");
> +  if (!(await OS.File.exists(cookiesPath)))
> -  if (!cookiesFile.exists())

Can you file a followup to make these and any other things that follow the pattern:

```js
if (foo.exists()) {
  // read foo
}
```

or 

```js
if (!foo.exists()) {
  return null
}
// read foo
```

just read `foo` and fail silently if the resulting error indicates the file doesn't exist?

Comment 23

[:Gijs (out for now; he/him)]

Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

https://reviewboard.mozilla.org/r/212598/#review218628

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils_path.js
(Diff revision 1)
>      expectedPath = OS.Path.join(getRootPath(), ".config", "google-chrome");
>    }
>    Assert.equal(chromeUserDataPath, expectedPath, "Should get the path of Chrome user data directory.");
>  });
> -
> -add_task(async function test_getExtensionPath_function() {

Out of curiosity, why remove this?

Comment 24

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

https://reviewboard.mozilla.org/r/212594/#review218616

> I'm pretty nervous about this, mostly because I think in theory this can race with people clicking e.g. the forward/next button, and I bet things go pearshaped pretty quickly in that case (ie if we try to move forward without the first on`<whatever>` having run to completion.
> 
> Can you check what happens if you make getSourceProfiles() never resolve, and if it's possible to move forward or if the migrator stays stuck on the previous page or... something?

It does allow the onpageadvanced and onpagerewound events to fire. What about something like this to disable those buttons while we're waiting for the promise to resolve?

```
  spinResolve(promise) {
    let canAdvance = this._wiz.canAdvance;
    let canRewind = this._wiz.canRewind;
    let canCancel = this._canCancel;
    this._wiz.canAdvance = false;
    this._wiz.canRewind = false;
    this._canCancel = false;
    let result = MigrationUtils.spinResolve(promise);
    this._wiz.canAdvance = canAdvance;
    this._wiz.canRewind = canRewind;
    this._canCancel = canCancel;
    return result;
  },

  onWizardCancel() {
    return this._canCancel;
  },
```

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Doug Thayer [:dthayer] from comment #24)
> Comment on attachment 8942331 [details]
> Bug 862127 - Convert migration.js to use async migration interfaces
> 
> https://reviewboard.mozilla.org/r/212594/#review218616
> 
> > I'm pretty nervous about this, mostly because I think in theory this can race with people clicking e.g. the forward/next button, and I bet things go pearshaped pretty quickly in that case (ie if we try to move forward without the first on`<whatever>` having run to completion.
> > 
> > Can you check what happens if you make getSourceProfiles() never resolve, and if it's possible to move forward or if the migrator stays stuck on the previous page or... something?
> 
> It does allow the onpageadvanced and onpagerewound events to fire. What
> about something like this to disable those buttons while we're waiting for
> the promise to resolve?

That seems OK to me, yeah.
> ```
>   onWizardCancel() {
>     return this._canCancel;
>   },
> ```

The only thing I wonder is if it's worth allowing cancel, but perhaps we can just leave that for a followup and play it safe for now.

Comment 26

[Alex Thayer [:alexical] (she/her)]

(In reply to :Gijs from comment #25)
> The only thing I wonder is if it's worth allowing cancel, but perhaps we can
> just leave that for a followup and play it safe for now.

Yeah - I specifically added the cancel case in because canceling successfully closes the wizard, but the rest of Firefox is unresponsive until the promise resolves. We could have cancel force a stop on the spinEventLoopUntil call, though.

Comment 27

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942330 [details]
Bug 862127 - Make migration interfaces more async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212592/diff/1-2/

Comment 28

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212594/diff/1-2/

Comment 29

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942332 [details]
Bug 862127 - Remove all sync IO from Chrome migration

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212596/diff/1-2/

Comment 30

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212598/diff/1-2/

Comment 31

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942332 [details]
Bug 862127 - Remove all sync IO from Chrome migration

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212596/diff/2-3/

Comment 32

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212598/diff/2-3/

Comment 33

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

https://reviewboard.mozilla.org/r/212598/#review218628

> Out of curiosity, why remove this?

Woops! I had removed the extensions stuff since it wasn't used, before I realized that it was added in order to be consumed later. Tried to undo all of that, but must have missed this.

Comment 34

[:Gijs (out for now; he/him)]

(In reply to Doug Thayer [:dthayer] from comment #26)
> (In reply to :Gijs from comment #25)
> > The only thing I wonder is if it's worth allowing cancel, but perhaps we can
> > just leave that for a followup and play it safe for now.
> 
> Yeah - I specifically added the cancel case in because canceling
> successfully closes the wizard, but the rest of Firefox is unresponsive
> until the promise resolves. We could have cancel force a stop on the
> spinEventLoopUntil call, though.

That seems like a good idea. Review will have to wait until tomorrow (it's 11.30pm here now) but either here or in a follow-up is fine.

Comment 35

[:Gijs (out for now; he/him)]

Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

https://reviewboard.mozilla.org/r/212594/#review219724

r=me with the nits below fixed and a follow-up for making cancel immediately kill the spin loop.

Thanks for working on this!

::: browser/components/migration/content/migration.js:250
(Diff revisions 1 - 2)
>      var dataSources = document.getElementById("dataSources");
>      while (dataSources.hasChildNodes())
>        dataSources.firstChild.remove();
>  
> -    var items = MigrationUtils.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
> +    var items = this.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
>                                                                           this._autoMigrate));

Nit: please fix the alignment of the arguments.

::: browser/components/migration/content/migration.js:361
(Diff revisions 1 - 2)
>      if (this._autoMigrate)
>        this._itemsFlags =
> -        MigrationUtils.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
> +        this.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
>                                                                   this._autoMigrate));
>  

Can you align the arguments for `getMigrateData` "properly" and put braces around the contents of this if block?

Comment 36

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212594/diff/2-3/

Comment 37

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942332 [details]
Bug 862127 - Remove all sync IO from Chrome migration

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212596/diff/3-4/

Comment 38

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212598/diff/3-4/

Comment 39

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1bb73e8f20f442bea0b101834942f24469356acb -d f5fe81e72056: rebasing 443753:1bb73e8f20f4 "Bug 862127 - Make migration interfaces more async r=Gijs"
merging browser/components/migration/360seProfileMigrator.js
merging browser/components/migration/ChromeProfileMigrator.js
merging browser/components/migration/EdgeProfileMigrator.js
merging browser/components/migration/FirefoxProfileMigrator.js
merging browser/components/migration/IEProfileMigrator.js
merging browser/components/migration/MigrationUtils.jsm
merging browser/components/migration/SafariProfileMigrator.js
warning: conflicts while merging browser/components/migration/360seProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/ChromeProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/FirefoxProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/IEProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/SafariProfileMigrator.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 40

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942330 [details]
Bug 862127 - Make migration interfaces more async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212592/diff/2-3/

Comment 41

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212594/diff/3-4/

Comment 42

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942332 [details]
Bug 862127 - Remove all sync IO from Chrome migration

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212596/diff/4-5/

Comment 43

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212598/diff/4-5/

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c73e6be9d2d
Make migration interfaces more async r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f7a3e9638b6f
Convert migration.js to use async migration interfaces r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e650035e766f
Remove all sync IO from Chrome migration r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ea23e995ded7
Fix migrator tests to use async interfaces r=Gijs

Comment 45

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/8c73e6be9d2d
https://hg.mozilla.org/mozilla-central/rev/f7a3e9638b6f
https://hg.mozilla.org/mozilla-central/rev/e650035e766f
https://hg.mozilla.org/mozilla-central/rev/ea23e995ded7