I'll initially check for balanced calls to the add/remove stuff

So, the right calls do get made from browser-FullZoom.js, but the observer doesn't get removed from the array. The method then returns NS_OK. We probably can't change the method raise an exception without breaking a bunch of stuff. I guess I'll add an assert.

jst and mrbkap explained that the object is getting rewrapped because it's being inserted into the array as a weak reference. mrbkap says this callback probably won't even work in the event that the preference changes. I say we should make the 3-argument version noscript. mrbkap agrees.

looks like the nsIObserverService has the same kind of interface. I will look at that thing as well.

(In reply to comment #2) > mrbkap says this callback probably won't even work in the event that the > preference changes. What callback won't work? Are you saying that the observer is currently broken? > I say we should make the 3-argument version noscript. mrbkap agrees. Do you mean remove support for addObserver(..., ..., true); from script?

(In reply to comment #4) > (In reply to comment #2) > > mrbkap says this callback probably won't even work in the event that the > > preference changes. > > What callback won't work? Are you saying that the observer is currently broken? mrbkap didn't see how it would work > > I say we should make the 3-argument version noscript. mrbkap agrees. > > Do you mean remove support for addObserver(..., ..., true); from script? Yes.

I was a little wrong before: the observers will work, but removing them is not guaranteed. The underlying problem is a bug in nsPrefBranch: it stores a strong reference to an nsISupportsWeakReference and a weak reference to the nsIObserver. It then later takes a reference to another nsIObserver and assumes that it can compare the two. However, in the presence of tearoffs, this is not the case. Instead, we need to compare nsISupports pointers.

Comment on attachment 457212 [details] [diff] [review] Proposed fix v1 Oops. The test changes aren't supposed to be there. In order to avoid QIing to nsISupports each time through the loop, I grab a reference to the nsISupports pointer in ::AddObserver. It should be safe (and commented). We also noticed that freeObserverList didn't deal with us re-entering, so I fixed that too (and got rid of a mostly useless check for count > 0).

Why are the eval() calls needed?

(In reply to comment #9) > Why are the eval() calls needed? they aren't. I think that is blake's ghetto breakpoint setting technique.

Yeah, forgot to take them out before qrefreshing. Forget they were ever there.

Comment on attachment 457212 [details] [diff] [review] Proposed fix v1 Possible to write a test for this?

(In reply to comment #12) > Comment on attachment 457212 [details] [diff] [review] > Proposed fix v1 > > Possible to write a test for this? Not without undue hardship. We spent an afternoon on it. Don't want to wait on this.

This test doesn't show the bug. I think it's because we don't have an nsXPCWrappedJS for observer that isn't an nsIObserver. I gave up trying to find an interface that had all of the right properties.

http://hg.mozilla.org/mozilla-central/rev/beb64fbb5d86 <-- bustage followup.

Comment on attachment 457212 [details] [diff] [review] Proposed fix v1 >@@ -779,20 +792,34 @@ NS_IMETHODIMP nsPrefBranch::RemoveObserv > if (!mObservers) > return NS_OK; > > // need to find the index of observer, so we can remove it from the domain list too > count = mObservers->Count(); > if (count == 0) > return NS_OK; > >+ nsCOMPtr<nsISupports> canonical(do_QueryInterface(aObserver)); Unfortunately aObserver isn't guaranteed to be live at this point. I'm surprised nobody has crashed here yet.

(In reply to comment #17) > I'm surprised nobody has crashed here yet. Ah, this is what the bustage fix was all about.

thanks for taking a look, though