Closed
Bug 1067547
Opened 9 years ago
Closed 6 years ago
XRE_InitChildProcess calls profiler_init() before NS_LogInit()
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: mccr8, Assigned: tromey)
Details
Attachments
(2 files, 3 obsolete files)
I was looking into what is causing this: WARNING: XPCOM objects created/destroyed from static ctor/dtor It looks like that in a child process starting up the profiler before we do NS_LogInit(), this causes the deadlock detector to start doing XPCOM stuff, which makes the XPCOM logging sad. I don't see a similar problem in the parent process for whatever reason, even though it also sets up the profiler before NS_LogInit(). I don't know if this is worth investigating or fixing but I figured I'd at least file a bug.
Comment 1•9 years ago
|
||
XPCOM-using processes call profiler_init from within NS_InitXPCOM, which is well after we've initialized logging/leak detection: http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#485 We seem to be *also* initializing profiling much earlier in various startup paths, with blame leading back to bug 872496. In Firefox, NS_InitLogging is already called before XRE_main for various things like local files, but we shouldn't rely on that. We should either make profiling imply another logging Init/Finish pair, or we should change the various places touched by http://hg.mozilla.org/mozilla-central/rev/383bed640c7b to move the NS_LogInit call before we start the profiler.
Assignee | ||
Comment 2•8 years ago
|
||
I tripped across this problem while working on bug 1047124. In that bug, I tried adding MOZ_COUNT_* calls to various profiler-related classes, e.g., PseudoStack. This lead to more apparent leaks being reported, because the profiler is initialized before logging. See https://bugzilla.mozilla.org/show_bug.cgi?id=1047124#c31 I have a patch that I'll attach shortly. It went through "try" (it was part of the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1047124#c33).
Assignee | ||
Comment 3•8 years ago
|
||
This patch arranges for NS_LogInit to be called before profiler_init. This makes it possible to track possible memory leaks coming from the profiler. It also adds some MOZ_COUNT_* markup to various profiler objects; these were sanity checks from bug 1047124. I believe I tracked down all the NS_LogInit calls that matter. For XRE_InitChildProcess I introduced a new ScopedLogging. This class already appears in several places in the source. I think it would be better to have a single definition of it, but I did not know where to put it. Also in XRE_InitChildProcess, the use of the RAII style means that XRE_DeinitCommandLine will be called in a different order relative to profiler_shutdown and NS_LogTerm. This did not seem to cause problems on "try" and seemed safe to me, but if it is not ok, it is readily solved by introducing a new scope.
Assignee | ||
Updated•8 years ago
|
Attachment #8518980 -
Flags: review?(benjamin)
Comment 4•8 years ago
|
||
Comment on attachment 8518980 [details] [diff] [review] call NS_LogInit before profiler_init You could probably put ScopedLogging in nsXPCOM.h.
Attachment #8518980 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ttromey
Assignee | ||
Comment 5•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9d4aa624d79c
Assignee | ||
Comment 6•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=85bffb3a7755
Assignee | ||
Comment 7•8 years ago
|
||
Here's an updated version that unifies the various copies of ScopedLogging into nsXPCOM.h.
Attachment #8518980 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Rebased for changes to bug 1047124; and moved one MOZ_COUNT_DTOR call from that patch into this one.
Attachment #8526304 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5f11635cb21c
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8535079 [details] [diff] [review] call NS_LogInit before profiler_init I wasn't sure if the r+ carried over, since I've moved ScopedLogging into nsXPCOM.h; so I'm requesting another review to be on the safe side. Thanks!
Attachment #8535079 -
Flags: review?(benjamin)
Updated•8 years ago
|
Attachment #8535079 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #11) > What about all the B2G debug leaks on the Try run? I will look. I had thought those were pre-existing but I was wrong about that.
Assignee | ||
Comment 13•8 years ago
|
||
Consider this successful try build: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=78d01e271d8a The B2G ICS Emulator Debug M(1) full log is here: https://tbpl.mozilla.org/php/getParsedLog.php?id=54852690&tree=Mozilla-Inbound&full=1 If you search for leakcheck you will see a bunch of existing leak reports: 23:44:25 INFO - TEST-INFO | leakcheck | default process: leaked 1 CameraPreferences (12 bytes) 23:44:25 INFO - TEST-INFO | leakcheck | default process: leaked 1 Composer2D (8 bytes) 23:44:25 INFO - TEST-INFO | leakcheck | default process: leaked 3 CondVar (72 bytes) [...] Something must be different in my run; I didn't look yet, but I thought I'd note this in the bug to make it simpler to find again.
Assignee | ||
Comment 14•8 years ago
|
||
The difference (of course) is from the patch; the failing run mentions: 13:57:38 INFO - TEST-INFO | leakcheck | default process: leaked 2 PseudoStack (32896 bytes)
Assignee | ||
Comment 15•8 years ago
|
||
I dug some more and the issue is that this patch changes the leak total to 38048 bytes leaked, but the default leak threshold is 5180 bytes. I'm reluctant to bump the leak threshold for this patch. This patch is a cleanup and a safety measure, but doesn't catch any actual leaks (they were fixed as a prerequisite to this). On the other hand, bumping the leak threshold might result in new leaks going unnoticed. I think this is related to bug 1038943 and its dependencies. My inclination is to mark this bug as blocked by some other bug, pending a fix, I think, to b2g leak-checking. I'm not sure which bug this might be. Maybe bug 1070068?
Assignee | ||
Comment 16•6 years ago
|
||
I finally got back to rebasing this patch. The main reason for this patch was landed in bug 1172138. So, one possible solution here would be to mark this as a dup. However, I think the remaining bits -- adding more uses of MOZ_COUNT_CTOR to the profiler, and unifying the ScopedLogging implementations -- are reasonable cleanups. Do reviews ever expire? I don't know, so I'll re-request review.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8535079 -
Attachment is obsolete: true
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8794914 [details] Bug 1067547 - add more uses of MOZ_COUNT_CTOR to tools/profiler; https://reviewboard.mozilla.org/r/81138/#review80336
Attachment #8794914 -
Flags: review?(benjamin) → review+
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8794915 [details] Bug 1067547 - unify ScopedLogging implementations and use in nsEmbedFunctions; https://reviewboard.mozilla.org/r/81140/#review80338
Attachment #8794915 -
Flags: review?(benjamin) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•6 years ago
|
||
Rebased. I re-ran the one failing test here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e762c2e4c3db13ddf3d72e5e915a61e1777f4571
Comment 24•6 years ago
|
||
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/326e6c945c37 add more uses of MOZ_COUNT_CTOR to tools/profiler; r=bsmedberg https://hg.mozilla.org/integration/autoland/rev/e8f4dc9c44cc unify ScopedLogging implementations and use in nsEmbedFunctions; r=bsmedberg
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/326e6c945c37 https://hg.mozilla.org/mozilla-central/rev/e8f4dc9c44cc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•