1265262 - startup crash in mozilla::LogModulePrefWatcher::RegisterPrefWatcher

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[[:philipp]]

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).

Comment 1

[Nathan Froyd [:froydnj]]

Eric, can you take a look at this?

Comment 2

[Eric Rahm [: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

Comment 3

[Eric Rahm [:erahm]]

Attachment: Check for null root branch when registering pref watcher

Comment 4

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
if we do another beta build, maybe we can slip this one in.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Eric Rahm [:erahm]]

(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

[Jim Mathies [:jimm]]

We'd like a fix, but not tracking due to crash volume.

Comment 9

[Jim Mathies [:jimm]]

Eric, do you plan to land this?

Comment 10

[Eric Rahm [: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?

Comment 11

[Nathan Froyd [:froydnj]]

(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...

Comment 12

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/548b35e65fe7e8d3a48f978ce3e8178df3b37753
Bug 1265262 - Check for null root branch when registering pref watcher. r=froydnj

Comment 13

[Eric Rahm [:erahm]]

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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/548b35e65fe7

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/50153501e96e

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/71734e4e560a

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

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.