If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Profile Migrators should not need to call doStartup

RESOLVED WONTFIX

Status

()

Toolkit
Startup and Profile System
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

I'm not sure how and why this happened.

1. All profile migrators call doStartup before they do any migration.
2. XRE also calls DoStartup as well after migration (or after deciding not to call migration). In the case of migration, DoStartup filters the second call.

Wouldn't it be simpler to call DoStartup before calling migration?

On a side note, nsIProfileStartup is passed to nsIProfileMigrator, in order to allow migrators to get the profile folder before "ProfD" can be retrieved from the directory service. Questions:
1. After calling DoStartup, isn't ProfD set already?
2. The new Firefox migrator "ignores" the provided aStartup and uses ProfDS. Is that bad?

So, it seems we can get rid of nsIProfileStartup altogether.

Comment 1

6 years ago
I'm sure *why* it happened: some of the previous profile migrators wanted to be able to copy files (the pref file, as well as bookmarks files, IIRC) from an existing profile before "starting" the current profile. For example the phoenix/firebird->firefox migrators. I thought that the seamonkey migrator did this as well.

Doesn't the new firefox->firefox migrator also rely on this feature?
Thanks for the explanation.  Those migrators are in the graveyard for some time now, and are very likely to stay there (even phoenix, yes).

The new Firefox->Firefox migrator definitely *could* take advantage of this feature, but it doesn't. See
http://hg.mozilla.org/mozilla-central/annotate/c550ffd3ba74/browser/components/migration/src/FirefoxProfileMigrator.js#l206

While I'm pretty sure that's just a result of copying code from the chrome migrator, I don't think we'd benefit much from relying on this optimization. It's optimizing the Ts for a very rare start-up scenario, in the price of an error-prone, untested API.
As for the second issue, getting "ProfD" after doStartup works fine, meaning we can indeed remove nsIProfileStartup altogether.
Other than the Firefox Migrator (which doesn't use it), the SeaMonkey profile migrator of Thunderbird is also a legitimate use case of this feature.  However, I think they can manage the same way the Firefox Migrator does.
Created attachment 589157 [details] [diff] [review]
Proposed removal (just the toolkit part)

I'll patch browser/ and mail/ when/if we generally agree this change is desired.
Assignee: nobody → mano
Attachment #589157 - Flags: feedback?
Attachment #589157 - Flags: review?(benjamin)
Attachment #589157 - Flags: review?(benjamin)
Attachment #589157 - Flags: feedback?(benjamin)
Attachment #589157 - Flags: feedback?

Updated

6 years ago
Attachment #589157 - Flags: feedback?(benjamin) → feedback+
Blocks: 718280
(In reply to Mano from comment #2)
> The new Firefox->Firefox migrator definitely *could* take advantage of this
> feature, but it doesn't.

I've actually noticed a bug where the key3.db is not being used after migrating and I suspect it's because of a race condition between copying the file to the destination profile and starting up the component that loads it.  I'm going to fix this in bug 717070 by moving the call to DoStartup after password migration. There will need to be a way to migrate some data before DoStartup if this is the actual cause.
Sigh, I didn't realize you've already commented on this bug. Sorry!

So, as we both are the only ones concerned, I won't repeat my words :)
Do most migrators need DoStartup called before they begin migration? It's not going to be trivial to get PSM to read the new key3.db from what I can tell. 

I filed bug 721242 to track the Firefox migrator password decryption issue.
WONTFIX, for the time being.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.