1390624 - --disable-crashreporter broken due to child minidumps

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Tom Ritter [:tjr]]

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 1

[Tom Ritter [:tjr]]

Attachment: Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h

Review commit: https://reviewboard.mozilla.org/r/168836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168836/

Comment 2

[Tom Ritter [:tjr]]

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();

Comment 3

[Tom Ritter [:tjr]]

Actually, it seems like the problem is that nsThread.h wasn't included alongside CrashReporterHost.h like it normally is.

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

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

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 8897565 [details]
Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168836/diff/1-2/

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 8897565 [details]
Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168836/diff/2-3/

Comment 8

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 8897565 [details]
Bug 1390624 Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168836/diff/3-4/

Comment 10

[Tom Ritter [:tjr]]

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

Comment 11

[Tom Ritter [:tjr]]

Ignoring broken buildbot, this is ready.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8915cb355a6
Resolve missing do_GetCurrentThread instance by including nsThreadUtils.h r=cyu

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b8915cb355a6