getPref() isn't working in custom.cfg file after upgrading to TB11 (MCD/autoconf)

RESOLVED FIXED in Firefox 12

Status

()

Toolkit
Preferences
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Barry Ralphs, Assigned: emk)

Tracking

({regression})

11 Branch
mozilla14
regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox12+ fixed, firefox13+ fixed, firefox14+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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"
What did you upgrade from ?
(Reporter)

Comment 2

5 years ago
I usually upgrade when each new update comes out. So the previous version was 10.0.2.
(Reporter)

Comment 3

5 years ago
So I see 11.0.1 has come out and this issue still hasn't been addressed...?

Comment 4

5 years ago
Can you say where are you running that code? In which file?
(Reporter)

Comment 5

5 years ago
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

5 years ago
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.
Component: Preferences → Preferences
Product: Thunderbird → Toolkit
QA Contact: preferences → preferences
Version: 11 → 11 Branch

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Probably a regression of bug 451161. AutoConfig can not read per-user settings anymore.
Blocks: 451161
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.
Summary: getPref() isn't working in custom.cfg file after upgrading to TB11 → getPref() isn't working in custom.cfg file after upgrading to TB11 (MCD/autoconf)
aceman: if you're curious how to test this:

http://mike.kaply.com/2012/03/16/customizing-firefox-autoconfig-files/
http://mike.kaply.com/2012/03/20/customizing-firefox-autoconfig-files-continued/
http://mike.kaply.com/2012/03/22/customizing-firefox-advanced-autoconfig-files/
(Reporter)

Comment 11

5 years ago
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.
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.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: regression
For anyone looking, the relevant code is here:

http://mxr.mozilla.org/mozilla-central/source/extensions/pref/autoconfig/src/prefcalls.js#129
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.
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.
Assignee: nobody → VYV03354
tracking-firefox12: --- → +
tracking-firefox13: --- → +
tracking-firefox14: --- → +
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 611098 [details] [diff] [review]
backout bug 451161 reading order change

That said, larger patch will not be suitable for branches.
Attachment #611098 - Flags: review?(roc)
Attachment #611098 - Flags: approval-mozilla-beta?
Attachment #611098 - Flags: approval-mozilla-aurora?
Attachment #611098 - Flags: review?(roc) → review+
(Assignee)

Comment 18

5 years ago
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
Attachment #611149 - Flags: review?(roc)
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
(Assignee)

Comment 20

5 years ago
Created attachment 611324 [details] [diff] [review]
Don't ignore user set values from pref files even if values are identical to default, v2
Attachment #611149 - Attachment is obsolete: true
Attachment #611149 - Flags: review?(roc)
Attachment #611324 - Flags: review?(roc)
Attachment #611324 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Keywords: regression
Whiteboard: regression → [autoland-try:-b do -p all -u all -t none]
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t none]
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:611324:-b do -p all -u all -t none]

Updated

5 years ago
Whiteboard: [autoland-try:611324:-b do -p all -u all -t none] → [autoland-in-queue]

Comment 21

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [land 611324 to trunk]
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.
Attachment #611098 - Flags: approval-mozilla-beta?
Attachment #611098 - Flags: approval-mozilla-beta+
Attachment #611098 - Flags: approval-mozilla-aurora?
Attachment #611098 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
Whiteboard: [land 611324 to trunk] → [land 611324 to trunk][land 611098 to aurora and beta]
Attachment 611324 [details] [diff] checked in to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d754f0fb17e6
Flags: in-testsuite?
Whiteboard: [land 611324 to trunk][land 611098 to aurora and beta] → [land 611098 to aurora and beta]
Target Milestone: --- → mozilla14
Attachment #611324 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/d754f0fb17e6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-beta/rev/57071196be2e
https://hg.mozilla.org/releases/mozilla-aurora/rev/78a6759335b5
status-firefox12: --- → fixed
status-firefox13: --- → fixed
Keywords: checkin-needed
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [land 611098 to aurora and beta]
Whiteboard: [qa-]

Updated

5 years ago
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.