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

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

+++ 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.

Comment 1

[Gabriele Svelto [:gsvelto]]

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

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Gabriele Svelto [:gsvelto]]

(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.

Comment 4

[Gabriele Svelto [:gsvelto]]

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.

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Gabriele Svelto [:gsvelto]]

(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.

Comment 7

[Gabriele Svelto [:gsvelto]]

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?

Comment 8

[Tom Ritter [:tjr]]

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! =)

Comment 9

[Gabriele Svelto [:gsvelto]]

(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.

Comment 10

[Gabriele Svelto [:gsvelto]]

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.

Comment 11

[(not currently active) Ted Mielczarek]

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 12

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - When the crash reporter code is disabled at configure time replace it with a dummy implementation; .mielczarek

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

Comment 13

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from accessible;

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

Comment 14

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from docshell;

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

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from dom;

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

Comment 16

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from gfx;

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

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from ipc;

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

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js;

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

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from layout;

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

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from modules/libpref;

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

Comment 21

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from netwerk;

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

Comment 22

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security;

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

Comment 23

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from storage;

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

Comment 24

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from testing; .mielczarek

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

Comment 25

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from toolkit;

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

Comment 26

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from widget;

This does not remove the directives in widget/ContentCache.cpp as those crash
annotations should be temporary and will be removed once bug 1405832 is fixed.

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

Comment 27

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1402519 - Remove MOZ_CRASHREPORTER directives from xpcom;

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

Comment 28

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922528 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from modules/libpref;

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

Comment 29

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922529 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from netwerk;

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

Comment 30

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922530 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security;

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

Comment 31

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922531 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from storage;

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

Comment 32

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922532 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from testing; .mielczarek

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

Comment 33

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922533 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from toolkit;

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

Comment 34

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922534 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from widget;

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

Comment 35

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922535 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from xpcom;

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

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 :-)

Comment 37

[Gabriele Svelto [:gsvelto]]

(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.

Comment 38

[Gabriele Svelto [:gsvelto]]

(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 39

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8922524 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from gfx;

https://reviewboard.mozilla.org/r/193590/#review198762

Comment 40

[Nicholas Nethercote [inactive]]

Comment on attachment 8922528 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from modules/libpref;

https://reviewboard.mozilla.org/r/193598/#review198782

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8922522 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from docshell;

https://reviewboard.mozilla.org/r/193586/#review198868

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

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 43

[(not currently active) Ted Mielczarek]

Comment on attachment 8922532 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from testing; .mielczarek

https://reviewboard.mozilla.org/r/193606/#review198962

Comment 44

[(not currently active) Ted Mielczarek]

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 45

[Marco Bonardo [:mak]]

Comment on attachment 8922531 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from storage;

https://reviewboard.mozilla.org/r/193604/#review199004

Comment 46

[Patrick McManus [:mcmanus]]

Comment on attachment 8922529 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from netwerk;

https://reviewboard.mozilla.org/r/193600/#review199006

Comment 47

[alexander :surkov (:asurkov)]

Comment on attachment 8922521 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from accessible;

https://reviewboard.mozilla.org/r/193584/#review199054

Comment 48

[Dave Townsend [:mossop]]

Comment on attachment 8922533 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from toolkit;

https://reviewboard.mozilla.org/r/193608/#review199064

Comment 49

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8922530 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security;

https://reviewboard.mozilla.org/r/193602/#review199070

Comment 50

[Nathan Froyd [:froydnj]]

Comment on attachment 8922534 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from widget;

https://reviewboard.mozilla.org/r/193610/#review199152

Comment 51

[Nathan Froyd [:froydnj]]

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.

Comment 52

[Gabriele Svelto [:gsvelto]]

(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.

Comment 53

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922520 [details]
Bug 1402519 - When the crash reporter code is disabled at configure time replace it with a dummy implementation; .mielczarek

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

Comment 54

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922521 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from accessible;

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

Comment 55

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922522 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from docshell;

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

Comment 56

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922523 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from dom;

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

Comment 57

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922524 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from gfx;

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

Comment 58

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922525 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from ipc;

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

Comment 59

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922526 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js;

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

Comment 60

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922527 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from layout;

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

Comment 61

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922528 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from modules/libpref;

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

Comment 62

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922529 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from netwerk;

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

Comment 63

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922530 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from security;

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

Comment 64

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922531 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from storage;

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

Comment 65

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922532 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from testing; .mielczarek

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

Comment 66

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922533 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from toolkit;

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

Comment 67

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922534 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from widget;

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

Comment 68

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922535 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from xpcom;

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

Comment 69

[Gabriele Svelto [:gsvelto]]

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 70

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8922525 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from ipc;

https://reviewboard.mozilla.org/r/193592/#review200256

Comment 71

[Peter Van der Beken [:peterv]]

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 72

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8922526 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js;

Redirecting review, Nicolas can you have a look please?

Comment 73

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8922526 [details]
Bug 1402519 - Remove MOZ_CRASHREPORTER directives from js;

https://reviewboard.mozilla.org/r/193594/#review205954

Comment 74

[Gabriele Svelto [:gsvelto]]

(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.

Comment 75

[Gabriele Svelto [:gsvelto]]

Try is green, time to land.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b6cdf3184447657b1788edfde8370f929e75ef

Comment 76

[Pulsebot]

Pushed by [e-mail]:
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

Comment 77

[Stefan Hindli [:stefan_hindli]]

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

Comment 78

[Gabriele Svelto [:gsvelto]]

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

Comment 79

[Pulsebot]

Pushed by [e-mail]:
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

Comment 80

[Arthur Iakab [arthur_iakab]]

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

Comment 81

[Mike Hommey [:glandium]]

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