Closed
Bug 1265262
Opened 9 years ago
Closed 8 years ago
startup crash in mozilla::LogModulePrefWatcher::RegisterPrefWatcher
Categories
(Core :: XPCOM, defect, P3)
Tracking
()
People
(Reporter: philipp, Assigned: erahm)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
1.14 KB,
patch
|
froydnj
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is
report bp-6f8631f3-aa42-4937-99c7-c43a42160416.
=============================================================
Crashing Thread (0)
Frame Module Signature Source
0 xul.dll mozilla::LogModulePrefWatcher::RegisterPrefWatcher() xpcom/base/LogModulePrefWatcher.cpp
1 xul.dll nsComponentManagerImpl::Init() xpcom/components/nsComponentManager.cpp
2 xul.dll NS_InitXPCOM2 xpcom/build/XPCOMInit.cpp
there is a couple of those startup crashes showing up in firefox 46 beta builds (apparently affecting particular installations repeatedly).
Assignee | ||
Comment 2•9 years ago
|
||
It's claiming it crashes at [1]:
> LoadExistingPrefs();
Which doesn't make a ton of sense, but I suppose things are getting inlined. My best guess is we're getting nullptr back from |Preferences::GetRootBranch()| [2] which shouldn't happen as the pref system should already be initialized, but checking for that would makes sense.
The other possibility is that the names array somehow has a null value, but it looks like that can't happen [3].
[1] https://hg.mozilla.org/releases/mozilla-beta/annotate/076bf6a0ac85/xpcom/base/LogModulePrefWatcher.cpp#l75
[2] https://hg.mozilla.org/releases/mozilla-beta/annotate/076bf6a0ac85/xpcom/base/LogModulePrefWatcher.cpp#l56
[3] https://dxr.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08/modules/libpref/nsPrefBranch.cpp#588
Flags: needinfo?(erahm)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8742462 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment 4•9 years ago
|
||
[Tracking Requested - why for this release]:
if we do another beta build, maybe we can slip this one in.
Comment 5•9 years ago
|
||
Tempting but the crash volume (and for only a few installations - 30 crashes on 3 installs in a week) I can't justify uplifting to 46. 47 seems possible though.
The multiple crashes on a tiny number of installs to me sounds like someone testing or fuzzing. Still could be worth uplifting on the theory that fixing an issue like this could fix other crashes too.
tracking-firefox47:
--- → +
Comment 6•9 years ago
|
||
Comment on attachment 8742462 [details] [diff] [review]
Check for null root branch when registering pref watcher
Review of attachment 8742462 [details] [diff] [review]:
-----------------------------------------------------------------
I don't feel like we've figured out what's going wrong here. We put the pref watcher registration there to ensure that the preferences have already been initialized...but we're ending up in a situation where the preferences haven't been initialized? That seems suspicious, and this null-check seems like it will hide other issues later.
Attachment #8742462 -
Flags: review?(nfroyd)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8742462 [details] [diff] [review]
> Check for null root branch when registering pref watcher
>
> Review of attachment 8742462 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I don't feel like we've figured out what's going wrong here. We put the
> pref watcher registration there to ensure that the preferences have already
> been initialized...but we're ending up in a situation where the preferences
> haven't been initialized? That seems suspicious, and this null-check seems
> like it will hide other issues later.
I'm not sure what else to do here, I can't reproduce the issue. It seems to affect a max of 8 x86 Windows users on Beta.
Comment 8•8 years ago
|
||
We'd like a fix, but not tracking due to crash volume.
status-firefox48:
--- → wontfix
status-firefox49:
--- → affected
Comment 9•8 years ago
|
||
Eric, do you plan to land this?
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #9)
> Eric, do you plan to land this?
I can't land without an r+. Nathan wdyt?
Flags: needinfo?(erahm) → needinfo?(nfroyd)
Comment 11•8 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #10)
> (In reply to Jim Mathies [:jimm] from comment #9)
> > Eric, do you plan to land this?
>
> I can't land without an r+. Nathan wdyt?
Let's take it. I guess this only would affect being able to dynamically control prefs, which is something a user is unlikely to do anyway...
Flags: needinfo?(nfroyd)
Updated•8 years ago
|
Attachment #8742462 -
Flags: review+
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/548b35e65fe7e8d3a48f978ce3e8178df3b37753
Bug 1265262 - Check for null root branch when registering pref watcher. r=froydnj
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8742462 [details] [diff] [review]
Check for null root branch when registering pref watcher
Approval Request Comment
[Feature/regressing bug #]: Since the code landed in bug 1265262.
[User impact if declined]: Startup crashes on Windows (somewhat low volume).
[Describe test coverage new/current, TreeHerder]: Code is executed on every startup of Firefox.
[Risks and why]: Low risk, adds a null pointer check. If the check fails users won't be able to dynamically enable logging, but they also won't crash.
[String/UUID change made/needed]: None.
Attachment #8742462 -
Flags: approval-mozilla-beta?
Attachment #8742462 -
Flags: approval-mozilla-aurora?
Comment 14•8 years ago
|
||
Comment on attachment 8742462 [details] [diff] [review]
Check for null root branch when registering pref watcher
Fix for a startup crash, let's uplift. This should end up in beta 4 early next week. We should circle back and check for crashes to verify the fix in beta after that.
Attachment #8742462 -
Flags: approval-mozilla-beta?
Attachment #8742462 -
Flags: approval-mozilla-beta+
Attachment #8742462 -
Flags: approval-mozilla-aurora?
Attachment #8742462 -
Flags: approval-mozilla-aurora+
Updated•8 years ago
|
Flags: qe-verify+
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 16•8 years ago
|
||
bugherder uplift |
Comment 17•8 years ago
|
||
bugherder uplift |
Comment 18•8 years ago
|
||
There are no crashes with this signature on Fx 49, 50 and 51 in the last week.
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3ALogModulePrefWatcher%3A%3ARegisterPrefWatcher
Closing as verified.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•