Closed Bug 1432979 Opened 3 years ago Closed 2 years ago

logging nsHostResolver:5 crashes Firefox: "accessing non-early pref before late prefs are set"

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox60 --- affected

People

(Reporter: bagder, Unassigned)

References

Details

(Whiteboard: [trr])

Attachments

(2 files)

STR:

I build current Firefox nightly (from github/gecko-dev), updated today.

$ ./mach run --debug

In the browser, go to "about:networking" and select "Logging" in the left menu.

In the bottom-most text field, edit the text to only contain "timestamp,sync,nsHostResolver:5" and press set "Set Log Modules" button.

Open a new tab, and go to a web site you didn't visit before. In my case in my just started browser I entered "www.twitter.com" and by then Firefox had already crashed.

The back trace for the crash is attached.
That's pretty spectacular. We're in `SetLatePreferences` [1], that sets 'logging.config.sync', which triggers an observer that then tries to load the value [2], and that blows up [3] because we haven't finished `SetLatePreferences` yet.

[1] https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/modules/libpref/Preferences.cpp#3491-3493
[2] https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/xpcom/base/LogModulePrefWatcher.cpp#153-156
[3] https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/modules/libpref/Preferences.cpp#789-790
I think adding logging.config.sync to the early prefs list in dom/ipc/ContentPrefs.cpp should be enough to fix this.
I tried adding the single pref to the early prefs list, but then I instead hit the same problem with "logging.config.add_timestamp" ... so I added that one as well and retried only to then have the same happen with "logging.nsHostResolver"...

This is the patch I ultimately needed to make it stop crashing. 4 logging.* prefs.
Over in bug 1434852 I want to land a new larger name resolver feature soonish and for debugging that feature, setting "nsHostResolver:5" is what I want - I would therefore prefer to have this crash fixed timely to help me and others debug it!

Is there anything more I can do to help?
We should fix the late preferences stuff; setting `gPhase` [1] before updating the late prefs is probably the easiest fix.

[1] https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/modules/libpref/Preferences.cpp#3495-3498
Flags: needinfo?(n.nethercote)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #5)
> We should fix the late preferences stuff; setting `gPhase` [1] before
> updating the late prefs is probably the easiest fix.

That's not safe in general. A pref update callback could consult another pref that hasn't been set yet.

In the short term, adding the prefs to the early list is the best solution. Beyond that, I hope to get rid of the early/late split in bug 1436911.
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #6)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #5)
> > We should fix the late preferences stuff; setting `gPhase` [1] before
> > updating the late prefs is probably the easiest fix.
> 
> That's not safe in general. A pref update callback could consult another
> pref that hasn't been set yet.
> 
> In the short term, adding the prefs to the early list is the best solution.
> Beyond that, I hope to get rid of the early/late split in bug 1436911.

You'll have to add literally every log module to the whitelist (~420).
Depends on: 1436911
Whiteboard: [trr]
I tried to reproduce this issue on a current build, but I would say that the fixing of bug 1436911 seems to have solved this one (as intended).
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.