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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Note in minidumps when we generate them by RUNTIMEABORT()ing

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

See bug 628152 for an example of where having this note is quite important.  This patch is not risky IMHO.

Comment 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/9521cf441d3a

Comment 8

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

I'm all for breaking non-libxul, but we should probably make this libxul only until after 2.0.

Comment 9

[Reed Loden [:reed]]

(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?