Closed Bug 1472789 Opened 2 years ago Closed 2 years ago

crashreporter bits link into libxul and the crashreporter binary


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



(firefox63 fixed)

Tracking Status
firefox63 --- fixed


(Reporter: froydnj, Assigned: froydnj)


(Blocks 1 open bug)



(1 file)

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

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:

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

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/ and toolkit/crashreporter/
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/ now? The sender and bits are definitely not needed in libxul:

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
make the windows crashreporter not dependent on libxul files; r=ted.mielczarek
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.