Last Comment Bug 677773 - Simplify JS crash diagnostics and disable for Aurora/beta/release
: Simplify JS crash diagnostics and disable for Aurora/beta/release
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla8
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-08-09 17:03 PDT by Bill McCloskey (:billm)
Modified: 2011-08-14 04:52 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

diagnostics patch (24.46 KB, patch)
2011-08-09 17:03 PDT, Bill McCloskey (:billm)
dmandelin: review+
Details | Diff | Splinter Review
patch to set JS_CRASH_DIAGNOSTICS (2.30 KB, patch)
2011-08-09 17:10 PDT, Bill McCloskey (:billm)
benjamin: review-
Details | Diff | Splinter Review
revised patch (2.25 KB, patch)
2011-08-10 11:15 PDT, Bill McCloskey (:billm)
ted: review+
Details | Diff | Splinter Review
mozconfig changes (9.09 KB, patch)
2011-08-10 11:16 PDT, Bill McCloskey (:billm)
bhearsum: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-08-09 17:03:25 PDT
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.
Comment 1 Bill McCloskey (:billm) 2011-08-09 17:10:44 PDT
Created attachment 551952 [details] [diff] [review]

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 [:bsmedberg] 2011-08-10 07:10:42 PDT
Comment on attachment 551952 [details] [diff] [review]
patch to set JS_CRASH_DIAGNOSTICS 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 (:billm) 2011-08-10 11:15:52 PDT
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.
Comment 4 Bill McCloskey (:billm) 2011-08-10 11:16:34 PDT
Created attachment 552130 [details] [diff] [review]
mozconfig changes

This patch enables the configure switch in nightlies.
Comment 5 Ben Hearsum (:bhearsum) 2011-08-10 12:35:20 PDT
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 Ben Hearsum (:bhearsum) 2011-08-10 12:41:43 PDT
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 (:billm) 2011-08-10 16:28:12 PDT

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] 2011-08-10 17:04:02 PDT
Comment on attachment 551947 [details] [diff] [review]
diagnostics patch

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

crash::StackBuffer is cool.
Comment 9 Bill McCloskey (:billm) 2011-08-10 17:13:21 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-08-11 11:22:04 PDT
Comment on attachment 552129 [details] [diff] [review]
revised patch

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

@@ +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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-14 04:52:20 PDT

Note You need to log in before you can comment on or make changes to this bug.