nsPrefBranch tries and fails to clean up some stale weak references

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Preferences: Backend
--
minor
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(status1.9.2 .13-fixed, status1.9.1 .16-fixed)

Details

(Whiteboard: [tb31needed])

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #416477 - Flags: review?(benjamin)
Attachment #416477 - Flags: review?(benjamin) → review+
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.
Blocks: 519962, 533290
(Assignee)

Comment 3

8 years ago
(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.
Blocks: 488587
(Assignee)

Comment 4

8 years ago
Pushed changeset 7eec560e1a1d to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Whiteboard: [I guess this is wanted on m-1.9.2/m-1.9.1 too...]
Target Milestone: --- → mozilla1.9.3a1
No longer blocks: 488587
Attachment #416477 - Flags: approval1.9.2.1?

Comment 5

7 years ago
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

7 years ago
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

7 years ago
gozer, can we do moz-central try server builds?
Attachment #416477 - Flags: approval1.9.2.2? → approval1.9.2.3?
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.
Attachment #416477 - Flags: approval1.9.2.4?
(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

7 years ago
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

7 years ago
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.
Attachment #416477 - Flags: approval1.9.2.7?
Attachment #416477 - Flags: approval1.9.2.7?
Whiteboard: [I guess this is wanted on m-1.9.2/m-1.9.1 too...] → [tb31needs]
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.
Attachment #416477 - Flags: approval1.9.2.12?
Attachment #416477 - Attachment description: Proposed patch → Proposed patch [Checked in: Comment 4]
If this fixes bug 519962 would it be worth bringing back to 1.9.1 for the last 3.0.x update?
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
Attachment #416477 - Flags: approval1.9.2.12? → approval1.9.2.12+
(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 on attachment 416477 [details] [diff] [review]
Proposed patch
[Checked in: Comment 4 & 16]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/09831b863075
Attachment #416477 - Attachment description: Proposed patch [Checked in: Comment 4] → Proposed patch [Checked in: Comment 4 & 16]
status1.9.2: --- → .13-fixed
Whiteboard: [tb31needs]
Whiteboard: [tb31needed]
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.
Attachment #427960 - Flags: approval1.9.1.16?
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
Attachment #427960 - Flags: approval1.9.1.16? → approval1.9.1.16+

Comment 19

7 years ago
fixed for 1.9.1.16 - changeset:   27186:f807c68b55c4
status1.9.1: --- → .16-fixed
Attachment #427960 - Attachment description: 1.9.1 patch → 1.9.1 patch [Checked in: Comment 19]
You need to log in before you can comment on or make changes to this bug.