494186 - Stockticker breaks Firefox since upvar2 patch has landed

Status: RESOLVED DUPLICATE of bug 494946

(Core :: Preferences: Backend, defect)

Description

[TheRave]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre

When Stockticker is installed you can't open or close tabs, addonmanager has the scrollbar on left side, scroll with mouse in addon manager doesnt work, options window is empty...

Reproducible: Always

Steps to Reproduce:
1. Create a clean profile.
2. Install Stockticker 1.04 or 1.05 and restart
3. go to js console - no error
4. go to addon manager and then you get the following messages (possible you need 2 or 3 trys before fx break)

Error: formatURLPref: Couldn't get pref: extensions.getMoreExtensionsURL
Source File: file:///C:/Programme/Mozilla_Firefox31/components/nsURLFormatter.js
Line: 68

Error: uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch2.getBoolPref]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: updateGlobalCommands :: line 2187"  data: no]

6. Addonmanager scrollbar on left side, no mouse scrolling

7. go to options window - its empty, messages:

Error: uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch.getBoolPref]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://global/content/bindings/preferences.xml ::  :: line 576"  data: no]

8. Impossible to close/open tabs.

9. several extensions doesnt work



When Stockticker is installed and Fx was compiled with changeset 24809:b2d7dbeb0f1b its ok, but compiled with changeset 24812:79606200f871 the extension breaks the entire firefox.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Just for clarity, b2d7dbeb0f1b is a changeset only present on the 1.9.1 branch.  The pushlog between those two changesets is:

http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=b2d7dbeb0f1b&tochange=79606200f871

I'd be interested in the equivalent range from m-c, or better yet tracemonkey...  All that range tells me on branch is that we had a TM merge.

I'd also be interested in an actual link to the extension in question, just to make sure that the right thing is being tested.

Comment 2

[Marien Zwart]

Tested the addon StockTicker 1.04 from amo (https://addons.mozilla.org/en-US/firefox/downloads/latest/183/addon-183-latest.xpi) on i686 linux:

Testing mozilla-central:

good: Gecko/20090406 (http://hg.mozilla.org/mozilla-central/rev/b14428284d51)
bad:  Gecko/20090407 (http://hg.mozilla.org/mozilla-central/rev/f0507c4d0abb)

Testing tracemonkey:

good: http://hg.mozilla.org/tracemonkey/rev/7d681f116714
bad:  http://hg.mozilla.org/tracemonkey/rev/0f7ab38ff862

which does point at upvar2.

To test I installed the addon in a clean profile (reused with all builds, ignoring some places breakage, and with extensions.checkCompatibility off), then opened the error console and addon manager (in any order), then repeatedly closed and opened the addon manager (extensions tab). Opening less than half a dozen times triggers this on the error console on closing the addon manager:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPrefBranch2.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: Shutdown :: line 1140"  data: no]

The next open triggers:

Error: formatURLPref: Couldn't get pref: extensions.getMoreExtensionsURL
Source File: file:///home/marienz/tmp/firefox-2009-04-07-03-tracemonkey/components/nsURLFormatter.js
Line: 68

Error: uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch2.getBoolPref]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: updateGlobalCommands :: line 2176"  data: no]

and after that the main firefox preferences window is blank. I did not check the other breakage mentioned in comment #0, just watched the error console and confirmed the pref window is blank.

Comment 3

[Boris Zbarsky [:bzbarsky]]

ok, mzz can reproduce this on Linux, with all three of 1.9.1, m-c, t-m.

The ranges he found were:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b14428284d51&tochange=f0507c4d0abb
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=7d681f116714&tochange=0f7ab38ff862

So looks like this is an upvar2 regression.

Comment 4

[TheRave]

Yes, its the 1.9.1 Branch. The headline of changeset 79606200f871 is Brendan Eich — upvar2, aka the big one take 2 (452498, r=mrbkap).

http://mozmonkey.com/packages/stockticker/stockticker.xpi

Comment 5

[Brendan Eich [:brendan]]

Can a testcase (however reduced) be extracted from the add-on?

/be

Comment 6

[Michael Ryan]

Attachment: reduced testcase

I can reproduce with this testcase in the latest 1.9.1 nightly and 3.0.10. Relevant line of code:

var prefBranch = Components.classes["@mozilla.org/preferences-service;1"].createInstance(Components.interfaces.nsIPrefBranch2);

Comment 7

[Brendan Eich [:brendan]]

What do I set via about:config to get the add-on to install? It doesn't like 3.6a1pre...

/be

Comment 8

[TheRave]

Confirm testcase on 1.9.1.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #7)
> What do I set via about:config to get the add-on to install? It doesn't like
> 3.6a1pre...

Brendan, you should install the nightly tester tools from dave. See https://addons.mozilla.org/en-US/firefox/addon/6543. That way you can override the compatibility. Otherwise setting extensions.checkCompatibility to false could help.

I'll have a look at it right now.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That's happening because the init function of this test extension calls createInstance instead of getService on the nsIPrefBranch2 interface. Changing it to getService everything is fine.

I'll check for check-ins on the given date.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, I missed that the regression window is already found. But the referenced bug on the dependency list was wrong. Updating it to the upvar2 bug.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Testcase (chrome privileges)

This is a more reduced testcase which needs chrome privileges to run. No need to install an extension anymore.

Steps:
1. Open and run the testcase
2. Reload the page

After step 2 you can see the same behavior and errors as described above.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Updating the summary to reflect the latest investigations. I think that's all what I can do here for now.

Comment 14

[Brendan Eich [:brendan]]

What has this bug to do with upvar2, then? Is it possible the extension happened to switch to createInstance at the same time? Or did we finally outlaw calling of createInstance on a service at the same time?

I see no upvars at all in the reduced testcase, so removing the dependency.

/be

Comment 15

[TheRave]

Stockticker was last packed at July 2008 and works since years. This bug came with bugfix 452498 and this was before 4 weeks. When you did outlaw calling of createInstance on a service, we have likely some more extensions which need an update. If say there is no problem with 452498 we have likely more extensions which breaks the fx...

Comment 16

[Brendan Eich [:brendan]]

This was an XPCOM change? Or XPConnect? Trying XPCOM first.

/be

Comment 17

[Robert Sayre]

so, at first I thought there was no way these could be upvar2 problems. But then I noticed that the error messages in the console were from sessionStore and other things trying to access the pref service. Maybe something isn't falling out of scope that should be?

Comment 18

[Brendan Eich [:brendan]]

Could be two bugs -- clone this bug if necessary.

What were those sesionStore error messages, and implicated code? Thanks,

/be

Comment 19

[TheRave]

SessionStore: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch.getIntPref]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: file:///C:/Programme/Mozilla_Firefox31/components/nsSessionStore.js :: sss_serializeHistoryEntry :: line 1181"  data: no]

Error: formatURLPref: Couldn't get pref: extensions.getAddons.recommended.browseURL
Source file: file:///C:/Programme/Mozilla_Firefox31/components/nsURLFormatter.js
Line: 68

Error: uncaught exception: unknown (can't convert to string)

Error: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch.getIntPref]
Source file: file:///C:/Programme/Mozilla_Firefox31/components/nsSessionStore.js
Line: 2541

Error: Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIPrefBranch2.setIntPref]
Source file: file:///C:/Programme/Mozilla_Firefox31/components/nsUpdateService.js
Line: 2878

Error: Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIPrefBranch.setCharPref]
Source file: file:///D:/Backup/Mozilla/Firefox/Firefox31/extensions/%7B73a6fe31-595d-460b-a920-fcc0f8843232%7D/components/noscriptService.js
Line: 1825

Comment 20

[Michael Ryan]

Attachment: Testcase (breaks latest 1.9.1, works for 3.0.10)

This testcase reproduces the issue in the latest 1.9.1 but not in 3.0.10 (as opposed to the first testcase which reproduces it for both).

Comment 21

[Michael Ryan]

Attachment: Simple testcase

This testcase reproduces the issue without having to do the add-on window stuff. It forces a garbage collection and then tries to read a pref. Fails in latest 1.9.1 and 3.0.10.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I don't that this is a regression at all. I can go back to Firefox 2.0.0.20 which is also affected and even crashes when opening the Addons Manager after the test file has been loaded.

I'll check with the mentioned Addon and the given time frame if there is a difference.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Brendan, the regression which is specific to the addon is really a separate issue. After cloning the tracemonkey branch and building own builds I can confirm the above regression range and that http://hg.mozilla.org/tracemonkey/rev/2cf0bbe3772a caused this problem.

How shall we proceed? Filing the above regression as a new bug since we morphed this one a lot? I would think so.

Comment 24

[Benjamin Smedberg]

calling Components.classes['@mozilla.org/preferences-service;1'].createInstance(Components.interfaces.nsIPrefBranch) is *always wrong* (you should be using .getService instead). It has been wrong since the beginning of time. It's not clear how upvar2 made this worse, but I don't see how this is a blocker.

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #23)
> Brendan, the regression which is specific to the addon is really a separate
> issue. After cloning the tracemonkey branch and building own builds I can
> confirm the above regression range and that
> http://hg.mozilla.org/tracemonkey/rev/2cf0bbe3772a caused this problem.

Any hope of a reduced testcase? Or at least an HTML one?

> How shall we proceed? Filing the above regression as a new bug since we morphed
> this one a lot? I would think so.

Either that, or morph this back -- shouldn't have morphed it, in hindsight.

/be

Comment 26

[Michael Ryan]

Attachment: Simple testcase (fails after upvar2)

This testcase demonstrates the "regression" caused by upvar2.

Good: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090406 Minefield/3.6a1pre
Bad: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090407 Minefield/3.6a1pre

Removing "load;" from prefObserver.observe causes this to fail for builds before upvar2.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Ok, sounds like something really is weird here, per comment 26, and that it has to do with upvar2.  Back to js.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Restoring summary.

Comment 29

[Mike Shaver (:shaver emeritus)]

I wonder if removing the reference to the enclosing function is causing us to pass a null/flat/strange/charmed closure to XPConnect.  But I know basically nothing about the upvar analysis, so maybe use as a call argument already ensures that we don't do anything that would be broken by being called back on a different context or after the enclosing frame has died or whatever....

Comment 30

[Mike Shaver (:shaver emeritus)]

I don't understand that test case: it sets an observer on "this.pref.does.not.exist" and then _gets_ a _different_ pref, and we expect what to happen?

Comment 31

[Jason Orendorff [:jorendorff]]

Here's my theory.

1.  This code creates an nsPrefBranch.  You're not supposed to do that, by the way.

2.  Then the nsPrefBranch gets GC'd, which destroys the global hash table of prefs (gHashTable in prefapi.cpp) and causes all subsequent pref accesses to fail with NS_ERROR_NOT_INITIALIZED.

So two questions: why did this work before? and can we do something to mitigate the pain?

It worked before upvar2 because the lambda in the observer kept all enclosing scopes alive, which prevented the first nsPrefBranch from getting GC'd. This was arguably a bug, as the nsPrefBranch really does become unreachable when the function exits, and it should be GC'd.  upvar2 simply fixed that bug!

sdwilsh suggested mitigating this by making the appropriate objects singletons.

Comment 32

[Mike Shaver (:shaver emeritus)]

Yeah, I agree that it's a bug in the extension, but it sucks that we're going to be changing that behaviour after the add-on compat beta promise.  Pref observers are quite common, so I wouldn't be surprised if this affects a lot of add-ons.

(I don't quite follow why destroying a single prefBranch causes all future pref accesses to fail that way, but that code is...imperfect anyway.)

Makes this a non-blocker, though, IMO.

Comment 33

[Jason Orendorff [:jorendorff]]

Un-assigning.  This seems to be a case of bug 494946.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So this bug is more a Wontfix?

I have contacted the author of the extension so it will hopefully be fixed for the final release or RC.

Comment 35

[Benjamin Smedberg]

Yes this bug is basically WONTFIX except for the generic long-term fix for bug 494946