Closed Bug 1390624 Opened 3 years ago Closed 3 years ago

--disable-crashreporter broken due to child minidumps

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

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.
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();
Actually, it seems like the problem is that nsThread.h wasn't included alongside CrashReporterHost.h like it normally is.
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 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 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+
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
Ignoring broken buildbot, this is ready.
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(tom)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/b8915cb355a6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.