1438209 - Disabling crash reporter crashes on startup in [struct ParamTraits<mozilla::ipc::TransportDescriptor>::Read]

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P1)

Description

[RealDolos]

STR:
- Add `ac_add_options --disable-crashreporter` to your .mozconfig
- Build for Windows (I tested a win64 build on Windows 10, using MSVC2017)
- ./mach build && ./mach run

Result
- Always crashes in this release assertion https://hg.mozilla.org/mozilla-central/file/bcc0a91dd43f/ipc/glue/Transport_win.h#l80

When removing `ac_add_options --disable-crashreporter` again, there is no such crash.

I bisected this to first occur due to https://hg.mozilla.org/mozilla-central/rev/bcc0a91dd43f which was added via bug 1407693 (Part 2), therefore CC'ing people involved there.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

I suspect what needs to happen is these few lines https://hg.mozilla.org/mozilla-central/rev/bcc0a91dd43f#l1.101 need to be wrapped in an `if (!CrashReporter::IsDummy())`, but I won't be able to verify with a Windows machine until tomorrow.

Comment 2

[RealDolos]

Alex, yep, wrapping like that makes the crash go away for me.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Cool. If you're up for uploading a patch that does that, I (or probably :gsvelto) can review it and get this landed!

Comment 4

[RealDolos]

Sorry, but I feel not comfortable to provide such a patch, since I don't actually know what it does and how it fixes things specifically, nor do I know anything of the code it touches, and therefore cannot determine if it actually fixes the bug itself (or what the actual bug is for that matter), or merely hides it. I only blindly applied your suggestion.

My intuition tells me that aResult->mDestinationProcessId as used in the assertion might have a bogus value (e.g. never actually gets set up, or gets set to a garbage value) and thus should be fixed to always have a sane value (even if only to a sentinel value), but that's only intuition and you actually have worked with the code and written a bunch of it, so you're in a better position to diagnose what's going on and what needs to change.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1438209 - fixed Windows builds with the crashreporter disabled;

Do not pass command line arguments to child processes that they won't read.

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

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8951263 [details]
Bug 1438209 - fixed Windows builds with the crashreporter disabled;

Redirecting to Ted since I'm on PTO.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8951263 [details]
Bug 1438209 - fixed Windows builds with the crashreporter disabled;

https://reviewboard.mozilla.org/r/220522/#review226870

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Thanks Ted!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea26341dbc8a
fixed Windows builds with the crashreporter disabled; r=ted

Comment 12

[Raul Gurzau (:RaulG)]

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