Closed Bug 1472789 Opened 2 years ago Closed 2 years ago

crashreporter bits link into libxul and the crashreporter binary

Categories

(Firefox Build System :: General, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This seems weird, and dangerous, but we have e.g.:

https://searchfox.org/mozilla-central/source/toolkit/crashreporter/breakpad-windows-libxul/moz.build

which compiles a bunch of stuff for FINAL_LIBRARY = 'xul'.  It disables STL wrapping, which seems sketchy as well.  The wrapper disabling is probably necessary because of:

https://searchfox.org/mozilla-central/source/toolkit/crashreporter/client/moz.build

which pulls in different breakpad bits for each platform.  On windows, that's done with:

https://searchfox.org/mozilla-central/source/toolkit/crashreporter/client/moz.build#29

which pulls in the bits we were defining for xul, above.

It seems not so great to have this situation, but it obviously works.  Would anything be radically wrong if we:

a) compiled this code for libxul and the crashreporter separately (which seems bad); or
b) just made the crashreporter link against mozglue?

(The wider context here is that I'm trying to make some linker order file bits work for our clang-cl builds, and it's annoying to have code compiled for libxul that also gets linked elsewhere...and specifically in this case, doesn't get linked against mozglue.)
Flags: needinfo?(gsvelto)
I haven't written this code but it seems to me it's doing two separate things that happen to be together just because It Works™ (or worked until now). The first thing is provide the bits of breakpad that the crashreporter client needs (namely the HTTP upload code and possibly some bits from common) then it also provides the parts of breakpad that we do use in libxul.

There's no point in having those two parts lumped together because the crashreporter client does not link with libxul and we don't need HTTP upload support in libxul. So we can probably get rid of this file and move the required bits into toolkit/crashreporter/client/moz.build and toolkit/crashreporter/moz.build.
Flags: needinfo?(gsvelto)
This makes one less place where we link code compiled for libxul into a
place that doesn't link mozglue, and is cleaner to boot.  We don't need
the BREAKPAD_NO_TERMINATE_THREAD define that breakpad-windows-libxul
defines because we're not including the handler code in the
crashreporter binary.
Attachment #8989314 - Flags: review?(core-build-config-reviews)
Assignee: nobody → nfroyd
Blocks: 1444171
Comment on attachment 8989314 [details] [diff] [review]
make the windows crashreporter not dependent on libxul files

Review of attachment 8989314 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but can you remove the few extraneous bits from breakpad-windows-libxul/moz.build now? The sender and http_upload.cc bits are definitely not needed in libxul:
https://dxr.mozilla.org/mozilla-central/rev/9c02d2ecf22050bfee5d70c04a359d8aaff6eb91/toolkit/crashreporter/breakpad-windows-libxul/moz.build#7

Can you also file a followup to do a more thorough cleanup here? It may be that we can do a lot more cleanup, but I don't want to block you on that for now. (Specifically a lot of this complexity is to support the "inject a crash reporter into the flash player" which I suspect we could probably get rid of at this point.)
Attachment #8989314 - Flags: review?(core-build-config-reviews) → review+
Blocks: 1473185
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be04a499d0d0
make the windows crashreporter not dependent on libxul files; r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/be04a499d0d0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.