Closed Bug 1125929 Opened 5 years ago Closed 5 years ago

Remove JS_CRASH_DIAGNOSTICS

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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.
Attachment #8555929 - Flags: review?(wmccloskey)
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.
Attachment #8555929 - Flags: review?(wmccloskey) → review+
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." :-(
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.
Attachment #8556746 - Flags: review?(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!
Attachment #8556746 - Flags: review?(terrence) → review+
(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.
(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.
Let's move the jsobj.h changes to a different bug.
Here's a try run to see if that combination fixes the issue as expected:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d91143f6f8f
https://hg.mozilla.org/mozilla-central/rev/97e79fde7e22
https://hg.mozilla.org/mozilla-central/rev/822f56612d95
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Flags: needinfo?(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
Flags: needinfo?(terrence)
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.
You need to log in before you can comment on or make changes to this bug.