Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#3944 closed defect (fixed)

TBB prefs are set as user_prefs (may cause exceptions/settings errors?)

Reported by: mikeperry Owned by: erinn
Priority: High Milestone: TorBrowserBundle 2.3.x-stable
Component: Applications/Tor bundles/installation Version:
Severity: Keywords: tbb-bounty, tbb-usability
Cc: g.koppen@…, mcs, brade, mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

All of the prefs we change in TBB appear to be set as user prefs. This makes it hard for me in Torbutton to tell if the user has changed a value.

We also have occasional reports from people that some prefs seem inexplicably set (such as form fill options). Since user_prefs() sets a user value, it probably causes Firefox to emit observer events for each preference we change in this way. If one of them has an exception because the observer for that pref is buggy and/or tries to touch some not-yet-initialized piece of Firefox at startup, that could easily cause some prefs to fail to apply randomly.

Child Tickets

Attachments (2)

torbutton-patch.txt (2.3 KB) - added by mcs 6 years ago.
Part 1 of proposed fix: In Torbutton, use getComplexValue() to retrieve homepage pref.
torbrowser-patch.txt (43.6 KB) - added by mcs 6 years ago.
Part 2 (rev. 2) of proposed fix: Move all default prefs out of prefs.js

Download all attachments as: .zip

Change History (24)

comment:1 Changed 8 years ago by erinn

Keywords: tbb-2.2.32-4 added

comment:2 Changed 8 years ago by erinn

Keywords: tbb-2.2.32-4 removed

comment:3 Changed 8 years ago by mikeperry

This bug also makes it hard for users to overwrite their old TBBs with new versions while still keeping their preferences, and they end up doing crazy, dangerous stuff as a result: https://lists.torproject.org/pipermail/tor-talk/2011-October/021771.html

comment:4 Changed 7 years ago by mikeperry

We probably also need this to make our lives easier for Thandy, for the same reason we needed split user pref config files in Tor and vidalia.

comment:5 Changed 7 years ago by gk

Cc: g.koppen@… added

comment:6 Changed 7 years ago by mikeperry

Milestone: TorBrowserBundle 2.2.x-stableTorBrowserBundle 2.3.x-stable
Priority: normalmajor

We need this for #1878.

comment:7 Changed 7 years ago by mikeperry

Note: In more than a couple places in torbutton (as well as the Firefox-close patch), I do things like:

if (this.prefs.prefHasUserValue("torbrowser.version"))

These checks will all need to be updated when we do this switch, because the switch will cause our pref settings to stop being "user values". If anyone decides to work on this bug, please don't forget to ping me when you start, so I can update the Torbutton checks before we deploy this.

comment:8 Changed 6 years ago by mcs

Cc: mcs brade added

comment:9 Changed 6 years ago by mikeperry

Keywords: tbb-bounty added

mcs: Good find. When Firefox 17 is released, you guys can do this if we can get our own update server working in #4234.

comment:10 Changed 6 years ago by mikeperry

Keywords: tbb-usability added

Changed 6 years ago by mcs

Attachment: torbutton-patch.txt added

Part 1 of proposed fix: In Torbutton, use getComplexValue() to retrieve homepage pref.

comment:11 Changed 6 years ago by mcs

Erinn and Mike -- please review the two patches. Some details:

  • Our browser.startup.homepage value is now handled like the Firefox default value for that pref. It must come from a properties file so that code in Firefox that calls getComplexValue() will work. And we need to change Torbutton to always use getComplexValue() too.
  • The other prefs. are now split between a #tor.js file which is placed in the browser application area (named so pref values in it override other Firefox default preferences) and a preferences/extension-overrides.js file which is placed in the user profile area (necessary in order to override default preferences contained within extensions such as NoScript).
  • In windows-alpha.mk, we made a couple of somewhat unrelated changes: add /c prefix to MING path and remove some PYMAKE leftovers). Someone who better understands the implications should make sure those changes are desirable.

Also, Kathy and I removed the following preferences which we do not think need to be set:

pref("app.update.lastUpdateTime.addon-background-update-timer", 1216566538);
pref("app.update.lastUpdateTime.background-update-timer", 1216566535);
pref("app.update.lastUpdateTime.blocklist-background-update-timer", 1216566538);
pref("app.update.lastUpdateTime.microsummary-generator-update-timer", 1232574822);
pref("app.update.lastUpdateTime.search-engine-update-timer", 1216566539);
pref("urlclassifier.keyupdatetime.https://sb-ssl.google.com/safebrowsing/newkey", 1235166825);
pref("urlclassifier.tableversion.goog-black-enchash", "1.55536");
pref("urlclassifier.tableversion.goog-black-url", "1.23256");
pref("urlclassifier.tableversion.goog-white-domain", "1.481");
pref("urlclassifier.tableversion.goog-white-url", "1.371");

comment:12 Changed 6 years ago by mikeperry

I have a few questions/comments:

  1. Shouldn't we place the Torbutton preferences in extension-overrides.js too? Is there a reason why we didn't? We can probably actually just eliminate most of these entirely, though. I can work on that bit in Torbutton.
  1. If users change their addon preferences (NoScript for example), will those values take precedence over extension-overrides.js? Ie will prefs.js extension prefs override extension-overrides.js? I think we want this to be the case.
  1. Can we entirely remove the prefs.js from the distribution? A key goal here is not overwriting user preferences during updates.
  1. There may be some more prefHasUserValue checks in Torbutton. I'll get to those soon.

comment:13 in reply to:  12 Changed 6 years ago by mcs

Replying to mikeperry:

  1. Shouldn't we place the Torbutton preferences in extension-overrides.js too? Is there a reason why we didn't? We can probably actually just eliminate most of these entirely, though. I can work on that bit in Torbutton.

Yes, those should be placed in extension-overrides.js too (if they cannot be defaulted appropriately inside Torbutton itself). Good catch.

  1. If users change their addon preferences (NoScript for example), will those values take precedence over extension-overrides.js? Ie will prefs.js extension prefs override extension-overrides.js? I think we want this to be the case.

Yes, the <user-profile>/prefs.js is where all user prefs are written and they always take precedence.

  1. Can we entirely remove the prefs.js from the distribution? A key goal here is not overwriting user preferences during updates.

Kathy and I were thinking that when we implement an automatic update mechanism that that file would just be excluded (i.e., we would never overwrite it). But it is safer to just omit the prefs.js file entirely so there is no risk that the new (empty) file would overwrite a user's preferences.

  1. There may be some more prefHasUserValue checks in Torbutton. I'll get to those soon.

Thanks. I think they are all gone now, but please double-check.

comment:14 Changed 6 years ago by mikeperry

For prefs.js, we want to omit it entirely from the bundle archive so that alternate update mechanisms (such as Thandy, #5236, or even simply untarring a new tarball over an old TBB dir) don't have to deal with it.

Can you do this and move the torbutton prefs for now, and then I'll clean up and remove redundant ones when I merge?

Also, out of curiosity, do prefs set in extensions-overrides.js show up as UserValue, or as defaults?

comment:15 in reply to:  14 Changed 6 years ago by mcs

Replying to mikeperry:

Can you do this and move the torbutton prefs for now, and then I'll clean up and remove redundant ones when I merge?

Done. I will post an updated patch in a minute. We removed these two because the same pref. values are included in Torbutton:

pref("extensions.torbutton.locked_mode", true);
pref("extensions.torbutton.restore_tor", true);

Also, out of curiosity, do prefs set in extensions-overrides.js show up as UserValue, or as defaults?

They show up as defaults.

Changed 6 years ago by mcs

Attachment: torbrowser-patch.txt added

Part 2 (rev. 2) of proposed fix: Move all default prefs out of prefs.js

comment:16 Changed 6 years ago by mikeperry

I've been testing this and cleaning up our prefs a bit. It seems to be good so far, but one thing I've noticed with the Torbutton patch is that you didn't change the torbutton's update of the browser.startup.homepage pref to use setComplexValue. I went ahead and added that in. It seems to work, but probably needs more testing: https://gitweb.torproject.org/mikeperry/torbutton.git/commitdiff/aeb7ba1d950104d2e6565ea6611678f441de53e7

Let me know if there's some reason we shouldn't also use setComplexValue there.

comment:17 Changed 6 years ago by brade

Since you are setting it to a string, it's OK to use either set call. Internally, SetComplexValue will set it as a string preference since it is a string. See:

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#387

comment:18 Changed 6 years ago by brade

Cc: mikeperry added

The patch here caused a regression. People on #tor are reporting that "update needed" is shown by the browser after installing the latest package. The problem is caused by a mismatch between the torbrowser.version preference value and the version reported by the web site.

On Mac OS and Linux, the version used to include -$(ARCH_TYPE). Mark and I accidentally omitted this when we made the changes for this bug.

In the short run, someone may want to change the RecommendedTBBVersions list (remove last component of MacOS and Linux versions).

In the long run, is it desirable to include the architecture?

comment:19 Changed 6 years ago by mikeperry

Resolution: fixed
Status: newclosed

Ok, we've fixed the versions file. I filed #8310 for the pref change. We likely do need arch. In the past we've had to update the arch builds separately sometimes..

comment:20 Changed 6 years ago by arma

Fyi, my tor-browser-gnu-linux-x86_64-2.3.25-4-dev-en-US.tar.gz when it starts sends me to the https://check.torproject.org/?lang=en-US&small=1&uptodate=0 page -- i.e. they "there is a security update" page. This is with RecommendedVersions saying

[
"2.3.25-4-MacOS",
"2.3.25-4-Windows",
"2.3.25-4-Linux",
"2.4.10-alpha-2-MacOS",
"2.4.10-alpha-2-Windows",
"2.4.10-alpha-2-Linux"
]

The Torbutton icon no longer blinks. So I assume something new is broken here -- perhaps there are still two update mechanisms in place, and by fixing one you broke the other?

comment:21 Changed 6 years ago by mikeperry

Did you first start it before we fixed the versions file? The default value for the homepage pref is "https://check.torproject.org/?lang=en-US&small=1&uptodate=1".. The only way it should be able to get to "uptodate=0" is if a earlier version check failed.

Remember that we don't block on the version check anymore before launching the browser or loading the homepage, so whatever versioncheck result you got previously is what homepage value you'll have (until another version check can be performed to update your homepage value).

comment:22 in reply to:  21 Changed 6 years ago by arma

Replying to mikeperry:

Did you first start it before we fixed the versions file? The default value for the homepage pref is "https://check.torproject.org/?lang=en-US&small=1&uptodate=1".. The only way it should be able to get to "uptodate=0" is if a earlier version check failed.

Remember that we don't block on the version check anymore before launching the browser or loading the homepage, so whatever versioncheck result you got previously is what homepage value you'll have (until another version check can be performed to update your homepage value).

Even after a complete restart? I turned it all off and started it again.

I hope it doesn't cache stuff across runs.

That said, I left the old one up for a few hours, then just restarted it, and now the check page is the right one.

*Does* it cache stuff across runs?

Note: See TracTickets for help on using tickets.