Closed
Bug 677773
Opened 13 years ago
Closed 13 years ago
Simplify JS crash diagnostics and disable for Aurora/beta/release
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: billm, Assigned: billm)
Details
(Whiteboard: [notacrash])
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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 2•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
This patch enables the configure switch in nightlies.
Attachment #552130 -
Flags: review?(bhearsum)
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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•13 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 8•13 years ago
|
||
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•13 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 :-).
Updated•13 years ago
|
Whiteboard: [notacrash]
Comment 10•13 years ago
|
||
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•13 years ago
|
Whiteboard: [notacrash] → [notacrash][inbound]
Status: NEW → RESOLVED
Closed: 13 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.
Description
•