Last Comment Bug 738043 - getPref() isn't working in custom.cfg file after upgrading to TB11 (MCD/autoconf)
: getPref() isn't working in custom.cfg file after upgrading to TB11 (MCD/autoc...
Status: RESOLVED FIXED
[qa-]
: regression
Product: Toolkit
Classification: Components
Component: Preferences (show other bugs)
: 11 Branch
: All All
: -- critical (vote)
: mozilla14
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on:
Blocks: 451161
  Show dependency treegraph
 
Reported: 2012-03-21 14:41 PDT by Barry Ralphs
Modified: 2012-05-02 15:25 PDT (History)
13 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed


Attachments
backout bug 451161 reading order change (903 bytes, patch)
2012-03-30 17:54 PDT, Masatoshi Kimura [:emk]
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
Don't ignore user set values from pref files even if values are identical to default (5.03 KB, patch)
2012-03-31 04:19 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Don't ignore user set values from pref files even if values are identical to default, v2 (5.58 KB, patch)
2012-04-01 16:15 PDT, Masatoshi Kimura [:emk]
roc: review+
ryanvm: checkin+
Details | Diff | Review

Description Barry Ralphs 2012-03-21 14:41:02 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.79 Safari/535.11

Steps to reproduce:

I upgraded to TB11


Actual results:

getPref() stopped returning correct values. It always returns null after upgrading to TB11.


Expected results:

It should return correct values. e.g.
var listExistingAccounts = getPref("mail.accountmanager.accounts");
should return "account1,account2,account3"
Comment 1 Ludovic Hirlimann [:Usul] 2012-03-22 01:25:47 PDT
What did you upgrade from ?
Comment 2 Barry Ralphs 2012-03-22 09:47:48 PDT
I usually upgrade when each new update comes out. So the previous version was 10.0.2.
Comment 3 Barry Ralphs 2012-03-29 10:40:36 PDT
So I see 11.0.1 has come out and this issue still hasn't been addressed...?
Comment 4 :aceman 2012-03-29 10:51:17 PDT
Can you say where are you running that code? In which file?
Comment 5 Barry Ralphs 2012-03-29 10:55:35 PDT
I have a "custom.cfg" file in the install folder "C:\Program Files (x86)\Mozilla Thunderbird"

& these are set in my "...\defaults\pref\custom.js"

pref("general.config.obscure_value", 0);
pref("general.config.filename", "custom.cfg");
Comment 6 :aceman 2012-03-29 11:27:51 PDT
Usul, this could be a Core/toolkit problem with in the custom script support. Could you CC the right people? I doubt getPref is a TB specific function.
Comment 7 :aceman 2012-03-30 01:03:31 PDT
Just for the record, I have not tried replicating the bug because I would not know how to test it, how to see the error in that custom file. Hopefully someone more familiar with the functionality can see the problem straight.
Comment 8 Masatoshi Kimura [:emk] 2012-03-30 03:35:25 PDT
Probably a regression of bug 451161. AutoConfig can not read per-user settings anymore.
Comment 9 Mark Banner (:standard8) 2012-03-30 06:19:19 PDT
To clarify, this is all about Mission control desktop/autoconf. I know the lockPref stuff was working in 11 and 12, I don't use the getPref items myself.
Comment 11 Barry Ralphs 2012-03-30 09:48:44 PDT
You can easily see this by adding the following lines to your custom.cfg file:

var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
                              .getService(Components.interfaces.nsIPromptService);
var listExistingAccounts = getPref("mail.accountmanager.accounts");
promptService.alert(null, "title", listExistingAccounts);

In TB11+ you'll get a blank message box. In TB10- you'll get a list of the accounts.
Comment 12 Mike Kaply [:mkaply] 2012-03-30 10:27:55 PDT
Just verified it is broke in Firefox as well.

The getPref function works for default preferences but it no longer works for user set preferences.

This is a big regression.
Comment 13 Mike Kaply [:mkaply] 2012-03-30 10:29:04 PDT
For anyone looking, the relevant code is here:

http://mxr.mozilla.org/mozilla-central/source/extensions/pref/autoconfig/src/prefcalls.js#129
Comment 14 Mike Kaply [:mkaply] 2012-03-30 10:46:04 PDT
Here's more specific code that used to work in an autoconfig file but doesn't anymore. It works fine elsewhere.

var prefService = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService);    
var prefBranch = prefService.getBranch(null);
var prefValue = prefBranch.getCharPref("intl.charsetmenu.browser.cache");

At this point, prefValue is empty.
Comment 15 Alex Keybl [:akeybl] 2012-03-30 13:34:35 PDT
Masatoshi - Michael believes that this is a regression from bug 451161. Would you mind taking a look?

Tracking for FF12 and up since this regressed in 11 and will cause pain for deployments.
Comment 16 Masatoshi Kimura [:emk] 2012-03-30 16:40:33 PDT
The reason is obvious. Bug 451161 changed a reading order so that AutoConfig is called before user settings.
Bug 451161 comment 20 explains why the order was reversed. How can I reverse the order again without regressing bug 451161? I have no idea at all.
Comment 17 Masatoshi Kimura [:emk] 2012-03-30 17:54:15 PDT
Created attachment 611098 [details] [diff] [review]
backout bug 451161 reading order change

That said, larger patch will not be suitable for branches.
Comment 18 Masatoshi Kimura [:emk] 2012-03-31 04:19:47 PDT
Created attachment 611149 [details] [diff] [review]
Don't ignore user set values from pref files even if values are identical to default

If an user set value is identical to the default one and AutoConfig doesn't change the default, the user set value will be removed from prefs.js on exit anyway.
https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/prefapi.cpp#331
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-01 15:48:41 PDT
Comment on attachment 611149 [details] [diff] [review]
Don't ignore user set values from pref files even if values are identical to default

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

::: modules/libpref/src/Preferences.cpp
@@ +467,5 @@
>    rv = reader->Open(aFile);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsIUTF8StringEnumerator> files;
> +  rv = reader->FindEntries(nsDependentCString("defaults/preferences/""*.(J|j)(S|s)$"),

I'm not sure what this change is for

::: modules/libpref/src/prefapi.cpp
@@ +172,5 @@
>  /* -- Prototypes */
>  static nsresult pref_DoCallback(const char* changed_pref);
>  
> +enum {
> +    kPrefSetDefault = true,

I think 1 instead of true

@@ +745,5 @@
>      {
>          /* If new value is same as the default value, then un-set the user value.
>             Otherwise, set the user value only if it has changed */
>          if (!pref_ValueChanged(pref->defaultPref, value, type) &&
> +            pref->flags & PREF_HAS_DEFAULT &&

() around the & expression
Comment 20 Masatoshi Kimura [:emk] 2012-04-01 16:15:46 PDT
Created attachment 611324 [details] [diff] [review]
Don't ignore user set values from pref files even if values are identical to default, v2
Comment 21 Mozilla RelEng Bot 2012-04-01 20:53:55 PDT
Autoland Patchset:
	Patches: 611324
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=148d6b76519a
Try run started, revision 148d6b76519a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=148d6b76519a
Comment 22 Mozilla RelEng Bot 2012-04-02 01:17:17 PDT
Try run for 148d6b76519a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=148d6b76519a
Results (out of 224 total builds):
    exception: 1
    success: 188
    warnings: 34
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-148d6b76519a
Comment 23 Alex Keybl [:akeybl] 2012-04-02 10:35:12 PDT
Comment on attachment 611098 [details] [diff] [review]
backout bug 451161 reading order change

[Triage Comment]
Low-risk backout of a regression in FF11. Approved for Aurora 13 and Beta 12.
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-04-03 17:12:40 PDT
Attachment 611324 [details] [diff] checked in to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d754f0fb17e6
Comment 25 Marco Bonardo [::mak] 2012-04-04 05:06:13 PDT
https://hg.mozilla.org/mozilla-central/rev/d754f0fb17e6

Note You need to log in before you can comment on or make changes to this bug.