376721 - send time-since-last-crash with crash report

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

Crash reporter should send time-since-last-crash along with the crash report.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: send SecondsSinceLastCrash and CrashTime

This patch sends a SecondsSinceLastCrash field, as well as a CrashTime field (with the current time when the crash happens, as a time_t).  I still need to test this on Linux/OS X, but the platform-specific parts are pretty minor.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 272657 [details] [diff] [review]
send SecondsSinceLastCrash and CrashTime

Mark, can you take a look at this?  I added a bit of code to our exception handler callback, and I'd like to make sure I'm not doing anything unsafe there.

Comment 3

[Mark Mentovai]

Comment on attachment 272657 [details] [diff] [review]
send SecondsSinceLastCrash and CrashTime

+  HANDLE hFile = CreateFile(lastCrashTimeFilename, GENERIC_WRITE, 0,


lastCrashTimeFilename may not be initialized yet here, because SetupExtraData is called after the exception handler is installed.

+    DWORD nBytes;
+    char buf[36];
+    sprintf(buf, "%ld\n", crashTime);

First, a general principle: share buf and this sprintf (and strlen - or use the return value of sprintf?) between the Win32 and POSIX cases?

Second, a practical matter: on Windows at least, sprintf will hit the CRT and can cause an allocation attempt, and the malloc lock might be held.

+    WriteFile(hFile, buf, strlen(buf), &nBytes, NULL);

Do you want to CloseHandle here?

+                0666);

Didn't we decide that we should use 0644 or 0600?


+      sprintf(buf, "CrashTime=%ld\n", crashTime);


sprintf warning again, and again for SecondsSinceLastCrash.

+    lastCrashTime = (time_t)atol(data.get());

And if the file is messed up, atol returns 0, and you'll get a really high value for timeSinceLastCrash.  You probably want lastCrashTime to be -1 in this case, or you want to use 0 as the "undefined" value instead of -1.


+  // It's ok to destructively modify aAppDataDirectory at this point.

If you do this, it's got a high potential to become the cause of future bugs, as does the existing Append of "Crash Reports".

(Should "Crash Reports" have the space eliminated?  You took care not to use spaces in other filenames.)

+  wcsncpy(lastCrashTimeFilename, filename.get(), filename.Length());

Check that filename.Length() compares favorably to XP_PATH_MAX here and in the non-Win32 strncpy case.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: updated with review comments

(In reply to comment #3)
> lastCrashTimeFilename may not be initialized yet here, because SetupExtraData
> is called after the exception handler is installed.

Initialized it to {0}, and added a check for it.

> First, a general principle: share buf and this sprintf (and strlen - or use the
> return value of sprintf?) between the Win32 and POSIX cases?

Moved out of those blocks and shared.
 
> Second, a practical matter: on Windows at least, sprintf will hit the CRT and
> can cause an allocation attempt, and the malloc lock might be held.

Switched to ltoa / _i64toa on Windows, still using sprintf on other OSes.

> Do you want to CloseHandle here?

Fixed, thanks.
 
> Didn't we decide that we should use 0644 or 0600?

Changed to 0600.

> And if the file is messed up, atol returns 0, and you'll get a really high
> value for timeSinceLastCrash.  You probably want lastCrashTime to be -1 in this
> case, or you want to use 0 as the "undefined" value instead of -1.

Changed to use 0 as the uninitialized value.

 
> +  // It's ok to destructively modify aAppDataDirectory at this point.
> 
> If you do this, it's got a high potential to become the cause of future bugs,
> as does the existing Append of "Crash Reports".

Added an additional clone at the top of the function, and another clone at this point.
 
> (Should "Crash Reports" have the space eliminated?  You took care not to use
> spaces in other filenames.)

I left this as-is because we've been using it for a while, and although we haven't released anything but alphas with this code in, I'm loathe to change it just to make it look nicer at this point.  You're right that it's sort of inconsistent.

> Check that filename.Length() compares favorably to XP_PATH_MAX here and in the
> non-Win32 strncpy case.

Added a check to both cases.

Comment 5

[Mark Mentovai]

Comment on attachment 273650 [details] [diff] [review]
updated with review comments

+#define XP_TTOA(time, buffer, base) _i64toa(time, buffer, base)

_ui64toa?

+#define XP_TTOA(time, buffer, base) ltoa(time, buffer, base)

What's this do with input that has the high bit set?

+  rv = dataDirectory->Append(NS_LITERAL_STRING("Crash Reports"));

I'd give this more consideration - it's almost at the now-or-never point, right?

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to comment #5)
> (From update of attachment 273650 [details] [diff] [review])
> +#define XP_TTOA(time, buffer, base) _i64toa(time, buffer, base)
> 
> _ui64toa?
> 
> +#define XP_TTOA(time, buffer, base) ltoa(time, buffer, base)
> 
> What's this do with input that has the high bit set?

time_t values are defined as signed types.  I don't know why, but that's just what it is.

> +  rv = dataDirectory->Append(NS_LITERAL_STRING("Crash Reports"));
> 
> I'd give this more consideration - it's almost at the now-or-never point,
> right?

I think I'm happy with it.  It only really looks out of place on Linux, and I think anyone running Linux can deal with it.

Comment 7

[Mark Mentovai]

OK, then, I'm satisfied.

Comment 8

[Mark Mentovai]

Ted, food for future thought: this data's being stored in the profile, where it'll be shared between any version of an application that happens to use the profile.  Should time-since-last-crash be made version-specific somehow?

Comment 9

[timeless]

it definitely shouldn't be stored in the profile.

i crash a browser version 5 times one w/ each profile, then i use and crash a different version w/ those profiles.

Comment 10

[(not currently active) Ted Mielczarek]

It's not stored in the profile, but it is shared by any version of the app.  It might make sense to change this to differentiate by Build ID.

Comment 11

[(not currently active) Ted Mielczarek]

Checked in.