Closed Bug 1235858 Opened 4 years ago Closed 4 years ago

Timestamp graphics critical error crash annotations

Categories

(Core :: Graphics, defect)

43 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: milan, Assigned: milan)

Details

(Keywords: feature)

Attachments

(1 file, 1 obsolete file)

We get a lot of useful information from the crash report annotations, but it would be even more useful if we could tell "when" some of these happened, in relationship to the time when we crashed.
Assignee: nobody → milan
(In reply to Milan Sreckovic [:milan] from comment #1)
> Created attachment 8702949 [details]
> MozReview Request: Bug 1235858: Record the time stamp, use it for crash
> reports. r?botond
> 
> Review commit: https://reviewboard.mozilla.org/r/29229/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/29229/

The changes in this patch don't match the description (they are changes to image/Downscaler.cpp). Wrong patch?
Comment on attachment 8702949 [details]
MozReview Request: Bug 1235858: Record the time stamp, use it for crash reports. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29229/diff/1-2/
(In reply to Botond Ballo [:botond] from comment #2)
> 
> The changes in this patch don't match the description (they are changes to
> image/Downscaler.cpp). Wrong patch?

Extremely wrong.
Comment on attachment 8702949 [details]
MozReview Request: Bug 1235858: Record the time stamp, use it for crash reports. r?botond

https://reviewboard.mozilla.org/r/29229/#review26031

::: gfx/2d/Logging.h:226
(Diff revision 2)
> +typedef mozilla::Tuple<int32_t,std::string,double> LoggingRecordEntry;

I would prefer making the type of the third element TimeDuration, which is what

(TimeStamp::NowLoRes()-TimeStamp::ProcessCreation(ignored))

gives you. UpdateCrashReport() can then call ToSecondsSigDigits() on it.

This way, the interpretation (duration vs. stamp) is clearer, and knowing what units it's stored in (seconds) becomes a non-issue.

::: gfx/2d/Logging.h:237
(Diff revision 2)
> -  // ones we did save.
> +  // ones we did save.  double is the timestamp in seconds.

I think "time since process creation" is a more accurate description than "timestamp".

::: gfx/2d/Logging.h:238
(Diff revision 2)
> -  virtual std::vector<std::pair<int32_t,std::string> > StringsVectorCopy() = 0;
> +  virtual LoggingRecord StringsVectorCopy() = 0;

Suggestion: rename this to LoggingRecordCopy() (and UpdateStringsVector() to UpdateLoggingRecord())

::: gfx/thebes/gfxPlatform.cpp:199
(Diff revision 2)
> -  bool UpdateStringsVector(const std::string& aString);
> +  bool UpdateStringsVector(const std::string& aString, bool aStampTime);

Does anyone pass false?

::: gfx/thebes/gfxPlatform.cpp:428
(Diff revision 2)
> +#include "mozilla/TimeStamp.h"

Move the include up top (especially as the use of TimeStamp is above here).
Attachment #8702949 - Flags: review?(botond) → review+
I'll take care of these, thanks for the quick review.  I will have to "ignore" the part about TimeDuration instead of double, because TimeStamp.h is not accessible by Moz2D files, so I can't use it in Logging.h.  I'll add comments to try and clarify this situation.
Addressed the review comments, except for the TimeDuration (include rules not allowing this.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed487b519aa
Attachment #8702949 - Attachment is obsolete: true
Attachment #8702972 - Flags: review+
(In reply to Milan Sreckovic [:milan] from comment #6)
> I'll take care of these, thanks for the quick review.  I will have to
> "ignore" the part about TimeDuration instead of double, because TimeStamp.h
> is not accessible by Moz2D files, so I can't use it in Logging.h.

Ah, sorry, I overlooked that.
https://hg.mozilla.org/mozilla-central/rev/9115adb58910
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.