startup crash in mozilla::LogModulePrefWatcher::RegisterPrefWatcher

VERIFIED FIXED in Firefox 49

Status

()

Core
XPCOM
P3
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: erahm)

Tracking

({crash, regression})

46 Branch
mozilla51
x86
Windows NT
crash, regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox46- wontfix, firefox47+ wontfix, firefox48 wontfix, firefox49 verified, firefox50 verified, firefox51 verified)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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).
Eric, can you take a look at this?
Flags: needinfo?(erahm)
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)
Created attachment 8742462 [details] [diff] [review]
Check for null root branch when registering pref watcher
Attachment #8742462 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED

Comment 4

2 years ago
[Tracking Requested - why for this release]:
if we do another beta build, maybe we can slip this one in.
status-firefox46: affected → wontfix
status-firefox47: --- → affected
tracking-firefox46: --- → ?
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-firefox46: ? → -
tracking-firefox47: --- → +
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)
(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

2 years ago
We'd like a fix, but not tracking due to crash volume.
status-firefox47: affected → wontfix
status-firefox48: --- → wontfix
status-firefox49: --- → affected

Comment 9

2 years ago
Eric, do you plan to land this?
status-firefox49: affected → fix-optional
status-firefox50: --- → fix-optional
status-firefox51: --- → fix-optional
Flags: needinfo?(erahm)
(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)
(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)
Attachment #8742462 - Flags: review+
Priority: -- → P3
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 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+
Flags: qe-verify+

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/548b35e65fe7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Comment 16

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/50153501e96e
status-firefox50: fix-optional → fixed

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/71734e4e560a
status-firefox49: fix-optional → fixed
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
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.