Closed Bug 628885 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: cjones, Assigned: cjones)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
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: --- → ?
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?
blocking2.0: ? → -
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
Closed: 11 years ago
Resolution: --- → FIXED
I'm all for breaking non-libxul, but we should probably make this libxul only until after 2.0.
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?
You need to log in before you can comment on or make changes to this bug.