730369 - cuddlefish/prefs.py's DEFAULT_THUNDERBIRD_PREFS are a bad idea

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Andrew Sutherland [:asuth] (he/him)]

The cuddlefish prefs for Thunderbird in python-lib/cuddlefish/prefs.py were taken from Thunderbird's mozmill automated test infrastructure.

This has the following upside:

- When you start Thunderbird with a new profile, Thunderbird won't prompt you to create a new account because it will think it has an existing account.

This has the following downsides:

- That new profile won't have any messages and the settings won't help it get any messages.  The thunderbird mozmill tests don't care about this because they synthesize fake messages using helpers not available to jetpack.

- The profile preferences are also applied to existing profiles (when you use "cfx -p"), destructively clobbering an account's settings so all the previously existing accounts look like they are entirely gone.  While the data-loss is limited, it's unpleasant and most people will have no idea how to get their accounts back.

I propose that all of the Thunderbird prefs be removed.  It would probably be advisable to also modify prefs.js to have a layered concept of "preferences for new profiles only" and "preferences for all profiles", and have "cfx -p" not use the new profile preferences when pointed at an existing profile.  For example, DEFAULT_COMMON_PREFS will dummy out extensions.update.url with a gibberish localhost entry, which is not something that anyone would want in a real profile.

Comment 1

[Dave Townsend [:mossop]]

Urk, clobbering people's profile settings? Does that happen on Firefox too? This seems pretty important to fix.

Comment 2

[Brian Warner [:warner :bwarner]]

This only happens under "cfx run", right? If I understand correctly, generated XPIs don't get any embedded prefs. Ah, but I see how using "cfx run -p MYPROFILE" could mess things up. I'm happy to remove these prefs, if those in the know tell me it's a good idea (I don't fully understand the consequences myself, but I'm happy to produce a patch).

Comment 3

[Brian Warner [:warner :bwarner]]

Hm, yeah, there are other settings in DEFAULT_COMMON_PREFS that make sense for ephemeral testing profiles, but which you wouldn't want applied to your real profile. I guess the deeper issue is that perhaps we've been treating -p as "use-but-clobber your existing profile", and that's not good enough to enable a "cfx run in my real (valuable) profile" form of testing.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Right, all of the docs I have seen seem to imply that it's a safe way to use an existing profile.  The docs could be amended, but it seems far better to avoid clobbering preferences.

The set of developer preferences (turn on "dump", etc.) seem acceptable-ish to clobber into existence, but permanently nuking extension updates or account settings seems bad.

I'll count myself as "in the know" for Thunderbird here as a lapsed Thunderbird dev to say those are bad bad preferences, but I also copied active Thunderbird dev Standard8 to this bug.  In terms of the goal of those prefs, for Thunderbird we (Thunderbird people) can provide an extension that makes it easy to create accounts and fill them with synthetic messages.


Part of the problem is likely that using "-p" is the easiest way to get your extension running in the app because the non-jetpack tricks of "symlink your development directory into your extensions/ dir" and "put a file with the id of your extension in your extensions/ dir that contains the path of your development directory" don't really work with jetpack.  I've been getting by for Thunderbird development by symlinking the .xpi to the jetpack dir and then running "cfx xpi" and then restarting thunderbird (or toggling it in the add-on manager maybe?)

Comment 5

[Brian Warner [:warner :bwarner]]

I spoke with Mossop briefly about this, and it seems like we could go in one
of two directions:

1: "don't do that": either remove -p, or put a big "this will clobber your
   profile" warning on it. I think this would slow down your development
   cycle by requiring one of (symlink .xpi and restart) or (install .xpi and
   restart). Or, if you can get your addon working restartlessly (not always
   easy, especially if you're writing low-level code), then you can add
   (install .xpi and don't restart), which would be a bit faster.

2: don't change any prefs when using -p (only modify prefs on ephemeral
   profiles). I think there are some prefs that we really do need to modify
   for 'cfx test' and ephemeral profiles, e.g. to keep the account wizard
   from opening at startup, so I think we'd need to forbid "-p" on 'cfx
   test', and would need some code to remember whether -p was provided or not
   and bypass the prefs stuff in that case.

I'm vaguely inclined to do #1: just get rid of -p. What would that do to your
dev cycle? Are there other ways to get the extension running that might work
better but still not use -p?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Getting rid of -p is fine.

I think the add-on builder has the idealized experience that would be good to duplicate for local development: hit one button and have the updated extension running in Firefox/Thunderbird.  An extension that can uninstall the extension, run a command to update the .xpi, and then install that XPI would be perfect.  Or one where cfx xpi can ping the extension that lives in Firefox/Thunderbird.

Comment 7

[Brian Warner [:warner :bwarner]]

Attachment: remove --profiledir option

Here's a patch to remove -p, under the theory that it's easier to remove it than to warn people away from using it to clobber their normal (non-development) profiles.

I'm looking for more feedback on this, though. After removing the documentation that referenced --profiledir, I saw that simple-storage is harder to test against (ephemeral profiles make it hard to re-launch the addon against old+stored data to confirm that it got loaded properly). Is that too much to lose?

If so, and we want to retain -p.. I'm not sure how to warn the user against clobbering their valuable normal-use profile. To test simple-storage, we have to accept setting -p to an existing profile. Maybe we could write some other marker file into the profiledir ("JETPACK WAS HERE"), and refuse to use a pre-existing profile that doesn't have this marker? And add the marker when we create the profile for the first time?

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

I don't know enough about your long-term testing plans, but it might be worth considering losing -p and kicking the more complicated multiple-firefox-run test cases up to a mozmill-based harness.  Mozmill knows how to do restart tests and arguably has the advantage of being decoupled from jetpack so it could also do things like uninstall the test extension and then reinstall in the same session, etc.  This also seems like it would be aligned with the change in direction to help fancy pants extension authors because it could make it easier for them to write mozmill tests of their own extensions, let AMO potentially automatically run them, etc.

Comment 9

[Brian Warner [:warner :bwarner]]

I asked folks at the Jetpack meeting about removing -p and almost universally asked to leave it in: apparently it's being used in a lot of test scenarios, especially when trying to figure out why certain broken profiles cause problems with their addon. They asked if instead, -p could just not modify preferences when pointed at an existing profile.

So I'm going to try to work up a patch for that instead: if the -p directory exists, use it without changing any of the prefs. If the -p directory doesn't exist, create the profile and populate it with the existing default prefs.

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 609556 [details] [diff] [review]
remove --profiledir option

cancelling review, I'll build a different patch which leaves prefs alone for pre-existing profiles, as per the discussion in yesterday's meeting

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So, bug 758092 sets a preference to disable application updates. That's one pref we probably don't want to clobber existing profiles with if we can avoid it. Any word on this different patch, Brian?

Comment 12

[Alexandre Poirot [:ochameau]]

Wes, I think Brian is busy writing c++ crypto things now :)

Here is what we would do, to continue his work:
https://github.com/ochameau/addon-sdk/compare/bug/730369-add-prefs-only-on-new-profiles

It sticks to comment 10 description. But we may want to tune this in order to:
- force some minimal preferences like dump one in any case?
- force all preferences in case of `cfx test`

In any case, if you thing we should land this patch as-is, we could do that too.
Any feedback/contribution is welcomed :)

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I think we should bring this up at next week's meeting.

Might be worth seeing how many tests fail if we do cfx testall from our normal profiles if none of the preferences get overridden by the Jetpack defaults.

Comment 14

[Alexandre Poirot [:ochameau]]

Ahah it just fails during first example test as it doesn't receive any output!
So this two line patch won't be enough.

Here is some updated possibilities:
- force some minimal preferences like dump one in any case?
- force some minimal preferences like dump one just for tests?
- force all preferences in case of `cfx test`
- do not accept -p/--profile for tests, always use an empty directory
- ?

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

Sorry we won't be releasing any new versions of cfx, jpm is the replacement https://www.npmjs.com/package/jpm

JPM doesn't set any prefs.