Closed Bug 1402519 Opened 6 years ago Closed 6 years ago

Use a dummy crashreporter implementation when building with --disable-crashreporter

Categories

(Toolkit :: Crash Reporting, enhancement)

Product:
Toolkit
Component:
Crash Reporting
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(16 files)

Bug 1402519 - When the crash reporter code is disabled at configure time replace it with a dummy implementation; .mielczarek
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from accessible;
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from docshell;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from dom;
59 bytes, text/x-review-board-request
peterv
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from gfx;
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from ipc;
59 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js;
59 bytes, text/x-review-board-request
nbp
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from layout;
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from modules/libpref;
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from netwerk;
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security;
59 bytes, text/x-review-board-request
ttaubert
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from storage;
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from testing; .mielczarek
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from toolkit;
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from widget;
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from xpcom;
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
+++ 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.
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
(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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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?
Flags: needinfo?(ted)
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?
Flags: needinfo?(ted)
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 :-)
Attachment #8922527 - Flags: review?(dbaron) → review+
(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
Attachment #8922524 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8922528 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from modules/libpref;

https://reviewboard.mozilla.org/r/193598/#review198782
Attachment #8922528 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922522 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from docshell;

https://reviewboard.mozilla.org/r/193586/#review198868
Attachment #8922522 - Flags: review?(bzbarsky) → review+
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.
Attachment #8922530 - Flags: review?(ttaubert) → review+
Comment on attachment 8922532 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from testing; .mielczarek

https://reviewboard.mozilla.org/r/193606/#review198962
Attachment #8922532 - Flags: review?(ted) → review+
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?
Attachment #8922520 - Flags: review?(ted) → review+
Comment on attachment 8922531 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from storage;

https://reviewboard.mozilla.org/r/193604/#review199004
Attachment #8922531 - Flags: review?(mak77) → review+
Comment on attachment 8922529 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from netwerk;

https://reviewboard.mozilla.org/r/193600/#review199006
Attachment #8922529 - Flags: review?(mcmanus) → review+
Comment on attachment 8922521 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from accessible;

https://reviewboard.mozilla.org/r/193584/#review199054
Attachment #8922521 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8922533 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from toolkit;

https://reviewboard.mozilla.org/r/193608/#review199064
Attachment #8922533 - Flags: review?(dtownsend) → review+
Comment on attachment 8922530 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security;

https://reviewboard.mozilla.org/r/193602/#review199070
Attachment #8922530 - Flags: review?(jld) → review+
Comment on attachment 8922534 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from widget;

https://reviewboard.mozilla.org/r/193610/#review199152
Attachment #8922534 - Flags: review?(nfroyd) → review+
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.
Attachment #8922535 - Flags: review?(nfroyd) → review+
(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
Attachment #8922525 - Flags: review?(wmccloskey) → review+
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?
Attachment #8922523 - Flags: review?(peterv) → review+
Blocks: 1416078
Comment on attachment 8922526 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js;

Redirecting review, Nicolas can you have a look please?
Attachment #8922526 - Flags: review?(jorendorff) → review?(nicolas.b.pierron)
Comment on attachment 8922526 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js;

https://reviewboard.mozilla.org/r/193594/#review205954
Attachment #8922526 - Flags: review?(nicolas.b.pierron) → review+
(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.
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
Depends on: 1419959
I've updated the patch-set with a fix for bug 1419959, trying to land it again.
Flags: needinfo?(gsvelto)
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
You should have left memory/gtest/TestJemalloc.cpp alone. Not a big deal, I'll just revert in bug 1415794.
Depends on: 1422291
You need to log in before you can comment on or make changes to this bug.