Closed
Bug 1390624
Opened 7 years ago
Closed 7 years ago
--disable-crashreporter broken due to child minidumps
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tor])
Attachments
(1 file)
It appears that as part of #1360308, PluginModuleParent.h was refactored, and CrashReporterHost.h was included unconditionally. In all (or most) other cases, this file is only included under #ifdef MOZ_CRASHREPORTER
A naive attempt and ifdef-ing all of this under MOZ_CRASHREPORTER failed:
In TerminateChildProcess I found that mTerminateChildProcessCallback (which I had ifdef-ed away) was explicitly included outside the MOZ_CRASHREPORTER block:
http://searchfox.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#1254
This lead me to believe a more delicate approach is needed.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
The actual error:
> 6:03.34 /home/tom/Documents/moz/mingw-work/just-build-7/obj-mingw/dist/include/mozilla/ipc/CrashReporterHost.h:54:45: error: there are no arguments to ‘do_GetCurrentThread’ that depend on a template parameter, so a declaration of ‘do_GetCurrentThread’ must be available [-fpermissive]
> 6:03.34 mTargetThread = do_GetCurrentThread();
Assignee | ||
Updated•7 years ago
|
Attachment #8897565 -
Flags: review?(cyu)
Assignee | ||
Comment 3•7 years ago
|
||
Actually, it seems like the problem is that nsThread.h wasn't included alongside CrashReporterHost.h like it normally is.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tom
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8897565 [details]
Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h
https://reviewboard.mozilla.org/r/168834/#review174312
Thannks for catching this, but I don't think including nsThead.h in PluginModuleParent.h the right way to fix this breakage.
The root cause is that CrashReporterHost.h needs to see the definition of do_GetCurrentThread() so we should have that definition avaiable for any file that includes CrashReporterHost.h. That makes the intention of including nsThread.h in the user of CrashReporterHost.h unclear. Also the topmost header for using do_GetCurrentThread() is nsThreadUtil.h, not nsThread.h. So we should include nsThreadUtil.h in CrashReporterHost.h. Since nsThreadUtil.h also includes nsIThread.h, so I think we can remove the exising #include "nsIThread.h" in CrashReporterHost.h and #include "nsThreadUtil.h" to fix the breakage.
::: dom/plugins/ipc/PluginModuleParent.h:14
(Diff revision 1)
>
> #include "base/process.h"
> #include "mozilla/FileUtils.h"
> #include "mozilla/HangAnnotations.h"
> #include "mozilla/PluginLibrary.h"
> +#include "nsThread.h"
Don't include nsThread.h here. See the above reason.
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8897565 [details]
Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h
https://reviewboard.mozilla.org/r/168836/#review174334
r- for this is not the right way to fix this missing #include bug as in the previous comment.
Attachment #8897565 -
Flags: review?(cyu) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8897565 [details]
Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h
https://reviewboard.mozilla.org/r/168836/#review174746
::: commit-message-2ca97:1
(Diff revision 3)
> +Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtil.h
nit: nsThreadUtils.h in the patch summary.
Attachment #8897565 -
Flags: review?(cyu) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•7 years ago
|
||
Here's a correct try run that doesn't have a bunch of other stuff. I'll let this run then re-mark it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97afae839425136d61562901ce678010310b4d5
Keywords: checkin-needed
Comment 12•7 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(tom)
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(tom)
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b8915cb355a6
Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h r=cyu
Keywords: checkin-needed
Comment 14•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•