Closed
Bug 1240160
Opened 7 years ago
Closed 7 years ago
crash reports and TimeStamp
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: milan, Assigned: milan)
Details
(Keywords: feature)
Attachments
(1 file, 4 obsolete files)
58 bytes,
text/x-review-board-request
|
ted
:
review+
BenWa
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8708526 -
Attachment is patch: true
Attachment #8708526 -
Flags: feedback?(ted)
Assignee | ||
Comment 1•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31093/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31093/
Assignee | ||
Comment 2•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31095/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31095/
Assignee | ||
Comment 3•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31097/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31097/
Assignee | ||
Comment 4•7 years ago
|
||
Hmm, MozReview seems to have added a few patches I didn't meant to add.
Assignee | ||
Updated•7 years ago
|
Attachment #8708548 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8708550 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8708549 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
https://reviewboard.mozilla.org/r/31097/#review28041
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
I'm thinking UptimeTS.
Assignee: nobody → milan
Flags: needinfo?(ted)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8708526 -
Attachment is obsolete: true
Attachment #8718427 -
Flags: review?(ted)
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
(sorry, I should have caught that the first time around)
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Assignee | ||
Comment 13•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
That's used on Mac, where I believe it's OK. (The libc stuff is only a problem on Linux.)
Flags: needinfo?(ted)
Assignee | ||
Comment 15•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8718427 -
Attachment is obsolete: true
Comment 16•7 years ago
|
||
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+
Assignee | ||
Comment 17•7 years ago
|
||
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…
Assignee | ||
Comment 18•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
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)
Assignee | ||
Comment 19•7 years ago
|
||
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•7 years ago
|
||
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+
Assignee | ||
Comment 21•7 years ago
|
||
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 :)
Assignee | ||
Comment 22•7 years ago
|
||
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)
Assignee | ||
Comment 23•7 years ago
|
||
Never mind, asan build doesn't have crash reporter.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 24•7 years ago
|
||
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.
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7ee67cdf537
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•7 years ago
|
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 28•7 years ago
|
||
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?
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+
Assignee | ||
Comment 32•7 years ago
|
||
(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.
Description
•