Add a crashreporter annotation when we're about to intentionally abort in nsDebugImpl (e.g. for RUNTIMEABORT())

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

(Depends on: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment)

In bug 628152 it's not possible to tell whether we're aborting from OOM or from RUNTIMEABORT, because of inlining and pgo and so forth.  It'd be better instead to get a minidump annotation when we abort.  Then we also can annotate with the abort message too.  This is safe because we don't RUNTIMEABORT() in OOM.
Created attachment 507015 [details] [diff] [review]
Note in minidumps when we generate them by RUNTIMEABORT()ing
Assignee: nobody → jones.chris.g
Attachment #507015 - Flags: review?(ted.mielczarek)
See bug 628152 for an example of where having this note is quite important.  This patch is not risky IMHO.
blocking2.0: --- → ?

Updated

8 years ago
Blocks: 628152
Comment on attachment 507015 [details] [diff] [review]
Note in minidumps when we generate them by RUNTIMEABORT()ing

Seems reasonable. Ideally we'd expose some Breakpad API to just write a non-crash minidump and put this in the assertion field, but this should suffice for now.
Attachment #507015 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 507015 [details] [diff] [review]
Note in minidumps when we generate them by RUNTIMEABORT()ing

This is a safe patch that gets more information into minidumps.
Attachment #507015 - Flags: approval2.0?

Updated

8 years ago
blocking2.0: ? → -

Updated

8 years ago
Attachment #507015 - Flags: approval2.0? → approval2.0+
While tracking down a tinderbox error, I noticed that we don't fsync() the .extra file after writing it.  I thought that might have been the problem with my patch, so I patched that.  It wasn't, but interested nonetheless?
I found that on mac 10.5 opt, nsTraceRefcntImpl::WalkTheStack() was crashing.  This is pretty much indistinguishable from a mozalloc_abort() on mac, except that we were crashing before the app-note code ran.  So I just moved that code to here

-   case NS_DEBUG_ABORT:
+   case NS_DEBUG_ABORT: {
+#ifdef MOZ_CRASHREPORTER
+     nsCString note("xpcom_runtime_abort(");
+     note += buf.buffer;
+     note += ")";
+     CrashReporter::AppendAppNotesToCrashReport(note);
+#endif  // MOZ_CRASHREPORTER
+
 #if defined(DEBUG) && defined(_WIN32)
      RealBreak();
 #endif
      nsTraceRefcntImpl::WalkTheStack(stderr);
      Abort(buf.buffer);
      return;
    }
+   }

I'm assuming that that's a trivial enough change that re-review isn't necessary.
http://hg.mozilla.org/mozilla-central/rev/9521cf441d3a
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
I'm all for breaking non-libxul, but we should probably make this libxul only until after 2.0.

Updated

8 years ago
Depends on: 631156
(In reply to comment #5)
> While tracking down a tinderbox error, I noticed that we don't fsync() the
> .extra file after writing it.  I thought that might have been the problem with
> my patch, so I patched that.  It wasn't, but interested nonetheless?

File a separate bug?
Depends on: 636081
You need to log in before you can comment on or make changes to this bug.