718280 - deCOM migrators

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P3)

Description

[Asaf Romano (gone)]

Soon all migrators will be converted to JS. We should then stop wrapping each migrator in a component, and remove nsIBrowserProfileMigrator.

Comment 1

[Asaf Romano (gone)]

Here are the remaining steps (after landing bug 718608):

1. JS Safari & IE migrators (both are almost done).

2.1. Make migrationUtils.getMigrator return the wrappedJSObject of the migrator.
2.2. Use sourceProfiles+getResources directly from migration.js rather than getMigrateData+migrate.  In other words, move/copy a lot of the code introduced in MigrationUtils to migration.js.
2.3. Use sourceProfiles+getResources in getMigraor for checking its existence, rather than sourceExists.

3.1. In MigrationUtils, create a migratorKey->script mapping, and load the migrators using loadSubScript.
3.2. Copy the migration types from the interface to MigratorPrototype 
3.3. Remove the interface, along with the implementations of getMigrateData, migrate and sourceExists.

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Ignore the previous comments. Let's keep this focused to getting rid of the COM usage for migrators. I won't even remove the interface in this bug.

This works nicely :)

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Asaf Romano (gone)]

I talked with Gavin about this patch. He gave his approval for doing this change.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 639179 [details] [diff] [review]
patch

Matt will review this one.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 639179 [details] [diff] [review]
patch

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

Remove the aliases for Components.* and redundant Cu.import's from IEProfileMigrator.js as well for consistency. Make sure to add "const Cr = Components.results;" to MigrationUtils.jsm because the IE migrator uses it.

On second thought, these changes make it so that the migrator implementations are not self-contained because they rely on symbols already in scope when they're loaded.  That seems like a step backwards for code modularization.  I also don't see how this deCOM is worthwhile as it basically removed 4 lines of XPCOM boilerplate from each migrator but then added a new hard-coded mapping of migrator keys to filenames.  If this was performance sensitive code then one could argue about XPCOM overhead but I don't think that applies here, especially since most users only run this code once. It's also risky to make these changes for little benefit when we still don't have automated tests for this code.

Gavin, what do you think?

::: browser/components/migration/src/MigrationUtils.jsm
@@ +75,5 @@
>   *
>   * To implement a migrator:
> + * 1. Create a js file for the new migrator in browser/components/migration/content,
> + *    and list in browser/components/migration/jar.mn.
> + * 2. Define a Migrator objectm extending MigratorPrototype.

typo: "objectm"

@@ +85,5 @@
>   *    override |sourceHomePageURL| getter.
> + * 6. For startup-only migrators, override |startupOnlyMigrator|.
> + * 7. Add a key for the migrator in MigrationUtils.getMigrator
> + * 8. Add the key to the the MigrationUtils.migrators iterator.
> + * 9. List the migrator in migration.xul.

It's unforunate all the places where a new migrator has to be listed and this patch is adding a new one.

@@ +465,5 @@
> +#ifdef XP_WIN
> +      ,  "ie":      "IEMigrator.js"
> +      ,  "safari":  "SafariMigrator.js"
> +#elifdef XP_MACOSX
> +      ,  "safari":  "SafariMigrator.js"

Nit: I can't say I'm a fan of the commas at the beginning of the line.  A trailing comma is fine here so we shouldn't go against the norm.

::: toolkit/content/resetProfile.js
@@ +35,3 @@
>    try {
> +    Components.utils.import("resource:///modules/MigrationUtils.jsm");
> +    return MigrationUtils.resetSupported;

This isn't as foolproof as someone who copies MigrationUtils to another application and doesn't change resetSupported to false will have this return true.

Comment 7

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

Comment on attachment 639179 [details] [diff] [review]
patch

I've not had any time to look into this deeply, sorry about that. Matt's concerns seem valid from a quick glance; it's not clear to me that getting rid of XPCOM usage on its own buys us much, but I could be convinced otherwise.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 10

[Mike Conley (:mconley) (:⚙️)]

We're trying to clean up the migrator component while we have a set of CalState LA students working on it. They've already been burned by needing to do full binary builds to add new migrators (since they're registered at build-time with components.conf). I'm going to push on this bug to try to get the migrators themselves de-COM'd.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 718280 - Part 1: Move resource type constants into MigrationUtils. r?NeilDeakin!

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 718280 - Part 2: Move registration of migrators into MigrationUtils instead of components.conf. r?NeilDeakin!

Depends on D164138

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 718280 - Part 3: Get rid of some leftover wrappedJSObject usage now that the migrators aren't acquired through XPCOM. r?NeilDeakin!

Depends on D164139

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 718280 - Part 4: Remove nsIBrowserProfileMigrator.idl and references. r?NeilDeakin!

Depends on D164140

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 718280 - Part 5: Use named options rather than an array for showMigrationWizard. r?NeilDeakin!

Depends on D164142

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 718280 - Part 6: Update MigratorBase documentation that mentions deCOM'ing. r?NeilDeakin!

Now that deCOM'ing migration is done, these comments can be removed. At the time of writing
those comments, it might have been reasonable to move those methods out at the same time as
deCOM'ing, but I don't want to blow out the scope of this patch series, so we can leave those
cleanups to some other time.

Depends on D164143

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 718280 - Part 7: Update comment block on how to create a new migrator. r?NeilDeakin!

Depends on D164144

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47e2cccd33c7
Part 1: Move resource type constants into MigrationUtils. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/f1dea7abdfb7
Part 2: Move registration of migrators into MigrationUtils instead of components.conf. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/007afe158ae5
Part 3: Get rid of some leftover wrappedJSObject usage now that the migrators aren't acquired through XPCOM. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/237be1f62d01
Part 4: Remove nsIBrowserProfileMigrator.idl and references. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/33027a0f9ce9
Part 5: Use named options rather than an array for showMigrationWizard. r=NeilDeakin,credential-management-reviewers,sgalich
https://hg.mozilla.org/integration/autoland/rev/eba58fd1ed1c
Part 6: Update MigratorBase documentation that mentions deCOM'ing. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/2d0f77d324f2
Part 7: Update comment block on how to create a new migrator. r=NeilDeakin

Comment 19

[Sandor Molnar[:smolnar]]

Backed out for causing xpc failures in browser/components/migration/tests/unit/test_Edge_db_migration.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/9db699eeb254eb425137fe2a8505012e0a84ddb9

Push with failures

Failure log

INFO -  TEST-START | browser/components/migration/tests/unit/test_Edge_db_migration.js
[task 2022-12-12T22:34:14.593Z] 22:34:14  WARNING -  TEST-UNEXPECTED-FAIL | browser/components/migration/tests/unit/test_Edge_db_migration.js | xpcshell return code: 0
[task 2022-12-12T22:34:14.594Z] 22:34:14     INFO -  TEST-INFO took 683ms
[task 2022-12-12T22:34:14.594Z] 22:34:14     INFO -  >>>>>>>
[task 2022-12-12T22:34:14.594Z] 22:34:14     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73666f48e2ef
Part 1: Move resource type constants into MigrationUtils. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/912f29a6243b
Part 2: Move registration of migrators into MigrationUtils instead of components.conf. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/14abbfd66f62
Part 3: Get rid of some leftover wrappedJSObject usage now that the migrators aren't acquired through XPCOM. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/320f76dbfb6b
Part 4: Remove nsIBrowserProfileMigrator.idl and references. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/b8cc70333590
Part 5: Use named options rather than an array for showMigrationWizard. r=NeilDeakin,credential-management-reviewers,sgalich
https://hg.mozilla.org/integration/autoland/rev/d02a9b2b3f91
Part 6: Update MigratorBase documentation that mentions deCOM'ing. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/1b7d66259fbf
Part 7: Update comment block on how to create a new migrator. r=NeilDeakin

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/73666f48e2ef
https://hg.mozilla.org/mozilla-central/rev/912f29a6243b
https://hg.mozilla.org/mozilla-central/rev/14abbfd66f62
https://hg.mozilla.org/mozilla-central/rev/320f76dbfb6b
https://hg.mozilla.org/mozilla-central/rev/b8cc70333590
https://hg.mozilla.org/mozilla-central/rev/d02a9b2b3f91
https://hg.mozilla.org/mozilla-central/rev/1b7d66259fbf

Comment 22

[Martin Giger [:freaktechnik]]

I think Part 2 of this is causing Thunderbird crashes (at least in tests, bug 1805619) due to loading MigrationUtils.sys.mjs in a toolkit file.