Closed Bug 1240160 Opened 4 years ago Closed 4 years ago

crash reports and TimeStamp

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: milan, Assigned: milan)

Details

(Keywords: feature)

Attachments

(1 file, 4 obsolete files)

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.
Attachment #8708526 - Attachment is patch: true
Attachment #8708526 - Flags: feedback?(ted)
Hmm, MozReview seems to have added a few patches I didn't meant to add.
Attachment #8708548 - Attachment is obsolete: true
Attachment #8708550 - Attachment is obsolete: true
Attachment #8708549 - Attachment is obsolete: true
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.
Attachment #8708526 - Flags: feedback?(ted)
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?
Flags: needinfo?(ted)
I'm thinking UptimeTS.
Assignee: nobody → milan
Flags: needinfo?(ted)
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.
Attachment #8718427 - Flags: review?(ted)
(sorry, I should have caught that the first time around)
(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.
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?
Flags: needinfo?(ted)
That's used on Mac, where I believe it's OK. (The libc stuff is only a problem on Linux.)
Flags: needinfo?(ted)
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/
Attachment #8708548 - Attachment description: MozReview Request: imported patch ShowPrefType → 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. r?ted
Attachment #8708548 - Attachment is obsolete: false
Attachment #8708548 - Flags: review?(ted)
Attachment #8718427 - Attachment is obsolete: true
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?
Attachment #8708548 - Flags: review?(ted) → review+
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/
Attachment #8708548 - Attachment description: 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. r?ted → 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. r=ted.mielcz…
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/
Attachment #8708548 - Attachment description: , avoiding the usage of C libraries to format some strings. r=ted.mielcz → , avoiding the usage of C libraries to format some strings. r=ted.mielczarek
Attachment #8708548 - Attachment description: , avoiding the usage of C libraries to format some strings. r=ted.mielcz → , avoiding the usage of C libraries to format some strings and a way to gtest these. r?benwa, while carrying ted.
Attachment #8708548 - Flags: review?(bgirard)
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 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.
Attachment #8708548 - Flags: review?(bgirard) → review+
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 :)
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/msreckovic@mozilla.com-a33cdf486c98cac38aa8cfe441cceae4b8ea1233/try-linux64-asan-debug/try-linux64-asan-debug-bm79-try1-build4230.txt.gz

It's fine in other Linux builds.
Flags: needinfo?(bgirard)
Never mind, asan build doesn't have crash reporter.
Flags: needinfo?(bgirard)
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/
Attachment #8708548 - Attachment description: , avoiding the usage of C libraries to format some strings and a way to → , avoiding the usage of C libraries to format some strings and a way to gtest these. r=benwa, also carrying ted.
https://hg.mozilla.org/mozilla-central/rev/b7ee67cdf537
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Flags: qe-verify-
Milan, do you want to request uplift here?  Looks like we may need it for bug 1257209. Thanks!
Flags: needinfo?(milan)
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.
Attachment #8708548 - Attachment description: 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 → 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
Attachment #8708548 - Flags: approval-mozilla-beta?
Milan's on PTO.
Flags: needinfo?(milan)
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.
Attachment #8708548 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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 :)
You need to log in before you can comment on or make changes to this bug.