1309573 - Many places (including all of SpiderMonkey) cannot set the crash reason in crash reports

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

As a result, we don't meet the conditions at [1], and as a result our MOZ_CRASHes and assertions don't get reason strings in Socorro!

I hacked up a fix locally that just copies the MOZ_CRASHREPORTER code from old-configure.in to js/src/old-configure.in and adds |DEFINES['MOZILLA_INTERNAL_API'] = True| to js/src/moz.build (which let me confirm that this fixes the problem in a local build [2]), but that breaks things like jsapi-tests that rely on the static js library.

This is preventing us from making further progress on bug 1124397, as the crashes have probably shifted to a new signature but aren't getting annotated. It probably also makes other SM-related crashes harder to figure out!

[1] https://dxr.mozilla.org/mozilla-central/rev/7ae377917236b7e6111146aa9fb4c073c0efc7f4/mfbt/Assertions.h#25
[2] https://crash-stats.mozilla.com/report/index/3e0eb989-96da-4686-85e6-d9d9c2161012

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Thinking about this a bit more, optionally making JSAPI MOZILLA_INTERNAL_API probably means compiling it twice unless we do something clever like linking in a dummy version of CrashReporter::AnnotateMozCrashReason() for the standalone JS library. Could we special case this function somehow and just take away the MOZILLA_INTERNAL_API requirement? MOZ_CRASHREPORTER seems much less onerous.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Since this concerns MFBT, you should probably be in the loop too!

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Weakly link AnnotateMozCrashReason so JSAPI can use it (WIP).

This changes MOZ_CRASH_ANNOTATE() to weakly link to AnnotateMozCrashReason(). Weak linking isn't something that Windows officially supports, but we can simulate it using an undocumented linker option that Microsoft uses internally in their VCRuntime (via [1]).

I tested this on 32-bit and 64-bit Windows 10, and it works just fine [2][3] (I also tested jsapi-tests and confirmed that gAnnotateMozCrashReason is null there). It also works on Linux [4].

On OSX, from the very helpful information at [5], we're going to need something like [6] - but I'm not sure where to put it (js/src/Makefile.in maybe?). It'd be great if someone else could finish this patch since I don't have a Mac to test on (and I doubt Try works for this).

Just asking for feedback right now since it's a WIP and since it *is* using an undocumented feature on Windows, and I'd like to hear people's thoughts on that.

[1] http://stackoverflow.com/questions/2290587/gcc-style-weak-linking-in-visual-studio/11529277#11529277
[2] https://crash-stats.mozilla.com/report/index/7fd5ab99-aea0-404b-b56d-4adb32161015
[3] https://crash-stats.mozilla.com/report/index/d138ce71-5902-41bf-a719-9cfc72161015
[4] https://crash-stats.mozilla.com/report/index/97553010-c114-446c-860c-828712161015
[5] https://glandium.org/blog/?p=2764
[6] https://dxr.mozilla.org/mozilla-central/source/mozglue/build/replace_malloc.mk

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Oh, that patch also copies the MOZ_CRASHREPORTER stuff over from old-configure.in to js/src/old-configure.in (I'm not sure if that's the right thing to do, but it works), and adds a new define (MOZILLA_JSAPI_INTERNAL) to give JSAPI access to the crash reporter without having to link with everything else related to MOZILLA_INTERNAL_API.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8801532 [details] [diff] [review]
Weakly link AnnotateMozCrashReason so JSAPI can use it (WIP).

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

My comment below expresses my understanding of why we need to do things this way.  Is this correct?

I do wonder if we shouldn't put the declaration of the symbol in mozglue instead, and then have startup set the symbol appropriately.  I guess we could crash even before that stuff is initialized correctly, so we'd rather have the linker do it for us?

::: js/src/jscrash.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/Assertions.h"
> +
> +/* Enables JSAPI to annotate crashes on Windows. */

Maybe expand on this a little, e.g.

Enables JSAPI to annotate crashes on Windows.

When JS code crashes via MOZ_CRASH, MOZ_CRASH_ANNOTATE is used to add extra information about the crash reason.  To do this, we need to be able to call into the crashreporter itself.  But as we compile the same set of JS object files for use in Firefox (which has the crash reporter machinery) and the JS shell (which does not have the crash reporter machinery), we need a way to do this that will work correctly in both cases.  MOZ_CRASH_ANNOTATE on Unix platforms uses weak linking for the function that calls into the crashreporter machinery.  On Windows platforms, which don't support weakly-linked functions, MOZ_CRASH_ANNOTATE uses a weakly-linked function pointer to do the job; the below sets up the declarations of the symbol correctly for JS code.  (Gecko code can just use the definition in nsExceptionHandler.h, which is not available to JS code.)

::: mfbt/Assertions.h
@@ -27,2 @@
>  namespace CrashReporter {
> -// This declaration is present here as well as in nsExceptionHandler.h

I think the information in this comment is still relevant--people might wonder why we're duplicating the information here when we could be including nsExceptionHandler.h.  Reword the comment so that it fits better with the new world order?

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks for the feedback!

(In reply to Nathan Froyd [:froydnj] from comment #5)
> I do wonder if we shouldn't put the declaration of the symbol in mozglue
> instead, and then have startup set the symbol appropriately.  I guess we
> could crash even before that stuff is initialized correctly, so we'd rather
> have the linker do it for us?

I initially tried to put the symbol in MFBT itself (by making an Assertions.cpp), which is part of mozglue, but I couldn't get it to link on Windows. That's probably because it ends up in a standalone dll file, but using MFBT_API to export and import it as appropriate didn't work either (I have no idea why the atomic variable in ChaosMode.cpp doesn't fail in the same way, but that's only used in a few files in JSAPI). It's entirely possible I was just doing something wrong though, my understanding of the build system is still pretty spotty.

> When JS code crashes via MOZ_CRASH, MOZ_CRASH_ANNOTATE is used to add extra
> information about the crash reason.  To do this, we need to be able to call
> into the crashreporter itself.  But as we compile the same set of JS object
> files for use in Firefox (which has the crash reporter machinery) and the JS
> shell (which does not have the crash reporter machinery), we need a way to
> do this that will work correctly in both cases.  MOZ_CRASH_ANNOTATE on Unix
> platforms uses weak linking for the function that calls into the
> crashreporter machinery.  On Windows platforms, which don't support
> weakly-linked functions, MOZ_CRASH_ANNOTATE uses a weakly-linked function
> pointer to do the job; the below sets up the declarations of the symbol
> correctly for JS code.  (Gecko code can just use the definition in
> nsExceptionHandler.h, which is not available to JS code.)

That matches my understanding, except for the last line: It's not so much that JSAPI can't use nsExceptionHandler.h (Gecko code mostly doesn't either, because it includes Windows.h), but that in some cases the (final) library doesn't get linked with the object file generated from nsExceptionHandler.cpp. I'll add something like that comment, thanks :)

> > -// This declaration is present here as well as in nsExceptionHandler.h
> 
> I think the information in this comment is still relevant--people might
> wonder why we're duplicating the information here when we could be including
> nsExceptionHandler.h.  Reword the comment so that it fits better with the
> new world order?

Yeah, I wasn't sure what to change it to so I put it off for the WIP patch. I'll come up with something more fitting for the final patch (depending on glandium's feedback on the approach of course).

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8801532 [details] [diff] [review]
Weakly link AnnotateMozCrashReason so JSAPI can use it (WIP).

Nathan, thanks for the suggestion. I finally figured out where I went wrong trying to get things to link, and dynamically initializing the function pointer when we actually set up the crash reporter is a great idea. I've got it working on Windows, and I'm testing Linux now.

Mike, I'm obsoleting this patch and removing the feedback request - the new approach isn't nearly as scary. I see that you have a lot of outstanding review requests, so I think I'll ping Chris for review on the new patch instead.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 1: Set AnnotateMozCrashReason() dynamically and let everything use it.

I'm still working out some build issues, but I think this part is ready for review. As per your suggestion, this adds a function pointer that is initially null and gets set in SetExceptionHandler() and a couple of other functions at startup.

This allows practically all code to use it, including C code, so I removed all the qualifiers except MOZILLA_EXTERNAL_LINKAGE, which I use in part 3 to get things to link. I considered making the function pointer atomic, but it should only be set once when a process is initializing, so I think it's okay (and using atomics would have made using it from C code more difficult).

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2: Tell the rooting analysis to ignore the new function pointer.

This adds the annotation I mentioned on IRC. As per part 1, this function pointer should only ever be null or point to a very simple function that just sets a global value prior to crashing, so it can't be a GC hazard.

Without this annotation, parts 1 and 3 add ~5k 'hazards' to the shell and 15k to the browser :)

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 3: Add MOZILLA_EXTERNAL_LINKAGE or link to mfbt or mozglue where necessary.

These are the changes needed to get this to link for all the configurations supported by automation. It's more ad hoc than I would like - I suggestively placed the MOZILLA_EXTERNAL_LINKAGE defines near instances of DISABLE_STL_WRAPPING, but they don't always match up. I tested several nonstandard configurations locally, like enabling jemalloc4, disabling jemalloc and enabling DMD, so hopefully I took care of everything used in practice.

In most cases adding MOZILLA_EXTERNAL_LINKAGE was enough, but in one case I needed to link to mozglue (because a dependency now relied on it), and for several of the libraries linked into mozglue I decided to link directly to mfbt. This should mean that crashes in jemalloc will now also get a crash reason, which I think would be useful as the stacks aren't always obvious due to inlining. In general I tried to ensure that as many places as possible have access to the crash reason, but it's possible I was a bit overzealous.

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

Try run, which will hopefully show that I worked out all the kinks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa85b3b2a965

(OS X 10.7 opt cgc and OS X 10.7 debug arm are expected to fail I think, with a timeout and an unused field warning respectively)

Comment 12

[Emanuel Hoogeveen [:ehoogeveen]]

Renaming this bug to better reflect the issue and fix (since the fix adds neither MOZ_CRASHREPORTER nor MOZILLA_INTERNAL_API to SpiderMonkey).

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8803575 [details] [diff] [review]
Part 3: Add MOZILLA_EXTERNAL_LINKAGE or link to mfbt or mozglue where necessary.

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

I expect Mike will be a better reviewer for this patch.

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8803552 [details] [diff] [review]
Part 1: Set AnnotateMozCrashReason() dynamically and let everything use it.

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

This version is much nicer than the previous version, thanks for investigating this!  r=me

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8803575 [details] [diff] [review]
Part 3: Add MOZILLA_EXTERNAL_LINKAGE or link to mfbt or mozglue where necessary.

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

Adding manual dependencies on mozglue and mfbt is most certainly wrong, the subsequent use of IMPL_MFBT is debatable, and the use of MOZILLA_EXTERNAL_LINKAGE seems not very useful.

Taking a step back, all AnnotateMozCrashReason does is set a global gMozCrashReason variable, that is then read in MinidumpCallback or PrepareChildExceptionTimeAnnotations. It seems to me we could just get rid of the function, and move gMozCrashReason to mfbt. Which means MOZ_CRASH_ANNOTATE can just set gMozCrashReason directly and be done.

And for the few things that do need the symbol because they use MOZ_CRASH but don't link mozglue, add /mfbt/Assertions.cpp to the sources in the corresponding directory. That shouldn't be required in more than a couple moz.build files.

Comment 16

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Mike Hommey [:glandium] from comment #15)
> Adding manual dependencies on mozglue and mfbt is most certainly wrong, the
> subsequent use of IMPL_MFBT is debatable, and the use of
> MOZILLA_EXTERNAL_LINKAGE seems not very useful.

In my defense, I only added a manual dependency to mozglue in one place and to mfbt inside libs that end up as part of mozglue anyway (except on Linux). Still I see your point, and including /mfbt/Assertions.cpp directly works as well (I didn't know it could be included that way, with the slash prefix). I do think MOZILLA_EXTERNAL_LINKAGE or equivalent is necessary in a couple of places where we create host libraries, but I'll minimize the usage.

> Taking a step back, all AnnotateMozCrashReason does is set a global
> gMozCrashReason variable, that is then read in MinidumpCallback or
> PrepareChildExceptionTimeAnnotations. It seems to me we could just get rid
> of the function, and move gMozCrashReason to mfbt. Which means
> MOZ_CRASH_ANNOTATE can just set gMozCrashReason directly and be done.

When you're right you're right. This is much simpler, thanks for the suggestion.

> And for the few things that do need the symbol because they use MOZ_CRASH
> but don't link mozglue, add /mfbt/Assertions.cpp to the sources in the
> corresponding directory. That shouldn't be required in more than a couple
> moz.build files.

I think you're underestimating the number of weird places that use MOZ_CRASH :) I think my patch was pretty minimal already, but I'll change it to build /mfbt/Assertions.cpp where possible.

Comment 17

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 1 v3: Define the crash reason in MFBT and let everything use it.

Asking for review again because of the changed approach. Even simpler now thanks to glandium's suggestion :)

Comment 18

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2 v2: Add MOZILLA_EXTERNAL_LINKAGE or link /mfbt/Assertions.cpp where necessary.

I think this is as direct as I can make it. This adds a few warnings about importing locally defined symbols on Windows (because IMPL_MFBT isn't defined), but otherwise looks clean on Try.

I used MOZILLA_EXTERNAL_LINKAGE in three places:
1) To build/unix/stdc++compat and mozglue/linker, because adding Assertions.cpp to the sources of a host library leads to redefined symbol errors, and
2) To toolkit/xre/test/win because adding sources to a CppUnitTests binary doesn't seem to work.

In addition I had to make mozglue/linker link with mfbt, because linking to Assertions.cpp directly lead to redefined symbol errors for mozglue.

Otherwise I just added /mfbt/Assertions.cpp to the sources in various places as suggested. I didn't use UNIFIED_SOURCES because that lead to inconsistent dll linkage warnings on Windows (presumably from including Assertions.h earlier in the unified file).

Comment 19

[Emanuel Hoogeveen [:ehoogeveen]]

The try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3684e60b2383

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 8804655 [details] [diff] [review]
Part 1 v3: Define the crash reason in MFBT and let everything use it.

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

WFM!

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8804660 [details] [diff] [review]
Part 2 v2: Add MOZILLA_EXTERNAL_LINKAGE or link /mfbt/Assertions.cpp where necessary.

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

I got things building with less modifications when removing MOZILLA_EXTERNAL_LINKAGE entirely from mfbt/Assertions.h. But it's not very satisfactory either.

::: mozglue/linker/moz.build
@@ +25,5 @@
>  
>  FINAL_LIBRARY = 'mozglue'
>  
> +USE_LIBS += [
> +    'mfbt',

This can't be right.

Comment 22

[Mike Hommey [:glandium]]

This is less invasive and passed try:

diff --git a/build/gecko_templates.mozbuild b/build/gecko_templates.mozbuild
index aacb3fc..8eab9d3 100644
--- a/build/gecko_templates.mozbuild
+++ b/build/gecko_templates.mozbuild
@@ -58,12 +58,14 @@ def GeckoBinary(linkage='dependent', msvcrt='dynamic', mozglue=None):
         LDFLAGS += CONFIG['MOZ_GLUE_WRAP_LDFLAGS']
         if mozglue == 'program':
             USE_LIBS += ['mozglue']
+            DEFINES['MOZ_HAS_MOZGLUE'] = True
             if CONFIG['MOZ_GLUE_IN_PROGRAM']:
                 if CONFIG['GNU_CC']:
                     LDFLAGS += ['-rdynamic']
                 if CONFIG['MOZ_MEMORY']:
                     USE_LIBS += ['memory']
         elif mozglue == 'library':
+            LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True
             if not CONFIG['MOZ_GLUE_IN_PROGRAM']:
                 USE_LIBS += ['mozglue']
         else:
diff --git a/js/src/moz.build b/js/src/moz.build
index b92af8b1..567a488 100644
--- a/js/src/moz.build
+++ b/js/src/moz.build
@@ -68,6 +68,8 @@ CONFIGURE_DEFINE_FILES += [
 ]
 
 if not CONFIG['JS_STANDALONE']:
+    DEFINES['MOZ_HAS_MOZGLUE'] = True
+
     CONFIGURE_SUBST_FILES += [
         '../../config/autoconf-js.mk',
         '../../config/emptyvars-js.mk',
diff --git a/mfbt/Assertions.h b/mfbt/Assertions.h
index 81fdb6f..bc70819 100644
--- a/mfbt/Assertions.h
+++ b/mfbt/Assertions.h
@@ -23,7 +23,7 @@
 #include "nsTraceRefcnt.h"
 #endif
 
-#ifndef MOZILLA_EXTERNAL_LINKAGE
+#ifdef MOZ_HAS_MOZGLUE
 /*
  * The crash reason set by MOZ_CRASH_ANNOTATE is consumed by the crash reporter
  * if present. It is declared here (and defined in Assertions.cpp) to make it

Please check it does what you want.

Comment 23

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks for looking into it, I'll do some testing when I get home.

Comment 24

[Emanuel Hoogeveen [:ehoogeveen]]

Hmm, unfortunately making it opt-in like that excludes a lot of things (more things than the status quo). I checked by making MOZ_ASSERT call a different function depending on whether it's using the real MOZ_CRASH_ANNOTATE or the dummy version, and it hits all over the place trying to start the browser. I think every Library that libxul uses would need that define to make it work, which defeats the purpose.

(In reply to Mike Hommey [:glandium] from comment #21)
> ::: mozglue/linker/moz.build
> @@ +25,5 @@
> >  
> >  FINAL_LIBRARY = 'mozglue'
> >  
> > +USE_LIBS += [
> > +    'mfbt',
> 
> This can't be right.

It's entirely possible I'm missing something, but including Assertions.cpp didn't work, giving me redefined symbol errors for mozglue. This might be some Android-specific weirdness, I'm not sure. Making that directory MOZILLA_EXTERNAL_LINKAGE would probably work.

Comment 25

[Mike Hommey [:glandium]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #24)
> Hmm, unfortunately making it opt-in like that excludes a lot of things (more
> things than the status quo). I checked by making MOZ_ASSERT call a different
> function depending on whether it's using the real MOZ_CRASH_ANNOTATE or the
> dummy version, and it hits all over the place trying to start the browser. I
> think every Library that libxul uses would need that define to make it work,
> which defeats the purpose.

LIBRARY_DEFINES is supposed to propagate to those. Do you have an example where it's not set where it should be?

> (In reply to Mike Hommey [:glandium] from comment #21)
> > ::: mozglue/linker/moz.build
> > @@ +25,5 @@
> > >  
> > >  FINAL_LIBRARY = 'mozglue'
> > >  
> > > +USE_LIBS += [
> > > +    'mfbt',
> > 
> > This can't be right.
> 
> It's entirely possible I'm missing something, but including Assertions.cpp
> didn't work, giving me redefined symbol errors for mozglue. This might be
> some Android-specific weirdness, I'm not sure. Making that directory
> MOZILLA_EXTERNAL_LINKAGE would probably work.

There shouldn't be a need for any non HOST_* change in this moz.build.

Comment 26

[Mike Hommey [:glandium]]

Ah, the changes to gecko_templates.mozbuild are wrong.

Comment 27

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #26)
> Ah, the changes to gecko_templates.mozbuild are wrong.

Huh, no, they actually look fine, so the question in comment 25 still applies.

Comment 28

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Mike Hommey [:glandium] from comment #25)
> LIBRARY_DEFINES is supposed to propagate to those. Do you have an example
> where it's not set where it should be?

The number of places is huge - I'm not sure what the best example would be. Various files from dom and layout like layout/forms/nsProgressFrame.cpp or dom/base/nsXMLNameSpaceMap.cpp, files I definitely wouldn't expect to be external.

Comment 29

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2 v3 (WIP): Add MOZ_NO_LIBMFBT or link /mfbt/Assertions.cpp where necessary.

I noticed in my original patch that most of the places where I needed to add exceptions also contained DISABLE_STL_WRAPPING. Here's an alternative patch that makes DISABLE_STL_WRAPPING imply MOZ_NO_LIBMFBT (I didn't want to reuse MOZILLA_EXTERNAL_LINKAGE for this), and greatly reduces the number of individual places that need to be changed.

This probably isn't the right place to insert these defines, as the testcase adjustment indicate, but I couldn't figure out how to put them in [1] and get it to work - I couldn't get that file to print anything to indicate what I was doing wrong.

[1] https://dxr.mozilla.org/mozilla-central/rev/3bfde35a0d18a643485ffd5073f3bc6a79e0ae48/python/mozbuild/mozbuild/compilation/database.py#107

Comment 30

[Mike Hommey [:glandium]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #28)
> (In reply to Mike Hommey [:glandium] from comment #25)
> > LIBRARY_DEFINES is supposed to propagate to those. Do you have an example
> > where it's not set where it should be?
> 
> The number of places is huge - I'm not sure what the best example would be.
> Various files from dom and layout like layout/forms/nsProgressFrame.cpp or
> dom/base/nsXMLNameSpaceMap.cpp, files I definitely wouldn't expect to be
> external.

Are you sure you applied the patch from comment 22 entirely? Because with it applied, both $objdir/layout/forms/backend.mk and $objdir/dom/base/backend.mk contain
DEFINES += (...) -DMOZ_HAS_MOZGLUE (...)

So I fail to see how those files can be built without the crash reason.

Comment 31

[Emanuel Hoogeveen [:ehoogeveen]]

Sorry about the delay, I realized that the compile time check I switched to was probably producing a lot of false positives, but I'm having a hard time combining it with the earlier runtime analysis (if it deserves to be called that). I keep getting stack overflow crashes, and I'm running out of ideas for the cause. I'll let you know as soon as I know more.

Comment 32

[Emanuel Hoogeveen [:ehoogeveen]]

Ah, I *did* apply it wrong. Sorry, I did it by hand and totally missed the fact that the 2nd added line used LIBRARY_DEFINES. With the correct patch and a working runtime analysis, I only see one instance of a file that wouldn't have access to the crash reason (though the analysis relies on the presence of assertions, so it isn't comprehensive). Using compile time warnings there are definitely still some files that don't get the crash reason compared to the patch from comment #29, but I don't know if there's anything significant in there.

Comment 33

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 1 v3.1: Define the crash reason in MFBT to let everything use it.

Carrying forward r+. This just changes the condition to be opt-in instead of opt-out.

Comment 34

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2 v4: Define MOZ_HAS_MOZGLUE in various places so that the crash reason gets used.

Alright, I went through the list and I'm satisfied that this patch includes everything we care about. I had to add the MOZ_HAS_MOZGLUE define in a few more places, and had to add mfbt to the linker *tests* to satisfy Android (not the linker itself anymore), but hopefully this will be acceptable. With this patch, the libraries that still don't get the crash reason on Linux are:

 1:15.52 libmar.a.desc
 1:18.12 libmozgtk.so
 1:30.32 libmozgtk.so
11:22.45 libmozgtk_stub.so
63:14.20 libcubeb.a.desc
66:52.75 libwoff2.a.desc
91:54.22 libsignmar.a.desc
91:54.41 libverifymar.a.desc
91:54.57 signmar
92:45.28 libxpcomglue_s.a
93:29.63 libunicharutil_external_s.a
93:30.81 librdfutil_external_s.a
93:35.28 libhostmar.a
93:35.39 mar
93:52.95 libmtransport_s.a.desc

I think we don't care about those because they're either pretty irrelevant or standalone. I did add the define to libmemory and the two jemalloc implementations since jemalloc crashes are pretty common and stacks show a lot of inlining.

Comment 35

[Emanuel Hoogeveen [:ehoogeveen]]

This is green on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cac3eaa5c7cdf937c017e0654510d4d895a53b7c

Comment 36

[Emanuel Hoogeveen [:ehoogeveen]]

Oh, I wanted to add: the define for mozglue itself was justified by the runtime analysis - mozilla/TimeStamp.h wasn't getting it. The compile log also showed several other instances of files inside mfbt itself not getting the crash reason. The rest were judgment calls from eyeballing the compile log.

Comment 37

[Mike Hommey [:glandium]]

Comment on attachment 8808066 [details] [diff] [review]
Part 2 v4: Define MOZ_HAS_MOZGLUE in various places so that the crash reason gets used.

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

::: ipc/contentproc/moz.build
@@ +9,5 @@
>  SOURCES += [
>      'plugin-container.cpp',
>  ]
>  
> +DEFINES['MOZ_HAS_MOZGLUE'] = True

AFAICT, this file is useless. plugin-container.cpp is directly #included from where it's being used, and nothing has a USE_LIBS += ['plugin-container']

::: js/src/moz.build
@@ +67,5 @@
>      'js-confdefs.h',
>  ]
>  
>  if not CONFIG['JS_STANDALONE']:
> +    DEFINES['MOZ_HAS_MOZGLUE'] = True

using LIBRARY_DEFINES here should remove the need for it in fdlibm... as long as you remove the Library in config/external/fdlibm (which is misplaced and kind of useless), and change FINAL_LIBRARY in modules/fdlibm/moz.build to 'js'.

::: memory/build/moz.build
@@ +52,5 @@
>  
>  if CONFIG['MOZ_GLUE_IN_PROGRAM']:
>      SDK_LIBRARY = True
>      DIST_INSTALL = True
> +    DEFINES['MOZ_HAS_MOZGLUE'] = True

It shouldn't hurt to have it set unconditionally. Please also file a followup to turn that into a LIBRARY_DEFINES and make mozjemalloc/jemalloc4 use FINAL_LIBRARY = 'memory' (which may require some other adjustments, so I'm not asking to do it in this bug).

::: mozglue/build/moz.build
@@ +85,5 @@
>          SOURCES += [
>              'cpuacct.c',
>          ]
>  
> +    LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True

This shouldn't need to be conditional to "not JS_STANDALONE".

Comment 38

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2 v5: Define MOZ_HAS_MOZGLUE in various places so that the crash reason gets used.

Thanks! I think automation is busted right now, but this seems to work fine locally.

(In reply to Mike Hommey [:glandium] from comment #37)
> ::: ipc/contentproc/moz.build
> @@ +9,5 @@
> >  SOURCES += [
> >      'plugin-container.cpp',
> >  ]
> >  
> > +DEFINES['MOZ_HAS_MOZGLUE'] = True
> 
> AFAICT, this file is useless. plugin-container.cpp is directly #included
> from where it's being used, and nothing has a USE_LIBS +=
> ['plugin-container']

Alright, removed that change.

> ::: js/src/moz.build
> @@ +67,5 @@
> >      'js-confdefs.h',
> >  ]
> >  
> >  if not CONFIG['JS_STANDALONE']:
> > +    DEFINES['MOZ_HAS_MOZGLUE'] = True
> 
> using LIBRARY_DEFINES here should remove the need for it in fdlibm... as
> long as you remove the Library in config/external/fdlibm (which is misplaced
> and kind of useless), and change FINAL_LIBRARY in modules/fdlibm/moz.build
> to 'js'.

Done.

> ::: memory/build/moz.build
> @@ +52,5 @@
> >  
> >  if CONFIG['MOZ_GLUE_IN_PROGRAM']:
> >      SDK_LIBRARY = True
> >      DIST_INSTALL = True
> > +    DEFINES['MOZ_HAS_MOZGLUE'] = True
> 
> It shouldn't hurt to have it set unconditionally. Please also file a
> followup to turn that into a LIBRARY_DEFINES and make mozjemalloc/jemalloc4
> use FINAL_LIBRARY = 'memory' (which may require some other adjustments, so
> I'm not asking to do it in this bug).

OK, I made it unconditional, and I'll make sure to file that follow-up.

> ::: mozglue/build/moz.build
> @@ +85,5 @@
> >          SOURCES += [
> >              'cpuacct.c',
> >          ]
> >  
> > +    LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True
> 
> This shouldn't need to be conditional to "not JS_STANDALONE".

Done.

Comment 39

[Emanuel Hoogeveen [:ehoogeveen]]

Still fine on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72705f7db3c9a7c04f58701cf14b2ebb47e826c3

Comment 40

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks for the reviews and guidance! Glad to get this fixed.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f08b961830
Part 1: Define the crash reason in MFBT to let everything use it. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/151ddb5b85a1
Part 2: Define MOZ_HAS_MOZGLUE in various places so that the crash reason gets used. r=glandium

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5f08b961830
https://hg.mozilla.org/mozilla-central/rev/151ddb5b85a1