533355 - nsPrefBranch tries and fails to clean up some stale weak references

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[[e-mail]]

When nsPrefBranch discovers a stale weak reference during a pref change it tries to unregister the callback. Unfortunately it actually passes the pref being changed. This means that it fails to unregister the callback when child preferences change, e.g. for a weak observer for all preferences.

Comment 1

[[e-mail]]

Attachment: Proposed patch [Checked in: Comment 4 & 16]

Rather than make an additional allocation for the string I thought it might be better to copy the string into the tail of the existing allocation.

sizeof + strlen is correct since the sizeof already includes (at least) one byte for the null terminator.

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 416477 [details] [diff] [review]
Proposed patch
[Checked in: Comment 4 & 16]

>diff -prwu4 mozilla.d040e38c4107/modules/libpref/src/nsPrefBranch.cpp mozilla/modules/libpref/src/nsPrefBranch.cpp
>-            nsMemory::Free(pCallback);
>+        NS_Free(pCallback);

Indent nit.

Comment 3

[[e-mail]]

(In reply to comment #2)
> (From update of attachment 416477 [details] [diff] [review])
> >diff -prwu4 mozilla.d040e38c4107/modules/libpref/src/nsPrefBranch.cpp mozilla/modules/libpref/src/nsPrefBranch.cpp
> >-            nsMemory::Free(pCallback);
> >+        NS_Free(pCallback);
> 
> Indent nit.

-w diff.

Comment 4

[[e-mail]]

Pushed changeset 7eec560e1a1d to mozilla-central.

Comment 5

[David :Bienvenu]

this doesn't quite apply on 1.9.1, but I'm trying to get a version that does and generate a try server build with it to see if it fixes some TB 3.0x startup crashes.

Comment 6

[David :Bienvenu]

Attachment: 1.9.1 patch [Checked in: Comment 19]

I hand-resolved the conflicts. I tried to generate a try server build but I'm not sure we can do moz-central try server builds.

Comment 7

[David :Bienvenu]

gozer, can we do moz-central try server builds?

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 416477 [details] [diff] [review]
Proposed patch
[Checked in: Comment 4 & 16]

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to comment #8)
> If you believe
> this patch is still necessary on the 1.9.2 branch please re-request approval
> along with a risk/benefit analysis explaining why we need it.

bienvenu, is a 1.9.2 patch reasonable from your POV?

I haven't set fully assessed benefit+risk, but risk: no regressions have been marked against this bug :)

Comment 10

[David :Bienvenu]

Yes,a 1.9.2 patch would seem reasonable - I don't know if it would look more like the trunk patch or the 1.9.1 patch.

Comment 11

[David :Bienvenu]

Comment on attachment 416477 [details] [diff] [review]
Proposed patch
[Checked in: Comment 4 & 16]

this patch applies to 1.9.2 - I'm trying to run with it now.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 416477 [details] [diff] [review]
Proposed patch
[Checked in: Comment 4 & 16]

I've tested this on Thunderbird's MozMill tests on the 1.9.2 branch via our try server and it seems to run fine. I've also run with it in local tests and not experienced any problems either.

Therefore requesting approval for 1.9.2 branch. It is rumoured that this may fix a crasher in Thunderbird - bug 519962.

Comment 13

[Daniel Veditz [:dveditz]]

If this fixes bug 519962 would it be worth bringing back to 1.9.1 for the last 3.0.x update?

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 416477 [details] [diff] [review]
Proposed patch
[Checked in: Comment 4 & 16]

Approved for 1.9.2.12, a=dveditz for release-drivers

Comment 15

[Wayne Mery (:wsmwk)]

(In reply to comment #13)
> If this fixes bug 519962 would it be worth bringing back to 1.9.1 for the last
> 3.0.x update?

Strictly from a thunderbird POV (I haven't looked at firefox) if this had the potential to recapture anyone we lost through startup failure I'd say we want it. So not arguing against exactly, but ...
a) I'd think those people have already installed and attempted v3.1
b) the crash sigs of bug 519962 aren't in the top 300 of v3.0.8 crashes
c) it's a big "if" that it will help 3.0, because no one has proved or said it prevents the crash. If that's true, we've only got crash-stats data - which can't demonstrate the problem is gone until v3.1.6 ships and we have its stats.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 416477 [details] [diff] [review]
Proposed patch
[Checked in: Comment 4 & 16]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/09831b863075

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 427960 [details] [diff] [review]
1.9.1 patch
[Checked in: Comment 19]

m-1.9.2 is still green,
let's fix m-1.9.1 too, at least to be safe.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 427960 [details] [diff] [review]
1.9.1 patch
[Checked in: Comment 19]

Approved for 1.9.1.16, a=dveditz for release-drivers

Comment 19

[David :Bienvenu]

fixed for 1.9.1.16 - changeset:   27186:f807c68b55c4