1460329 - Stop using --disable-crashreporter in Asan builds

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[(not currently active) Ted Mielczarek]

Our ASan builds use `--disable-crashreporter` because Breakpad's handlers get in the way of ASan's handlers. However, we've had to jump through a bunch of hoops in the past to make things work because of this, so it would be easier if they could stop doing that. (Most recently: bug 1460243.)

Assuming all the crashreporter code builds OK with ASan enabled (which I don't think should be a problem) we should be able to make sure the Breakpad exception handler doesn't get set by tweaking this code (which currently doesn't enable it by default on debug builds, but allows explicitly enabling it):
https://dxr.mozilla.org/mozilla-central/rev/0cd106a2eb78aa04fd481785257e6f4f9b94707b/toolkit/crashreporter/nsExceptionHandler.cpp#1469

We could either:
a) Make ASan builds work like debug builds, where setting `MOZ_CRASHREPORTER=1` will enable Breakpad or
b) Make it so ASan builds can never have Breakpad enabled.

If we choose a) we'll probably also have to tweak some things in the test harnesses, because most of them set `MOZ_CRASHREPORTER=1` in the browser environment.

We'd also have to mark tests that use the crashreporter as `skip-if = asan` (or whatever the conditional we have is), like:
https://dxr.mozilla.org/mozilla-central/rev/0cd106a2eb78aa04fd481785257e6f4f9b94707b/toolkit/crashreporter/test/unit/xpcshell.ini#3

Comment 1

[Tyson Smith [:tsmith]]

Is there a situation where we would ever need Breakpad enabled in an ASan build? I've never come across one. I vote for b.

Comment 2

[Christian Holler (:decoder)]

I don't understand bug 1460243. ASan Nightly shouldn't have symbols.zip because it is useless for these builds. If I understand your proposal correctly, you suggest to enable the crashreporter so we have symbols.zip, which then serves no purpose for these builds? If that is the case, then I suggest sticking to --disable-crashreporter unless there is a reason why the crash reporter or parts of it would really be required for ASan builds. ASan builds will always require some special treatment, enabling the crash reporter just seems like moving that work to somewhere else (e.g. tests as you mention) and I would imagine there are other places as well that will suddenly become a problem.

We are currently progressing with the ASan Nightly Project, any kind of breakage due to this kind of change would be absolutely fatal. So overall I see a big risk associated with doing this (breaking the ASan builds in one way or another), but the gain from doing this is not clear to me right now. Can you please elaborate why this change is necessary and how you plan to ensure that this doesn't break production ASan builds and fuzzing operations?

Comment 3

[(Away)]

(In reply to Christian Holler (:decoder) from comment #2)
> I don't understand bug 1460243. ASan Nightly shouldn't have symbols.zip
> because it is useless for these builds.

Why is symbols.zip useless for those builds?

Comment 4

[Christian Holler (:decoder)]

(In reply to David Major [:dmajor] from comment #3)
> (In reply to Christian Holler (:decoder) from comment #2)
> > I don't understand bug 1460243. ASan Nightly shouldn't have symbols.zip
> > because it is useless for these builds.
> 
> Why is symbols.zip useless for those builds?

Because ASan doesn't (and can't?) use these symbols (at least on Linux and Mac, I don't know what the Windows situation currently is). The ASan symbolizer needs symbols in the binaries to work properly. Let me know if this is wrong for Windows.

Comment 5

[(Away)]

(In reply to Christian Holler (:decoder) from comment #4)
> Because ASan doesn't (and can't?) use these symbols (at least on Linux and
> Mac, I don't know what the Windows situation currently is). The ASan
> symbolizer needs symbols in the binaries to work properly. Let me know if
> this is wrong for Windows.

Ah, now I understand. Yeah, Windows uses separate .pdb files even today, the only difference is whether they are placed in symbols-full.zip or next to the binaries in target.zip.

So I guess the question is -- Ted, was your intent to do this just for Windows ASan, or all ASan?

Comment 6

[(not currently active) Ted Mielczarek]

Enabling it on Windows would make some things simpler because it would let us remove the workarounds we currently have to put PDB files in the package. My original thought was to enable it everywhere simply because it'd be one less difference between build types, and every difference is a potential point of failure. Since bug 1402519 landed it's less of a problem--we always have at least a dummy crashreporter implementation available, so there aren't as many `#ifdef MOZ_CRASHREPORTER` statements to deal with.

Christian: if you think the work to make this happen would outweigh the potential benefits then we can WONTFIX this and move on in life.

Comment 7

[(Away)]

Personally, I'd like to see this happen for Windows at least. If the needs of Linux builds are different, I have no objection to a WONTFIX for that platform.

Comment 8

[Christian Holler (:decoder)]

If Windows needs files generated by --enable-crashreporter then I am all for enabling it for Windows (only). Doing so would also allow us to get a first glimpse at what other problems we might hit in this combination (though not all, as some will be OS-specific), in case we plan to later enable this on Linux/Mac.