1240160 - crash reports and TimeStamp

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: If we were just to add the TimeStamp based value into crash reports

Crash reports include a computed "uptime" value, based on the start and crash time(nullptr) system clock.  This is good, but it would be useful to (also) have TimeStamp related value, if for no other reason, than to match the value that crash report annotations from graphics (added in bug 1235858) use.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

Review commit: https://reviewboard.mozilla.org/r/31093/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31093/

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1240160: Add the TimeStamp based values to crash reports. r=ted

Review commit: https://reviewboard.mozilla.org/r/31095/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31095/

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1235437: Add annotation for major graphics preferences. r?bgirard

Review commit: https://reviewboard.mozilla.org/r/31097/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31097/

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Hmm, MozReview seems to have added a few patches I didn't meant to add.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

https://reviewboard.mozilla.org/r/31097/#review28041

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8708526 [details] [diff] [review]
If we were just to add the TimeStamp based value into crash reports

Review of attachment 8708526 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +799,5 @@
>        apiData.Open(extraDataPath);
>        apiData.WriteBuffer(crashReporterAPIData->get(), crashReporterAPIData->Length());
>      }
>      WriteAnnotation(apiData, "CrashTime", crashTimeString);
> +    WriteAnnotation(apiData, "CrashTimeStamp", crashTimeStampString);

This seems like a misnomer--CrashTime is a unix timestamp, but this is uptime in seconds, right?

I'd be OK with this if you just called it "Uptime", that seems less confusing.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Naming things is the hardest :)  "Uptime" is displayed in the crash reports already, computed as a difference between the CrashTime and StartupTime.  It doesn't match the process uptime we get from TimeStamp class.  So, we will end up with two, which is fine with me, just will need to name the new one differently.  Some options:

1. ProcessUptime
2. UptimeStamp
3. UptimeAtCrash
4. ProcessCrashTime
5. UptimeInSeconds
6. ...

Any preferences or other ideas?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm thinking UptimeTS.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Save TimeStamp based uptime in the crash reports. r=ted

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8718427 [details] [diff] [review]
Save TimeStamp based uptime in the crash reports. r=ted

Review of attachment 8718427 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +758,5 @@
> +  bool ignored = false;
> +  double uptimeTS = (TimeStamp::NowLoRes()-
> +                     TimeStamp::ProcessCreation(ignored)).ToSecondsSigDigits();
> +  char uptimeTSString[32];
> +  snprintf(uptimeTSString, sizeof(uptimeTSString)-1, "%f", uptimeTS);

OK, so, the one other annoying thing here is that you can't call snprintf in the exception handler callback, because it's not safe to call into the C library at this point. It looks like we have a few dtoa implementations in mfbt nowadays, one of those might be usable:
https://dxr.mozilla.org/mozilla-central/source/mfbt/double-conversion/fast-dtoa.h#79

Although they do take a Vector<char>, so we'd also have to be careful not to do heap allocation.

Comment 11

[(not currently active) Ted Mielczarek]

(sorry, I should have caught that the first time around)

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> ...
> OK, so, the one other annoying thing here is that you can't call snprintf in
> the exception handler callback, because it's not safe to call into the C
> library at this point.

Nice catch.  I was careful to avoid the memory allocation, but didn't know about the C library limitation.  I'll take a look at the example.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

So, I do have some code that deals with this, using mfbt/double-conversion/double-conversion.h, but wanted to point out that we're using sprintf in this code, through XP_TTOA macros just a few lines above this change.  Is that a problem?

Comment 14

[(not currently active) Ted Mielczarek]

That's used on Mac, where I believe it's OK. (The libc stuff is only a problem on Linux.)

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31093/diff/1-2/

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

https://reviewboard.mozilla.org/r/31093/#review31637

Thanks! Sorry that wound up being such a pain.

::: toolkit/crashreporter/nsExceptionHandler.cpp:422
(Diff revision 2)
> +#define UNIT_TEST_NOCLIBDTOA 1

Running unit tests on startup is a pretty weird thing. I like the motivation here, but maybe we can figure out how to make these gtests or something?

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31093/diff/2-3/

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31093/diff/3-4/

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31093/diff/4-5/

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

https://reviewboard.mozilla.org/r/31093/#review31931

It's fine, just nit picking about the code you're moving. It doesn't need to be fixed since it's existing code.

::: gfx/tests/gtest/TestGfxPrefs.cpp:92
(Diff revision 5)
> +                      86400 * 100000000.0 * 10000000000.0 * 10000000000.0 * 100.0,

I'm not sure what the point of these expression here are but we've just moving the code and it's not making it worse.

::: gfx/tests/gtest/TestGfxPrefs.cpp:94
(Diff revision 5)
> +  for (unsigned int i=0; i<sizeof(testVal)/sizeof(double); i++) {

This could use mozilla::ArrayLength

::: gfx/tests/gtest/TestGfxPrefs.cpp:95
(Diff revision 5)
> +    ASSERT_TRUE(SimpleNoCLibDtoA(testVal[i], testBuffer, sizeof(testBuffer)));

It would be better if compared against libc DtoA IMO rather than reverse the conversion from atof but this does the job and it's probably fine as-is.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Good point on the ArrayLength, I'll make that change.  Comparing the libc result is a good idea, I'm worried about running into the formatting issues and getting scientific notation, for instance.
That extra long thing was meant to give me a number with more than 32 digits so that we catch a particular failure - don't see Firefox running that long (10^30 number of days or something like that :)

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Benoit, any idea what GTest needs for this error in Linux ASAN to go way:

09:00:12     INFO -  /builds/slave/try-l64-asan-d-000000000000000/build/src/gfx/tests/gtest/TestGfxPrefs.cpp:95: undefined reference to `SimpleNoCLibDtoA(double, char*, int)'

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-a33cdf486c98cac38aa8cfe441cceae4b8ea1233/try-linux64-asan-debug/try-linux64-asan-debug-bm79-try1-build4230.txt.gz

It's fine in other Linux builds.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Never mind, asan build doesn't have crash reporter.

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31093/diff/5-6/

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ee67cdf537

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7ee67cdf537

Comment 27

[Liz Henry (:lizzard)]

Milan, do you want to request uplift here?  Looks like we may need it for bug 1257209. Thanks!

Comment 28

[Benjamin Smedberg]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

I really need this for bug 1257209, so requesting beta. I didn't realize this was so new.

Comment 29

[Benjamin Smedberg]

Milan's on PTO.

Comment 30

[Liz Henry (:lizzard)]

Comment on attachment 8708548 [details]
MozReview Request: Bug 1240160: Add the TimeStamp based uptime value to crash reports, tagging it as UptimeTS, to differentiate from an existing Uptime value. A bit of additional code, avoiding the usage of C libraries to format some strings and a way to

Improves crash reports, adds a test. Aiming this at beta 6.

Comment 31

[Benjamin Smedberg]

https://hg.mozilla.org/releases/mozilla-beta/rev/a88d785922fc

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> Milan's on PTO.

Thanks for getting this through, I was digging through the backlog of messages and needinfos and didn't get to this early enough.  Freaked out a bit when I saw the beta patch landing, not recalling ever asking for uplift :)