Simplify JS crash diagnostics and disable for Aurora/beta/release

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla8
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [notacrash])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 551947 [details] [diff] [review]
diagnostics patch

Since there's going to be a merge soon, it's time to get the instrumentation to a point where it doesn't affect anything outside nightlies. Backing it out is the obvious thing to do, but some of the problems are still unsolved. I see no reason to back it out and then land it again right after the merge. Especially since Sheila has talked about possibly enabling it in Aurora or beta.

Instead, this patch disables everything based on the JS_CRASH_DIAGNOSTICS macro. It also removes some of the instrumentation that is no longer useful and simplifies some other stuff. There's now a fairly easy way to save stuff on the stack without writing your own memcpy. I also removed the OOM stack snapshotting: since the OOM stuff was called from outside JS, this eliminated the need for a few things to be in the public API.

The question of how to set the JS_CRASH_DIAGNOSTICS macro is a bit tricky, so I'm putting it in a separate patch.
Attachment #551947 - Flags: review?(dmandelin)
(Assignee)

Comment 1

6 years ago
Created attachment 551952 [details] [diff] [review]
patch to set JS_CRASH_DIAGNOSTICS

This patch sets JS_CRASH_DIAGNOSTICS in the obvious way.

I realize that this is not what you recommended in bug 675668 comment 2. However, I'm unsure of what the process would be for doing it that way. I could easily add some kind of --disable-js-diagnostics switch. But who do I talk to in releng about using it? Do you know what mozconfig is used for building releases? Could I (or someone else) land a patch to add the switch to it?
Attachment #551952 - Flags: review?(benjamin)
Comment on attachment 551952 [details] [diff] [review]
patch to set JS_CRASH_DIAGNOSTICS

http://hg.mozilla.org/build/buildbot-configs/file/5184ab5a2d19/mozilla2/win32/mozilla-aurora/nightly/mozconfig and other mozconfigs in that repo. And at the very least the JS_CRASH_DIAGNOSTICS define should be set in configure and have its own flag so that anyone can build with it.
Attachment #551952 - Flags: review?(benjamin) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 552129 [details] [diff] [review]
revised patch

This patch adds a configure switch for some JS diagnostics. This switch enables some code in opt builds that makes it easier to track down problems based on minidumps. However, it adds some performance overhead. We want it to be enabled in nightlies, but not in Aurora or anything after.
Attachment #551952 - Attachment is obsolete: true
Attachment #552129 - Flags: review?(bhearsum)
(Assignee)

Comment 4

6 years ago
Created attachment 552130 [details] [diff] [review]
mozconfig changes

This patch enables the configure switch in nightlies.
Attachment #552130 - Flags: review?(bhearsum)
Comment on attachment 552129 [details] [diff] [review]
revised patch

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

I'm definitely not the right reviewer for *this* patch, Ted might be.
Attachment #552129 - Flags: review?(bhearsum) → review?(ted.mielczarek)
Comment on attachment 552130 [details] [diff] [review]
mozconfig changes

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

Looks fine to me. Feel free to land this directly in buildbot-configs when you're ready but note that it will not take effect until the next time we pull changes into production (usually on Tuesdays & Thursdays).
Attachment #552130 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/build/buildbot-configs/rev/aed722586378

I wanted to get this in by Thursday so that we don't lose diagnostics on nightly when the other patches get merged.
Comment on attachment 551947 [details] [diff] [review]
diagnostics patch

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

crash::StackBuffer is cool.
Attachment #551947 - Flags: review?(dmandelin) → review+
(Assignee)

Comment 9

6 years ago
Ted, I'd appreciate a quick review. I'll be on vacation next week, and I really don't want to miss the merge. Also it's a very short patch :-).
Whiteboard: [notacrash]
Comment on attachment 552129 [details] [diff] [review]
revised patch

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

::: configure.in
@@ +7630,5 @@
>  
> +dnl ========================================================
> +dnl JS opt-mode assertions and minidump instrumentation
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(js-diagnostics,

Do you want to name this "--enable-js-crash-diagnostics" to better match exactly what it does?
Attachment #552129 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [notacrash] → [notacrash][inbound]
http://hg.mozilla.org/mozilla-central/rev/c1eadd115562
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [notacrash][inbound] → [notacrash]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.