+++ This bug was initially created as a clone of Bug #1348273 +++ There are around 300 places in the codebase where we use preprocessor directives to exclude code that should not be built when the crashreporter is disabled. In most instances the directives bracket a call to AnnotateCrashReport() or another function exposed in nsExceptionHandler.h. The directives are needed because when building with --disable-crashreporter the build process doesn't even take the toolkit/crashreporter directory into account even though our code can (and does) include nsExceptionHandler.h nevertheless. This approach is brittle, ugly and makes refactoring crash reporting code extremely hard, as many files need to be audited and sometimes modified in response to changes in nsExceptionHandler.h. Additionally we have crash reporting code living outside of toolkit/crashreporter - for example in ipc/glue - which *is* compiled when --disable-crashreporter is passed, with only the parts that explicitly refer to nsExceptionHandler.h culled via preprocessor directives. A better approach might be to include the toolkit/crashreporter directory in the build even when the crashreporter is disabled and modify it in the following way: - the crashreporter and minidump-analyzer tools are not built - all the breakpad machinery is not built - dummy implementations for the functions in nsExceptionHandler.h are provided This should allow us to freely call the crash reporting machinery without guarding it with preprocessor directives while guaranteeing that it does nothing when it's explicitly disabled.

Ted, what do you think of the approach I'm proposing? IMHO it would make everybody's life a lot easier.

This is a great idea and I have probably suggested it before. :) We do this same thing with the gamepad code, where if we don't have a supported backend we use the 'stub' backend: https://dxr.mozilla.org/mozilla-central/rev/2cd3752963fc8f24f7c202687eab55e83222f608/dom/gamepad/moz.build#49 We kept breaking unsupported platforms with missing ifdefs, which is exactly the problem we keep hitting with the crashreporter.

(In reply to Ted Mielczarek [:ted.mielczarek] (Away Sep 23rd-Oct 1st) from comment #2) > This is a great idea and I have probably suggested it before. :) We do this > same thing with the gamepad code, where if we don't have a supported backend > we use the 'stub' backend: > https://dxr.mozilla.org/mozilla-central/rev/ > 2cd3752963fc8f24f7c202687eab55e83222f608/dom/gamepad/moz.build#49 Thanks for the pointer, we do the same in the HAL component. > We kept breaking unsupported platforms with missing ifdefs, which is exactly > the problem we keep hitting with the crashreporter. Yeah, and ASAN builds too which have the crash reporter disabled. I'll get to work on it on Monday then.

First stab at the dummy crashreporter: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3ea683402c57760627c4551e1ab63ccb6838261 I've implemented all the public methods so that they return failures when used. This caused some interesting problems in the code that would prevent Firefox from starting since some of our code *expects* the crash reporter to be working at all times. Anyway this needs a ton of testing and a lot of extra polish too.

I would just make everything return success. If it's not going to do anything, it doesn't really matter if code thinks it works.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5) > I would just make everything return success. If it's not going to do > anything, it doesn't really matter if code thinks it works. That doesn't really cut it because some functions return objects that are supposed to be usable, such as the child notification pipe. So pretending we're successful prompts the code to act on that.

This is a try run with a working set of patches. The most relevant changes are in the last patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b1e8c8264073a804247b66ad448c4e85bc00159 Ted can you give me some feedback on this before I prepare it for review?

I have seen problems with --disable-crashreporter interacting with --disable-accessibility so I would suggest testing that configuration also. Tor depends on --disable-crashreporter for now, so this is helpful to them too! =)

(In reply to Tom Ritter [:tjr] from comment #8) > I have seen problems with --disable-crashreporter interacting with > --disable-accessibility so I would suggest testing that configuration also. Thanks for the tip, I'll double-check that. I've noticed that the a11y code has intermingled MOZ_CRASHREPORTER and MOZ_ACCESSIBILITY preprocessor guards so I intended to split that part for a separate review to be sure it's handled properly. > Tor depends on --disable-crashreporter for now, so this is helpful to them > too! =) Yeah, it should make things simpler.

I've rebased the patch and locally checked that it builds with all combinations of crashreporter and accessibility support. This is a recent try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=849cfeb712dde7e87d4f5d1714bf44868c4944e1 Ted if you think the approach is sound this is almost ready for review.

That looks sensible. So we'll still have the MOZ_CRASHREPORTER config and define, but we'll remove most usage of the latter, because the APIs will exist and most callers won't have to care?

Comment on attachment 8922527 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from layout; https://reviewboard.mozilla.org/r/193596/#review198760 r=dbaron, although I think a peer/owner of the crashreporting code could happly review this and you don't need to ask layout (etc.) peers. ::: layout/style/nsLayoutStylesheetCache.cpp:24 (Diff revision 1) > +#include "nsISimpleEnumerator.h" > +#include "nsISubstitutingProtocolHandler.h" if you're going to sort them, may as well sort these 2 better :-)

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11) > That looks sensible. So we'll still have the MOZ_CRASHREPORTER config and > define, but we'll remove most usage of the latter, because the APIs will > exist and most callers won't have to care? Yeah, that's the idea. This has two advantages: it cuts away all the #ifdef madness we have in the codebase and it makes further refactoring to the crashreporter a lot easier. The only places that will still care about the defines are those which instance UI elements (e.g. the about:crashes page) and stuff like that. It's a very small subset of what we have now.

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #36) > Comment on attachment 8922527 [details] > Bug 1402519 - Remove MOZ_CRASHREPORTER directives from layout; > > https://reviewboard.mozilla.org/r/193596/#review198760 > > r=dbaron, although I think a peer/owner of the crashreporting code could > happly review this and you don't need to ask layout (etc.) peers. In some modules the changes are straightforward but in others they're not so I thought it would be better to have more eyes on this. > ::: layout/style/nsLayoutStylesheetCache.cpp:24 > (Diff revision 1) > > +#include "nsISimpleEnumerator.h" > > +#include "nsISubstitutingProtocolHandler.h" > > if you're going to sort them, may as well sort these 2 better :-) Good point, I'll do that.

Comment on attachment 8922524 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from gfx; https://reviewboard.mozilla.org/r/193590/#review198762

Comment on attachment 8922528 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from modules/libpref; https://reviewboard.mozilla.org/r/193598/#review198782

Comment on attachment 8922522 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from docshell; https://reviewboard.mozilla.org/r/193586/#review198868

Comment on attachment 8922530 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security; https://reviewboard.mozilla.org/r/193602/#review198914 This looks like a safe patch but I'm not sure I can officially review it. Let's ask Jed too, he's working on our sandbox.

Comment on attachment 8922532 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from testing; .mielczarek https://reviewboard.mozilla.org/r/193606/#review198962

Comment on attachment 8922520 [details] Bug 1402519 - When the crash reporter code is disabled at configure time replace it with a dummy implementation; .mielczarek https://reviewboard.mozilla.org/r/193582/#review198966 Thanks for fixing this! This should make maintenence much easier in the future. ::: toolkit/crashreporter/nsDummyExceptionHandler.cpp:7 (Diff revision 1) > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsExceptionHandler.h" It might be worth putting a note at the top of nsExceptionHandler.h to let people know that when they add a new API they need to add a stub for it to this file as well. On the plus side, since ASAN builds are --disable-crashreporter, that should prevent people from accidentally landing that sort of bustage. ::: toolkit/crashreporter/nsDummyExceptionHandler.cpp:15 (Diff revision 1) > +namespace CrashReporter { > + > +void > +AnnotateOOMAllocationSize(size_t size) > +{ > + ; Are the extraneous semicolons intentional or just search-and-replace cruft?

Comment on attachment 8922531 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from storage; https://reviewboard.mozilla.org/r/193604/#review199004

Comment on attachment 8922529 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from netwerk; https://reviewboard.mozilla.org/r/193600/#review199006

Comment on attachment 8922521 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from accessible; https://reviewboard.mozilla.org/r/193584/#review199054

Comment on attachment 8922533 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from toolkit; https://reviewboard.mozilla.org/r/193608/#review199064

Comment on attachment 8922530 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security; https://reviewboard.mozilla.org/r/193602/#review199070

Comment on attachment 8922534 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from widget; https://reviewboard.mozilla.org/r/193610/#review199152

Comment on attachment 8922535 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from xpcom; https://reviewboard.mozilla.org/r/193612/#review199154 This is so much nicer, thank you for doing this.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #44) > Thanks for fixing this! This should make maintenence much easier in the > future. Yes, I hope it makes everybody's life easier. BTW can you also review the patch for bug 1402153? It removes cruft from the crash reporter code and I'd like to land that first and then this on top. > ::: toolkit/crashreporter/nsDummyExceptionHandler.cpp:7 > (Diff revision 1) > > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +#include "nsExceptionHandler.h" > > It might be worth putting a note at the top of nsExceptionHandler.h to let > people know that when they add a new API they need to add a stub for it to > this file as well. Good point, I'll add that. > On the plus side, since ASAN builds are --disable-crashreporter, that should > prevent people from accidentally landing that sort of bustage. Yeah, even though this was partially motivated precisely by the fact that not everybody knows that ASAN builds disable the crashreporter. This will ensure that if you add an innocent-looking AnnotateCrashReport() call somewhere w/o preprocessor guards then you don't have to wonder why the ASAN build is broken. > ::: toolkit/crashreporter/nsDummyExceptionHandler.cpp:15 > (Diff revision 1) > > +namespace CrashReporter { > > + > > +void > > +AnnotateOOMAllocationSize(size_t size) > > +{ > > + ; > > Are the extraneous semicolons intentional or just search-and-replace cruft? Search-and-replace cruft, I'll get rid of them.

I've addressed the comments, fixed some issues in the patch for the ipc folder as well as an issue with --disable-accessibility builds. Try runs still show a few test failures which I have to investigate. There's one in the ASAN build which is definitely caused by these patches. I still have to figure out if the others are just intermittent failures or not.

Comment on attachment 8922525 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from ipc; https://reviewboard.mozilla.org/r/193592/#review200256

Comment on attachment 8922523 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from dom; https://reviewboard.mozilla.org/r/193588/#review200920 ::: dom/base/nsDocument.cpp:7920 (Diff revision 2) > { > EnumSet<StyleDataType> styleDataTypes = StyleDataTypesWithNode(aAdoptedNode); > if (styleDataTypes.isEmpty()) { > return; > } > -#ifdef MOZ_CRASHREPORTER > + Hmm, given that you didn't have to remove any ifdef around an include it seems like this file is actually missing a nsExceptionHandler.h include?

Comment on attachment 8922526 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js; Redirecting review, Nicolas can you have a look please?

Comment on attachment 8922526 [details] Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js; https://reviewboard.mozilla.org/r/193594/#review205954

(In reply to Nicolas B. Pierron [:nbp] from comment #73) > Comment on attachment 8922526 [details] > Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js; > > https://reviewboard.mozilla.org/r/193594/#review205954 Thanks a lot! I'm still ironing out one last issue with xpcshell tests and this should be ready to land.

Try is green, time to land. https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b6cdf3184447657b1788edfde8370f929e75ef

Pushed by gsvelto@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f8fdf450613f When the crash reporter code is disabled at configure time replace it with a dummy implementation; r=ted.mielczarek https://hg.mozilla.org/integration/mozilla-inbound/rev/24e0dcd01898 Remove MOZ_CRASHREPORTER directives from accessible; r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/8d01c14ac1ca Remove MOZ_CRASHREPORTER directives from docshell; r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4139fa5dca Remove MOZ_CRASHREPORTER directives from dom; r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/e801b0c00134 Remove MOZ_CRASHREPORTER directives from gfx; r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dd6e5b073f Remove MOZ_CRASHREPORTER directives from ipc; r=billm https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3bfd9f932c Remove MOZ_CRASHREPORTER directives from js; r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bb350d1c34 Remove MOZ_CRASHREPORTER directives from layout; r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/a76356fd3359 Remove MOZ_CRASHREPORTER directives from modules/libpref; r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/f405337f3569 Remove MOZ_CRASHREPORTER directives from netwerk; r=mcmanus https://hg.mozilla.org/integration/mozilla-inbound/rev/e1964f4389cd Remove MOZ_CRASHREPORTER directives from security; r=ttaubert https://hg.mozilla.org/integration/mozilla-inbound/rev/cf298d3815de Remove MOZ_CRASHREPORTER directives from storage; r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/01425eae2c48 Remove MOZ_CRASHREPORTER directives from testing; r=ted.mielczarek https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3caca61294 Remove MOZ_CRASHREPORTER directives from toolkit; r=mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d2ad45d8e2 Remove MOZ_CRASHREPORTER directives from widget; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/07fcf163241a Remove MOZ_CRASHREPORTER directives from xpcom; r=froydnj

Backed out 16 changesets (bug 1402519) for conflicts during merge r=backout on a CLOSED TREE https://hg.mozilla.org/integration/mozilla-inbound/rev/ffc12802d5585e08de1a9ae4f2939e05bbea5767 https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=07fcf163241abc77d8d78479eab3880bd6665ffa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

I've updated the patch-set with a fix for bug 1419959, trying to land it again.

Pushed by gsvelto@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/de8b3d96bc55 When the crash reporter code is disabled at configure time replace it with a dummy implementation; r=ted.mielczarek https://hg.mozilla.org/integration/mozilla-inbound/rev/7a584878bf70 Remove MOZ_CRASHREPORTER directives from accessible; r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/cddce6f164c9 Remove MOZ_CRASHREPORTER directives from docshell; r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/536f8e448230 Remove MOZ_CRASHREPORTER directives from dom; r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/7d871d84a6a2 Remove MOZ_CRASHREPORTER directives from gfx; r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/5690f4b48802 Remove MOZ_CRASHREPORTER directives from ipc; r=billm https://hg.mozilla.org/integration/mozilla-inbound/rev/bba47232913a Remove MOZ_CRASHREPORTER directives from js; r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/42ddee4785ed Remove MOZ_CRASHREPORTER directives from layout; r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/d8441fed09a3 Remove MOZ_CRASHREPORTER directives from modules/libpref; r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/cf70ea17f0d1 Remove MOZ_CRASHREPORTER directives from netwerk; r=mcmanus https://hg.mozilla.org/integration/mozilla-inbound/rev/1114ed8bfacd Remove MOZ_CRASHREPORTER directives from security; r=ttaubert https://hg.mozilla.org/integration/mozilla-inbound/rev/c345fb484409 Remove MOZ_CRASHREPORTER directives from storage; r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/313a1a032031 Remove MOZ_CRASHREPORTER directives from testing; r=ted.mielczarek https://hg.mozilla.org/integration/mozilla-inbound/rev/9227bf4929f8 Remove MOZ_CRASHREPORTER directives from toolkit; r=mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/dfab473522a8 Remove MOZ_CRASHREPORTER directives from widget; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd068605282 Remove MOZ_CRASHREPORTER directives from xpcom; r=froydnj

https://hg.mozilla.org/mozilla-central/rev/de8b3d96bc55 https://hg.mozilla.org/mozilla-central/rev/7a584878bf70 https://hg.mozilla.org/mozilla-central/rev/cddce6f164c9 https://hg.mozilla.org/mozilla-central/rev/536f8e448230 https://hg.mozilla.org/mozilla-central/rev/7d871d84a6a2 https://hg.mozilla.org/mozilla-central/rev/5690f4b48802 https://hg.mozilla.org/mozilla-central/rev/bba47232913a https://hg.mozilla.org/mozilla-central/rev/42ddee4785ed https://hg.mozilla.org/mozilla-central/rev/d8441fed09a3 https://hg.mozilla.org/mozilla-central/rev/cf70ea17f0d1 https://hg.mozilla.org/mozilla-central/rev/1114ed8bfacd https://hg.mozilla.org/mozilla-central/rev/c345fb484409 https://hg.mozilla.org/mozilla-central/rev/313a1a032031 https://hg.mozilla.org/mozilla-central/rev/9227bf4929f8 https://hg.mozilla.org/mozilla-central/rev/dfab473522a8 https://hg.mozilla.org/mozilla-central/rev/5bd068605282

You should have left memory/gtest/TestJemalloc.cpp alone. Not a big deal, I'll just revert in bug 1415794.