1125929 - Remove JS_CRASH_DIAGNOSTICS

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

The original purpose was to get us the stack of the last GC when we crash in hopes that many of our crashes were caused by missing stack roots. This turned out to not be the case so much and I guess the stacks were ever that useful. Since then the flag has been adopted for everything from poisoning to compartment-mismatch checks. We should remove the stack capture and other less-than-useful bits and rename anything that remains to something more descriptive.

Comment 1

[Terrence Cole [:terrence]]

Attachment: remove_gc_crashstats-v0.diff

This preserves the poisoning aspects and compartment checking aspects of JS_CRASH_DIAGNOSTICS. I'm going to file a follow-up bug to split out the compartment checking meaning so that we can let it ride the trains separately.

Comment 2

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

Comment on attachment 8555929 [details] [diff] [review]
remove_gc_crashstats-v0.diff

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

::: js/src/jsgc.cpp
@@ +7229,5 @@
>      return rt->gc.isGenerationalGCEnabled();
>  }
> +
> +JS_PUBLIC_API(void)
> +JS_EnumerateDiagnosticMemoryRegions(JSEnumerateDiagnosticMemoryCallback callback)

Please instead remove the code in xpconnect that calls this.

Comment 3

[Terrence Cole [:terrence]]

Build failures in the try run are know tip bustage from yesterday:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=155c5d327654
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ac06e9143d

Comment 4

[Terrence Cole [:terrence]]

And backed out for breaking win32:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5969eb0fe8b5

This patch only removes code and the build error is in the windows sdk. This should be "fun." :-(

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Fix identifier conflict between dtoa.c and windows.h

If dtoa.c is included before windows.h, the macro "Bias" will break compiling _TIME_ZONE_INFORMATION. This bug removed jscrashreport.cpp which included windows.h, so it changed the include order in the compile unit of the unified build.
I also fixed another unified build mine while I'm here.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 8556746 [details] [diff] [review]
Fix identifier conflict between dtoa.c and windows.h

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

Thank you!

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> Created attachment 8556746 [details] [diff] [review]
> Fix identifier conflict between dtoa.c and windows.h
> 
> If dtoa.c is included before windows.h, the macro "Bias" will break
> compiling _TIME_ZONE_INFORMATION. This bug removed jscrashreport.cpp which
> included windows.h, so it changed the include order in the compile unit of
> the unified build.
> I also fixed another unified build mine while I'm here.

That makes sense, but why is the #include <windows.h> needed in jsobj.cpp? If it is indeed needed, then it should probably be done via #include "jswin.h" instead.

It looks like it was inherited from jscrashreport.cpp. It made more sense there, since that file contained a bunch of Windows-specific code, but it probably wasn't correct either.

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to Steve Fink [:sfink, :s:] from comment #7)
> That makes sense, but why is the #include <windows.h> needed in jsobj.cpp?
> If it is indeed needed, then it should probably be done via #include
> "jswin.h" instead.
> 
> It looks like it was inherited from jscrashreport.cpp. It made more sense
> there, since that file contained a bunch of Windows-specific code, but it
> probably wasn't correct either.

The jsobj.cpp change is actually unrelated to the build error atm. I found jsobj.cpp coundn't build alone when I was investigating the build error, so I piggybacked the jsobj.cpp change to prevent unified build boundary change from breaking jsobj.cpp in the future. Feel free to drop it from the patch if it should be handled in a new bug.

Comment 9

[Terrence Cole [:terrence]]

Let's move the jsobj.h changes to a different bug.

Comment 10

[Terrence Cole [:terrence]]

Here's a try run to see if that combination fixes the issue as expected:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d91143f6f8f

Comment 11

[Terrence Cole [:terrence]]

Looks like yes:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/97e79fde7e22
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/822f56612d95

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97e79fde7e22
https://hg.mozilla.org/mozilla-central/rev/822f56612d95

Comment 13

[Andrew McCreight [:mccr8]]

Is it just me, or did the configure.in changes here disable Nightly compartment checking? I haven't seen any crashes for them on crash-stats in quite a while.

Comment 14

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> Is it just me, or did the configure.in changes here disable Nightly
> compartment checking? I haven't seen any crashes for them on crash-stats in
> quite a while.

I don't think so? JS_CRASH_DIAGNOSTICS is only used in SpiderMonkey internal files, so we should only need the jsdiagnostics configury in the js configure given that any args passed to configure get passed to the sub-configure.

But don't take my word for it:

╰> objdump -t _cdD.nightly/dist/bin/libxul.so | grep CompartmentChecker
0000000003af6af0 l     F .text	0000000000000064              _ZN2js18CompartmentChecker4failEPN2JS4ZoneES3_
0000000003e4c1f0 l     F .text	00000000000000c8              _ZN2js18CompartmentChecker5checkEN2JS6HandleINS1_18PropertyDescriptorEEE
0000000003e453c0 l     F .text	0000000000000081              _ZN2js18CompartmentChecker5checkENS_16AbstractFramePtrE
0000000003aa8510 l     F .text	0000000000000064              _ZN2js18CompartmentChecker4failEP13JSCompartmentS2_
0000000003ebe050 l     F .text	0000000000000088              _ZN2js18CompartmentChecker5checkIP8JSStringEEvN2JS6HandleIT_EE
0000000003aa8410 l     F .text	00000000000000fb              _ZN2js18CompartmentCheckerC2EPNS_16ExclusiveContextE
0000000003af69b0 l     F .text	000000000000013a              _ZN2js18CompartmentChecker5checkERKN2JS5ValueE
0000000003e4c2c0 l     F .text	0000000000000088              _ZN2js18CompartmentChecker5checkEP8JSString

Comment 15

[Andrew McCreight [:mccr8]]

Thanks for checking. I'll have to do some more digging to try to figure out why there are no compartment check failures on Nightly.