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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: 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.

Comment 1

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

Attachment: 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?

Comment 2

[Benjamin Smedberg]

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.

Comment 3

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

Attachment: 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.

Comment 4

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

Attachment: mozconfig changes

This patch enables the configure switch in nightlies.

Comment 5

[[e-mail] (: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.

Comment 6

[[e-mail] (:bhearsum)]

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).

Comment 7

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

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 8

[David Mandelin [:dmandelin]]

Comment on attachment 551947 [details] [diff] [review]
diagnostics patch

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

crash::StackBuffer is cool.

Comment 9

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

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 :-).

Comment 10

[(not currently active) Ted Mielczarek]

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?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/c1eadd115562