1235858 - Timestamp graphics critical error crash annotations

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

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.

Comment 1

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

Attachment: 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/

Comment 2

[Botond Ballo [:botond]]

(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 3

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

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/

Comment 4

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

(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 5

[Botond Ballo [:botond]]

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).

Comment 6

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

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.

Comment 7

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

Attachment: Record the time stamp, use it for crash reports. Carry r=botond

Addressed the review comments, except for the TimeDuration (include rules not allowing this.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed487b519aa

Comment 8

[Botond Ballo [:botond]]

(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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9115adb58910

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9115adb58910