Last Comment Bug 533355 - nsPrefBranch tries and fails to clean up some stale weak references
: nsPrefBranch tries and fails to clean up some stale weak references
Status: RESOLVED FIXED
[tb31needed]
:
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla1.9.3a1
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 533290 519962
  Show dependency treegraph
 
Reported: 2009-12-07 16:08 PST by neil@parkwaycc.co.uk
Modified: 2010-11-03 15:34 PDT (History)
6 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.13-fixed
.16-fixed


Attachments
Proposed patch [Checked in: Comment 4 & 16] (4.58 KB, patch)
2009-12-07 16:14 PST, neil@parkwaycc.co.uk
benjamin: review+
dveditz: approval1.9.2.13+
Details | Diff | Splinter Review
1.9.1 patch [Checked in: Comment 19] (6.69 KB, patch)
2010-02-20 10:56 PST, David :Bienvenu
dveditz: approval1.9.1.16+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2009-12-07 16:08:54 PST
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 neil@parkwaycc.co.uk 2009-12-07 16:14:18 PST
Created attachment 416477 [details] [diff] [review]
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) 2009-12-15 09:24:40 PST
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 neil@parkwaycc.co.uk 2009-12-15 13:23:20 PST
(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 neil@parkwaycc.co.uk 2009-12-15 16:16:07 PST
Pushed changeset 7eec560e1a1d to mozilla-central.
Comment 5 David :Bienvenu 2010-02-20 08:44:40 PST
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 2010-02-20 10:56:05 PST
Created attachment 427960 [details] [diff] [review]
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 2010-02-20 10:56:39 PST
gozer, can we do moz-central try server builds?
Comment 8 Daniel Veditz [:dveditz] 2010-06-26 10:25:43 PDT
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, NI for questions) 2010-06-28 08:07:53 PDT
(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 2010-06-28 08:28:15 PDT
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 2010-07-15 14:42:19 PDT
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) 2010-10-25 02:40:33 PDT
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] 2010-10-25 10:48:34 PDT
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] 2010-10-25 10:51:57 PDT
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, NI for questions) 2010-10-25 11:09:36 PDT
(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) 2010-10-27 02:06:15 PDT
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) 2010-10-27 08:10:05 PDT
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] 2010-11-03 10:32:35 PDT
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 2010-11-03 15:25:35 PDT
fixed for 1.9.1.16 - changeset:   27186:f807c68b55c4

Note You need to log in before you can comment on or make changes to this bug.