578392 - leaking browser.zoom pref observer

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Robert Sayre]

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

Comment 1

[Robert Sayre]

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.

Comment 2

[Robert Sayre]

Attachment: fix

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.

Comment 3

[Robert Sayre]

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

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 5

[Robert Sayre]

(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.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v1

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Why are the eval() calls needed?

Comment 10

[Robert Sayre]

(In reply to comment #9)
> Why are the eval() calls needed?

they aren't. I think that is blake's ghetto breakpoint setting technique.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 12

[Benjamin Smedberg]

Comment on attachment 457212 [details] [diff] [review]
Proposed fix v1

Possible to write a test for this?

Comment 13

[Robert Sayre]

(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.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: non-working test

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.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/f3b709959472

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 17

[[e-mail]]

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.

Comment 18

[[e-mail]]

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

Comment 19

[Robert Sayre]

thanks for taking a look, though