User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 Build ID: 20141015030202 Steps to reproduce: 1. Install in FF 36 Nightly this extention https://addons.mozilla.org/en-US/firefox/addon/add-page-title-to-url-bar/ 2. Switch from one tab to another to see whether page title get added to the URL bar or not Actual results: 1. Page title does not get added, unlike in any previous versions of FireFox; 2. Press Control+Shift+J to see this error in the console: "Cc['@mozilla.org/preferences-service;1'].getService(...).getBranch is not a function" (for a file "overlay.js") Expected results: Page title had get added to the URL bar, like in any previous versions of FireFox. I see no changes in the methods to access nsIPrefService object, so, as of now, I do not see why "Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch)" has all of sudden stopped working. What could it be? How to fix that? Thanks in advance.

Additional commentary: 1. The extension does not return any other errors or warnings in none of versions of FireFox; 2. The complete string of code that returns the aforementioned error is this: var trimURLs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch).getBranch("browser.urlbar.").getBoolPref("trimURLs"); Always worked and still works like magic in previous versions of FF.

Update: for the extension to work under FF 36 I had to update the code to this: var Prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch); var trimURLs = (Prefs) ? true : Prefs.getBranch("browser.urlbar.").getBoolPref("trimURLs"); It at least prevents the error, though the fact that preferences could not be really accessed does not change. Thus, to reproduce the error, not the latest, but previous (1.0.12) version of the extension has to be installed.

Pushlog https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e25cd3e8219&tochange=35dcddea2c7a Treggered by : Bug 1030420 It works as expected if use Ci.nsIPrefService instead of Ci.nsIPrefBranch i.e. var trimURLs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).getBranch("browser.urlbar.").getBoolPref("trimURLs");

The only reason that this ever worked is because the pref service happened to already have been QIed to `nsIPrefService` in that compartment. `nsIPrefBranch` has no `getBranch` method, so if you want that, you need to QI to the correct interface rather than assuming it will already be present on the wrapper.

Alice and Kris, thanks for your help. Before filing this bug, I made certain that the code I use is not unusual -- and, indeed, it has many references in Mozilla without notion that it could be wrong: https://developer.mozilla.org/en-US/search?q=getService%28Ci.nsIPrefBranch%29 Could you please ask you colleagues to write a script that would replace ALL of uses of "getService(Ci.nsIPrefBranch)" with "getService(Ci.nsIPrefService)" through complete developer guide database? Because it is weird that the bug #1030420 exists for a long time already and said to be "RESOLVED FIXED" while Mozilla's own developer guide still has many example of wrong usage of preferences object. Thank you in advance.

All of the uses I see in the first few pages of those results are correct. `nsIPrefService` only needs to be used for the `getBranch` method (as well as a few less common ones). In all other cases, `nsIPrefBranch` is necessary and correct.

Sorry, Kris, my bad. The syntax turned out to be too confusing for me. Having it not strict (allowing omission of nsIPrefService in some cases) in not a good programming practice. And the fact that it is impossible to get a branch through nsIPrefBranch is not intuitive either, because examples of correct uses of this interface do not even have -- and, apparently, can not have part where you can actually select a branch, which seems weird to me. In such case, branch had to be selected somewhere earlier, but there is no such thing as the code starts right with nsIPrefBranch, there is no earlier point to select a branch, and, as it turns out, you can not select a branch after, too. I have made quick Google search I found out that there are a lot of add-ons who use this code, which is now wrong. Does Mozilla have script to process all of its thousands add-ons to automatically replace a wrong code line with correct one? (At least where authors of extension have chosen appropriate type of license where Mozilla could perform such purely technical updates without asking permission of authors.)

It's not an issue of "not strict". `nsIPrefBranch` and `nsIPrefService` do different things. `nsIPrefBranch` contains all of the methods that manipulate preferences. `nsIPrefService` contains all of the methods which access branches, and manipulate the state of the service. There is no overlap. As for intuitive, the whole service is unintuitive and should not be used directly from JavaScript. `Preferences.jsm` or the SDK's preference APIs should be used instead.

I understand your explanation, but the syntax is still baffling. 'nsIPrefBranch' contains all of the methods that manipulate preferences, except for getting actual branches even though the word is in the name. And even the initializing string makes little sense: "Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService)" -- you are required to use ";1" (as if you ever need 2, 22, or 222 as parameter), and you required to use "getService(Ci.nsIPrefService)" even though you already indicated that you need preference services before that, in quotes. This was another reason why I never noticed that you need to use "nsIPrefService" instead of "nsIPrefBranch". It looks like horrible archaic syntax (Apple has recently get rid of similar or worse lexical rubbish by changing programming language). I did not chose to use it, my extension is just a modification of code that was not mine. Thanks for your explanations, Kris. As fellow programmer (though in different field), I wish that you guys would have only the cleanest languages/syntax to work with, though it seems there is no real choice.

This breaking change in Firefox 36 also broke the FiddlerHook extension, which has worked since Firefox 4 or so.

I think we should just solve this by giving nsPrefBranch ClassInfo and making the QI unnecessary altogether.

To be clear, this is now failing because of addon scopes.

Hm, so this breaks code that does things like: let gGetBoolPref = Services.prefs.getBoolPref; let val = gGetBoolPref('foo'); This code really never should have worked, except for the compatibility hack in FixUpThisIfBroken in XPCWrappedNativeJSOps.cpp [1]. And moving to ClassInfo means that we can't use that compat hack anymore, because the methods live on the prototype and it's not clear which |this| to bind them to. We could special-case this in nsJSCID::GetService, but then we'd be paying the cost of that forever. So I'd say that we should keep that in mind as an option if we get in to trouble here, but for the time being people should just fix their extension to pass the proper IID. [1] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNativeJSOps.cpp#1184

It seems like the simplest thing would be to load code into each extension compartment to query the preference service to nsIPrefBranch and nsIPrefService. I think that the correct solution, though, is to do nothing and let these extension authors fix their code. What they're doing is plainly incorrect. It seems to have originated from a forum post rather than any of our documentation, and we have no reason to think it affects more than a small number of extensions.

We could also probably just make nsIPrefService inherit from nsIPrefBranch. A quick scan of MXR doesn't show anything that would have problems with that.

(In reply to Kris Maglione [:kmag] from comment #16) > We could also probably just make nsIPrefService inherit from nsIPrefBranch. > A quick scan of MXR doesn't show anything that would have problems with that. Wouldn't we need the opposite inheritance relationship for that to have any effect, though?

(In reply to Bobby Holley (:bholley) from comment #17) > Wouldn't we need the opposite inheritance relationship for that to have any > effect, though? I don't think so. People are QIing to nsIPrefService and trying to access methods from nsIPrefBranch.

(In reply to Kris Maglione [:kmag] from comment #18) > (In reply to Bobby Holley (:bholley) from comment #17) > I don't think so. People are QIing to nsIPrefService and trying to access > methods from nsIPrefBranch. That's not what it looks like to me: (In reply to User Dderss from comment #1) > var trimURLs = > Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch). > getBranch("browser.urlbar.").getBoolPref("trimURLs"); The reporter is QIing to nsIPrefBranch and trying to access methods from nsIPrefService. And given that multiple concrete classes implement nsIPrefBranch, we can't make it inherit nsIPrefService.

Ah, right. Sorry, it's been a while since I read the above comments. The solution, then, would be to move getBranch to nsIPrefBranch and make nsIPrefService inherit from it. I'm not sure it's worth the effort, but I have often wondered why it wasn't possible to getBranch a sub-branch from a pref branch directly.

We should probably do something here. In bug 1125570, the add-on starts with nsIPrefService and wants to use it as an nsIPrefBranch. In this bug they're starting with nsIPrefBranch and want to use it as nsIPrefService. So I don't think subclassing can help here. One think we haven't tried is to automatically give out an object that has been QIed to both already. Perhaps we could do it here? http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#667 If they ask for either interface, we could QI it to the other one for them. This is pretty hacky, but it seems to be a common problem. What do you think, Bobby? Another possibility would be to add an interposition for getService that would do the same thing. It would just operate at a different level.

(In reply to Bill McCloskey (:billm) from comment #22) > We should probably do something here. In bug 1125570, the add-on starts with > nsIPrefService and wants to use it as an nsIPrefBranch. In this bug they're > starting with nsIPrefBranch and want to use it as nsIPrefService. So I don't > think subclassing can help here. > > One think we haven't tried is to automatically give out an object that has > been QIed to both already. Perhaps we could do it here? > http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSID. > cpp#667 I suggested this in comment 14. The downside would be that we would have to pay this 2 x 16-byte comparison cost forever in a very commonly-used function. > Another possibility would be to add an interposition for getService that > would do the same thing. It would just operate at a different level. That sounds much better to me, assuming that it doesn't add any significant new overhead that wasn't already there.

Evil suggestion time: would it be very wrong to implement the things nsIPrefService does on nsIPrefBranch, and then make nsIPrefService an empty extension of nsIPrefBranch? AFAICT that solves both situations.

Bill, I think that my proposed solution from comment 15 solves both problems. Moving `getBranch` and `getDefaultBranch` to `nsIPrefBranch` and having `nsIPrefService` subclass it would work for either case. The other methods on nsIPrefService are heavyweight and dangerous enough that I don't think it's a problem to break them in cases where developers haven't adequately thought through how to use them.

(In reply to :Gijs Kruitbosch from comment #24) > Evil suggestion time: would it be very wrong to implement the things > nsIPrefService does on nsIPrefBranch, and then make nsIPrefService an empty > extension of nsIPrefBranch? AFAICT that solves both situations. Hi Gijs Thank you for the information in https://bugzilla.mozilla.org/show_bug.cgi?id=1125570#c6 it does indeed solve the issue, Based on the new information about nsIPrefBranch & nsIPrefService it would appear i was incorrectly using it :Scratches Head: I hope there could be some documentation on this as i am sure that i'm not the only one who has encountered the issue or going to encounter this. Thank for the help on this.

I like the idea Kris proposed in comment 25. Here's a try push for it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d847772436

I realize that using: var trimURLs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch).getBranch("browser.urlbar.").getBoolPref("trimURLs") is wrong, but semantically it makes sense, so no wonder someone in the past has firstly used the interface this way, and that it has worked for all those years perfectly -- just until recently, when someone changed Core code so it would be in comply with documentation. The problem is that the "correct" naming and syntax to use it is absolutely ugly and makes little sense. I support Kris' idea; thanks.

This implements Kris's idea. It's green on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d847772436 Benjamin, the basic problem here is that people are using the pref service but QIing to the wrong interface. They mostly get away with this as long as someone else has already QIed to the right interface. But some work being done for e10s inadvertently broke that by splitting add-ons into their own compartments and giving them separate xpconnect wrappers. This patch shifts around some methods to make everything a little bit easier to use. If they QI to nsPrefService, they're allowed to use all the nsIPrefBranch methods as well. And if they QI to nsIPrefBranch they're allowed to use getBranch, which is the most commonly used method on nsPrefService.

Forgot to qref.

Comment on attachment 8555507 [details] [diff] [review] prefs-refactoring v2 Changing the nsIPref* hierarchy makes me a little uncomfortable; it feels to me like this could cause other breakage, given the pervasiveness of this interface. Couldn't we solve this just as easily by adding classinfo to the pref service?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #31) > Comment on attachment 8555507 [details] [diff] [review] > prefs-refactoring v2 > > Changing the nsIPref* hierarchy makes me a little uncomfortable; it feels to > me like this could cause other breakage, given the pervasiveness of this > interface. Couldn't we solve this just as easily by adding classinfo to the > pref service? Bobby tried that in comments 11 to 14.

Any chance you can get to this soon?

Comment on attachment 8555507 [details] [diff] [review] prefs-refactoring v2 What happens if you call .getBranch on a nested branch? e.g. Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).getBranch("browser.").getBranch("startup.").getCharPref("homepage"); ? I don't think this is not a good API solution. I think we're probably better-off taking the addon-compat bustage and asking Jorge for an automated warning about this particular code pattern.

If `getBranch` is called on a non-root branch, I think it needs to return a sub-branch (the behavior in this patch is counter-intuitive). But I don't think that allowing that would be a bad thing. I've been doing it in my client-side wrappers myself, in any case, e.g., let prefs = Prefs("extensions.foo.") let thingPrefs = prefs.Branch("thing."); if (thingPrefs.get("do-stuff")) ...; The only problem I can see is code conflating a sub-branch with the preferences service itself winding up with unexpected behavior. But it seems unlikely to cause problems with existing code, since branches never previously had a `getBranch` method.

Mass-closing old Extension Compatibility bugs that relate to legacy add-ons or NPAPI plug-ins. If you think this bug is still valid, please reopen or comment. Sorry for the bug spam, and happy Friday!