1608158 - Consider including runnable name in crash reports

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

The name of the currently executing runnable on the main thread in each process is recorded in a static array, and it may be useful to include that information in generated crash reports.

Due to this information not being present in non-nightly builds (see https://searchfox.org/mozilla-central/rev/be7d1f2d52dd9474ca2df145190a817614c924e4/xpcom/threads/nsThreadUtils.h#444-449), this may not be a super useful probe to include in general, but writing a patch to add it was fairly straightforward.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1608158 - Include MainThreadRunnableName in crash reports,

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Request for data collection review form

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Holding off on formally requesting data-review until we determine whether it's worthwhile to collect this information.

Comment 4

[Andrew McCreight [:mccr8]]

Nathan, as somebody who probably looks at a lot of crashes with runnables in them, do you have any sense if this might be useful? My hope was that this information would help in those situations where things get really inlined.

I guess we could also look at some crash reports with runnables in the stack somewhere.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Request for data collection review form

Updated data-review form to reflect including this information in the "crash" ping.
See also: https://phabricator.services.mozilla.com/D59364?id=215794#inline-361681

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #4)

Nathan, as somebody who probably looks at a lot of crashes with runnables in them, do you have any sense if this might be useful? My hope was that this information would help in those situations where things get really inlined.

I guess we could also look at some crash reports with runnables in the stack somewhere.

My sense is that things can/will get inlined into the Run() method of the runnable on the stack, but Run() won't get inlined due to being a virtual method, so we should always be able to see Run() on the stack, shouldn't we?

That being said, there are two concerns that I think override that:

1. Sometimes we get absolute garbage for (part/all of the) stacks, and having information off to the side could help clarify things.
2. Sometimes many different runnables get their Run() method merged through code folding, and so the Run() you see in the stack is very much not what you would expect to see. Having the runnable name be available could help clarify things.

So I think we should go ahead and add this. Bonus points if some justification along those lines is added to the commit message.

Comment 7

[Chris H-C :chutten]

Comment on attachment 9120114 [details]
Request for data collection review form

Load-balancing to Megan.

Comment 8

[Chris H-C :chutten]

Comment on attachment 9120114 [details]
Request for data collection review form

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is documented [in the "crash" ping documentation](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/crash-ping.html).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Nika Layzell is responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default on for Nightly channel only.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+

Comment 9

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=93624f390bb7f536a0a07adcc977054faf42f094&selectedJob=285652015

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=285652015&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/a2e37e6c3ef6f40e27476400eec9d46fc65b0d89

[task 2020-01-20T17:00:11.512Z] 17:00:11 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -isysroot /builds/worker/workspace/build/src/MacOSX10.11.sdk -std=gnu++17 --target=x86_64-apple-darwin -o Unified_cpp_crashreporter0.o -c -fvisibility=hidden -fvisibility-inlines-hidden -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -ftrivial-auto-var-init=pattern -DDEBUG=1 -DOS_POSIX=1 -DOS_MACOSX=1 -DUNICODE -D_UNICODE -DNO_STABS_SUPPORT -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/toolkit/crashreporter -I/builds/worker/workspace/build/src/obj-firefox/toolkit/crashreporter -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/toolkit/crashreporter/google-breakpad/src -I/builds/worker/workspace/build/src/toolkit/crashreporter/breakpad-client -I/builds/worker/workspace/build/src/toolkit/crashreporter/google-breakpad/src -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -Wno-unused-local-typedefs -Wno-shadow -Wno-deprecated-declarations -Wno-bool-compare -Wno-unused-but-set-variable -Wno-c++11-narrowing -Wno-implicit-fallthrough -Wno-shadow -MD -MP -MF .deps/Unified_cpp_crashreporter0.o.pp Unified_cpp_crashreporter0.cpp
[task 2020-01-20T17:00:11.512Z] 17:00:11 INFO - In file included from Unified_cpp_crashreporter0.cpp:20:
[task 2020-01-20T17:00:11.512Z] 17:00:11 ERROR - /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp:1294:13: error: cannot initialize a variable of type 'void *' with an rvalue of type 'const void '
[task 2020-01-20T17:00:11.512Z] 17:00:11 INFO - if (void end = my_memchr(buf, '\0', len)) {
[task 2020-01-20T17:00:11.512Z] 17:00:11 INFO - ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2020-01-20T17:00:11.512Z] 17:00:11 INFO - 1 error generated.
[task 2020-01-20T17:00:11.512Z] 17:00:11 INFO - /builds/worker/workspace/build/src/config/rules.mk:744: recipe for target 'Unified_cpp_crashreporter0.o' failed
[task 2020-01-20T17:00:11.512Z] 17:00:11 ERROR - make[4]: *** [Unified_cpp_crashreporter0.o] Error 1
[task 2020-01-20T17:00:11.513Z] 17:00:11 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/crashreporter'
[task 2020-01-20T17:00:11.513Z] 17:00:11 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'toolkit/crashreporter/target-objects' failed
[task 2020-01-20T17:00:11.513Z] 17:00:11 ERROR - make[3]: *** [toolkit/crashreporter/target-objects] Error 2
[task 2020-01-20T17:00:11.513Z] 17:00:11 INFO - make[3]: *** Waiting for unfinished jobs....
[task 2020-01-20T17:00:11.513Z] 17:00:11 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/xre'
[task 2020-01-20T17:00:11.513Z] 17:00:11 INFO - toolkit/xre/nsAppRunner.o

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/69b085ba0b71