994707 - Record in FHR the submission rate for application crashes

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, enhancement)

Description

[Benjamin Smedberg]

Now that we are recording reliable crash counts in FHR (bug 875562 and bug 983313) we need to finish the statistics and record how many of those crashes users are submitting via the crash reporter.

This bug will track the submission rate for application crashes. I will file a separate bug to track the submission rate for plugin/content crashes, because those go through a separate submission UI.

This will allow us to prompt users who have crashed a lot but not submitted crashes to do so after the fact which will allow us to provide them with automated crash support. It will also allow us to know the usefulness of our crash-stats statistics.

Comment 1

[Steven MacLeod [:smacleod]]

I plan to introduce a new crash event for the log when a crash is submitted. We can count these and send them to FHR in a similar way to the count of main crash events.

So far I plan to include the "local" crash ID, the "remote" crash ID (coming back from crash-stats.mozilla.com). Is there anything else we should include with these events?

Comment 2

[Benjamin Smedberg]

Looking at the code, it would be nice to record whether the upload succeeded.

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: Part 1 - Provide crash events path as an env var for the crashreporter

In order to allow the Crash Reporter to generate crash events, it needs to know what directory to write them to.

This first small patch sets a new environment variable MOZ_CRASHREPORTER_EVENTS_DIRECTORY to tell the crash reporter where to write the new submission events it will generate. This follows the convention of the MOZ_CRASHREPORTER_DATA_DIRECTORY env var.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8416033 [details] [diff] [review]
Part 1 - Provide crash events path as an env var for the crashreporter

I'm a little worried about communicating these paths to the crash reporter using the environment: for other paths we use command-line args http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#723

Other than it's a little more painful, is there any reason that wouldn't work?

Bumping review to Ted in any case.

Comment 5

[Steven MacLeod [:smacleod]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 8416033 [details] [diff] [review]
> Part 1 - Provide crash events path as an env var for the crashreporter
> 
> I'm a little worried about communicating these paths to the crash reporter
> using the environment: for other paths we use command-line args
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/
> nsExceptionHandler.cpp#723

What worries do you have?

> Other than it's a little more painful, is there any reason that wouldn't
> work?

That would probably work as well. I went with the env var because it seemed simple to implement on both ends (I'm unfamiliar with all of this code), and we were already doing it for MOZ_CRASHREPORTER_DATA_DIRECTORY. If passing as an argument is going to be a lot better or more rubust let me know and I can work on switching it.

Comment 6

[Benjamin Smedberg]

In the past when we've communicated between processes using environment variables, we've ended up leaking those variables into the restarted Firefox and/or other applications launched by Firefox, including Thunderbird. We had to be very careful to re-set the envvars at the appropriate time.

It seems less important in this case, but I'll let Ted make the call.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8416033 [details] [diff] [review]
Part 1 - Provide crash events path as an env var for the crashreporter

Review of attachment 8416033 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine, it's no worse than what we already do for MOZ_CRASHREPORTER_STRINGS_OVERRIDE and MOZ_CRASHREPORTER_DATA_DIRECTORY.

Comment 8

[Marco Mucci [:MarcoM]]

Removed from Iteration 32.3 for selection as new work for the upcoming Iteration 33.1.

Comment 9

[Steven MacLeod [:smacleod]]

Attachment: Part 1 - Provide crash events path as an env var for the crashreporter

Un-bit-rot the env var patch.

Comment 10

[Steven MacLeod [:smacleod]]

Attachment: Part 3 - Write Crash Submission events

Crash submission events will be written to the path provided by the environment variable.

These crash events will provide the local crash id, the remote id, and whether the submission succeeded.

I've put up patch 3 before 2 (Which provides the documentation for the event, and handles reading it and putting the data in FHR) because Bug 983313 has totally ungeneralized the way crash events are handled. Before https://hg.mozilla.org/mozilla-central/rev/a6718c6dcdc9 landed crash events were handle as a generic event that could represent data about crashes and not just crashes themselves.

These events are now strictly a "crash type" for a "process type", which submissions don't really fit into. We'll either need to revert back to the more generalized handling, or represent these submissions in a different way (I would prefer to switch back to the general handling).

Benjamin, should I just go ahead and take the previous approach to handling as part of this patch?

Comment 11

[Benjamin Smedberg]

Comment on attachment 8433848 [details] [diff] [review]
Part 3 - Write Crash Submission events

>diff --git a/toolkit/crashreporter/client/crashreporter.cpp b/toolkit/crashreporter/client/crashreporter.cpp
>+static string GetDumpLocalID() {

style nit, opening brace of function in column 0

>+  string localId(gReporterDumpFile);
>+
>+  int namePos = localId.rfind(UI_DIR_SEPARATOR) + 1;
>+  int dot = localId.rfind('.');
>+  if (namePos < 0 || dot < 0)
>+    return "";
>+
>+  return localId.substr(namePos, dot - namePos);
>+}

I wonder if it would be better to reuse the existing Basename function for part of this.

Comment 12

[Benjamin Smedberg]

I don't really understand the question about event handling.

Comment 13

[Steven MacLeod [:smacleod]]

Attachment: Part 2 - Define Crash Submission events and handle them.

The method this patch takes is a bit hacky since it treats submissions as a type of crash in the crash manager. They are represented by crashes of process type "submission" with the sub types of "succeeded" or "failed".

If we want to represent the submissions separately it will be a lot more involved and we'd need to change the public API of the crash manager. If we want to take that path a followup might work so that we could start tracking submission numbers now.

Comment 14

[Steven MacLeod [:smacleod]]

Attachment: Part 3 - Write Crash Submission events v2

Updated from Benjamin's feedback.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8433844 [details] [diff] [review]
Part 1 - Provide crash events path as an env var for the crashreporter

Looks like the differences between this and attachment 8416033 [details] [diff] [review] (which got r+ already) are trivial, so it shouldn't need re-review?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Ted, do you have an estimate on when you can review this? Has been pending for >2 weeks. Can someone else review?

Comment 17

[Benjamin Smedberg]

Comment on attachment 8442338 [details] [diff] [review]
Part 2 - Define Crash Submission events and handle them.

It appears that the current version lumps all submissions into "submission-succeeded" and "submission-failed". Especially with bug 994708, we're going to be also including plugin crash/hang submissions, and they should probably be recorded separately.

So what about:
main-crash-submission-failed
main-crash-submission-succeeded
plugin-crash-submission-...

That will also help if we want to cover the case of post-facto submission (currently via about:crashes, but we will probably fold that into the FHR/support UI directly).

In terms of storage, you're currently using <crashid>-submission as the key in the crashstore. Is that just for simplicity-sake? I had imagined that what we'd do is just modify the existing <crashid> record to add the submission record. But this might indeed be simpler.

Comment 18

[Robert Kaiser]

As Benjamin says, we need to record those cases of different processes differently, as I suspect due to the kind of UI they have that even the submission rates of main and plugin processes differ vastly (we will need that data to assess that, though).

Comment 19

[Steven MacLeod [:smacleod]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> Comment on attachment 8442338 [details] [diff] [review]
> Part 2 - Define Crash Submission events and handle them.
> 
> It appears that the current version lumps all submissions into
> "submission-succeeded" and "submission-failed". Especially with bug 994708,
> we're going to be also including plugin crash/hang submissions, and they
> should probably be recorded separately.
> 
> So what about:
> main-crash-submission-failed
> main-crash-submission-succeeded
> plugin-crash-submission-...

Sure I can modify things to include the type.

 
> That will also help if we want to cover the case of post-facto submission
> (currently via about:crashes, but we will probably fold that into the
> FHR/support UI directly).
> 
> In terms of storage, you're currently using <crashid>-submission as the key
> in the crashstore. Is that just for simplicity-sake? I had imagined that
> what we'd do is just modify the existing <crashid> record to add the
> submission record. But this might indeed be simpler.

I considered modifying the crash record but yes, just treating submissions
as crashes with <crashid>-submission as the key and a different type was much
simpler. I also thought it would possible to do a more involved restructuring
in a followup since the format is versioned.

If we want to modify the format and add the submission data to the crash we
need to decide what happens when a crash is submitted that isn't tracked
in the store (It could have been cleaned up, or discarded due to too many
crashes). Should we just discard the submission? I thought it would be better
to track it still so that we can count it. Should the current API for
retrieving crashes return the submission data along side the crash (Again
then we can't just assume all the crash data is present) or should new API
be introduced for retrieving submission information?

Yes, I agree the current patch is a more hacky solution but I wanted to keep
things simple and get it working.

How do you think we should proceed Benjamin?

Comment 20

[Benjamin Smedberg]

Eventually we're going to want to use this API to drive about:crashes. But I think the format you have here will let us answer all the questions we need for that case.

I don't think there's actually much point in having a submission without the matching report, since we won't know what type of crash it was and so we won't include it in FHR anyway.

This does have the advantage of being simple, so I'm inclined to say we should go with it for now. You don't have to worry about race conditions where we process a submission event file before the corresponding crash event file.

So let's go with this version!

Comment 21

[Benjamin Smedberg]

(Except for the main-crash-submission-* bit, which I do want)

Comment 22

[poiru]

Comment on attachment 8442338 [details] [diff] [review]
Part 2 - Define Crash Submission events and handle them.

Review of attachment 8442338 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/crashes/CrashManager.jsm
@@ +389,5 @@
> +
> +  _addSubmissionAsCrash: function (store, succeeded, id, date) {
> +    if (succeeded) {
> +      return store.addCrash(this.PROCESS_TYPE_SUBMISSION,
> +                            this.SUBMISSION_TYPE_SUCCEEDED,

`succeeded ? this.SUBMISSION_TYPE_SUCCEEDED : ...` might be better here although it will need to be wrapped.

@@ +479,5 @@
> +          break;
> +
> +        case "crash.submission.1":
> +          if (lines.length == 3 && lines[1] === "false") {
> +            this._addSubmissionAsCrash(store, false, lines[0], date);

How about `this._addSubmissionAsCrash(store, lines[1] === "false", lines[0], date);`?

Comment 23

[poiru]

Comment on attachment 8442341 [details] [diff] [review]
Part 3 - Write Crash Submission events v2

Review of attachment 8442341 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for being a code police! :)

(In reply to Birunthan Mohanathas [:poiru] from comment #22)
> Comment on attachment 8442338 [details] [diff] [review]
> Part 2 - Define Crash Submission events and handle them.
> 
> Review of attachment 8442338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +479,5 @@
> > +          break;
> > +
> > +        case "crash.submission.1":
> > +          if (lines.length == 3 && lines[1] === "false") {
> > +            this._addSubmissionAsCrash(store, false, lines[0], date);
> 
> How about `this._addSubmissionAsCrash(store, lines[1] === "false", lines[0],
> date);`?

I meant `lines[1] === "true"`, of course.

::: toolkit/crashreporter/client/crashreporter.cpp
@@ +174,5 @@
>  }
>  
> +static string Basename(const string& file)
> +{
> +  int slashIndex = file.rfind(UI_DIR_SEPARATOR);

Can you change this to `size_t`/`string::size_type` for correctness?

@@ +175,5 @@
>  
> +static string Basename(const string& file)
> +{
> +  int slashIndex = file.rfind(UI_DIR_SEPARATOR);
> +  if (slashIndex >= 0)

... and this to `slashIndex != string::npos`.

@@ +184,5 @@
> +
> +static string GetDumpLocalID()
> +{
> +  string localId = Basename(gReporterDumpFile);
> +  int dot = localId.rfind('.');

`size_t`/`string::size_type` here.

@@ +186,5 @@
> +{
> +  string localId = Basename(gReporterDumpFile);
> +  int dot = localId.rfind('.');
> +
> +  if (dot < 0)

And `== string::npos` here.

@@ +193,5 @@
> +  return localId.substr(0, dot);
> +}
> +
> +static void WriteSubmissionEvent(const bool succeeded,
> +                                 const std::string& remoteId)

Remove `std::`.

@@ +218,1 @@
>  void LogMessage(const std::string& message)

Might as well remove `std::` here as well. All other uses of are already just `string`.

@@ +264,1 @@
>  

Can you remove a newline here to get rid of two empty lines?

@@ +554,5 @@
>      OpenLogFile();
>  
> +#ifdef XP_WIN32
> +    static const wchar_t kEventsDirKey[] = L"MOZ_CRASHREPORTER_EVENTS_DIRECTORY";
> +    const wchar_t *eventsPath = _wgetenv(kEventsDirKey);

Style nit, snuggle pointer star with the type (although this seems to be rather inconsistent in this file).

@@ +560,5 @@
> +      gEventsPath = WideToUTF8(eventsPath);
> +    }
> +#else
> +    static const char kEventsDirKey[] = "MOZ_CRASHREPORTER_EVENTS_DIRECTORY";
> +    const char *eventsPath = getenv(kEventsDirKey);

Same here.

Comment 24

[Steven MacLeod [:smacleod]]

Attachment: Part 2 - Define Crash Submission events and handle them. v2

Submission counts now exist for each crash type.

Comment 25

[Steven MacLeod [:smacleod]]

Attachment: Part 3 - Write Crash Submission events v3

(In reply to Birunthan Mohanathas [:poiru] from comment #23)
> Comment on attachment 8442341 [details] [diff] [review]
> Part 3 - Write Crash Submission events v2
> 
> Review of attachment 8442341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apologies for being a code police! :)

Reviews always welcome :)

 
> @@ +218,1 @@
> >  void LogMessage(const std::string& message)
> 
> Might as well remove `std::` here as well. All other uses of are already
> just `string`.

I didn't update this as I'd rather not make changes to code I'm not already
touching. If you feel strongly though just let me know and it's trivial to
do.


> @@ +264,1 @@
> >  
> 
> Can you remove a newline here to get rid of two empty lines?
> 
> @@ +554,5 @@
> >      OpenLogFile();
> >  
> > +#ifdef XP_WIN32
> > +    static const wchar_t kEventsDirKey[] = L"MOZ_CRASHREPORTER_EVENTS_DIRECTORY";
> > +    const wchar_t *eventsPath = _wgetenv(kEventsDirKey);
> 
> Style nit, snuggle pointer star with the type (although this seems to be
> rather inconsistent in this file).

I kept these pointer stars as they were, to maintain consistency with the other code
dealing with env vars, and the code directly near this. It seems that *most* of the
code in the file actually puts the star with the name.

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 8442341 [details] [diff] [review]
Part 3 - Write Crash Submission events v2

Review of attachment 8442341 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the long delay.

You're going to need a followup to implement this for Android since that uses a wholly separate crash reporter client.

::: toolkit/crashreporter/client/crashreporter.cpp
@@ +192,5 @@
> +
> +  return localId.substr(0, dot);
> +}
> +
> +static void WriteSubmissionEvent(const bool succeeded,

I think I'd be happier with an enum parameter here rather than a boolean. It makes the callers kind of hard to understand.

@@ +196,5 @@
> +static void WriteSubmissionEvent(const bool succeeded,
> +                                 const std::string& remoteId)
> +{
> +  string localId = GetDumpLocalID();
> +  string fpath = gEventsPath + UI_DIR_SEPARATOR + localId + "-submission";

You .clear() gEventsPath down below if the env var isn't set. You should probably just error out here rather than trying to write this to a bogus file path.

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 8445407 [details] [diff] [review]
Part 3 - Write Crash Submission events v3

Review of attachment 8445407 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, r+ed an obsolete patch...

Comment 28

[Benjamin Smedberg]

Comment on attachment 8445392 [details] [diff] [review]
Part 2 - Define Crash Submission events and handle them. v2

This looks good. It assumes that we know, at the time of submission, what "kind" of crash it was. This is always correct for process crashes in the crash reporter, and I think we can infer the data for plugin/content crashes based on the .extra file, so this should work well.

Comment 29

[Steven MacLeod [:smacleod]]

Part 1: https://hg.mozilla.org/integration/fx-team/rev/802779efce2b
Part 2: https://hg.mozilla.org/integration/fx-team/rev/3dd92fb7c846
Part 3: https://hg.mozilla.org/integration/fx-team/rev/76555f34c210

Comment 30

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

I had to back this out in  https://hg.mozilla.org/integration/fx-team/rev/b859e3748f30 for XPCshell bustage and a failure in Valgrind: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=42869330&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=42867851&tree=Fx-Team

Comment 31

[Benjamin Smedberg]

Looks like this was a hardcoded version number in the test here: http://hg.mozilla.org/mozilla-central/annotate/ffb8b976548b/services/healthreport/tests/xpcshell/test_provider_crashes.js#l75

Comment 32

[Steven MacLeod [:smacleod]]

Re-landed with fixes for the xpcshell test and the memory leak.

Part 1: https://hg.mozilla.org/integration/fx-team/rev/cfd608bce725
Part 2: https://hg.mozilla.org/integration/fx-team/rev/e5514ddc55f7
Part 3: https://hg.mozilla.org/integration/fx-team/rev/00309d102efc

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cfd608bce725
https://hg.mozilla.org/mozilla-central/rev/e5514ddc55f7
https://hg.mozilla.org/mozilla-central/rev/00309d102efc

Comment 34

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned to this bug for verification.

Comment 35

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Assigning to Kamil for now, since it's FHR. Will try to verify it in current sprint if time allows us.

Comment 36

[Marco Mucci [:MarcoM]]

Hi Kamil, do you have an update on when this bug could be verified.

Comment 37

[Kamil Jozwiak [:kjozwiak]]

I'll work on this tomorrow and should be done by tomorrow or Monday morning the latest.

Comment 38

[Kamil Jozwiak [:kjozwiak]]

Went through verification using following m-c build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-11-03-02-01-mozilla-central/

It appears that nothing other than main-crash is appearing under the "Raw Data" in about:healthreport. This is what I've attempted so far without any luck:

- crashed the browser several times using the "crashfirefox.exe" utility that can be found here: https://developer.mozilla.org/en-US/docs/How_to_Report_a_Hung_Firefox
- Once fx crashes, used Cu.import("resource://gre/modules/CrashManager.jsm"); & CrashManager.Singleton.runMaintenanceTasks(); to force fx to pick up the previous crash
- refreshed about:healthreport several times
- restarted the computer several times
- went through the above steps several different times creating at least 15 crashes and never saw submission entries under about:healthreport

- changed MOZ_CRASHREPORTER_URL=http://localhost:9999/invalid (through environment variables)
- restarted the computer
- crashed the browser using the "crashfirefox.exe" utility
- noticed that the crash submission window displayed "There was a problem submitting your report"
- used Cu.import("resource://gre/modules/CrashManager.jsm"); & CrashManager.Singleton.runMaintenanceTasks(); to force fx to pick up the previous crash
- refreshed about:healthreport several times
- restarted the computer several times
- went through the above steps several different times creating at least 15 crashes and never saw submission entries under about:healthreport

I've also attempted to use http://ted.mielczarek.org/mozilla/crashme.html but still couldn't get any submission data under about:healthreport. The only piece of data that I see under about:healthreport is:

"org.mozilla.crashes.crashes": {
     "_v": 4,
     "main-crash": 1
}

Benjamin, am I doing something incorrectly or is this not working as intended?

Comment 39

[Benjamin Smedberg]

Nothing sounds obviously wrong in your test steps. smacleod, how did you verify this locally?

Comment 40

[Steven MacLeod [:smacleod]]

Those steps seem like they would work to me, here is what I did:

- Created fresh profile
- set devtools.chrome.enabled to true
- Run the following in the browser console to cause a crash:
    Cu.import("resource://gre/modules/ctypes.jsm");
    let zero = new ctypes.intptr_t(8);
    let badptr = ctypes.cast(zero, ctypes.PointerType(ctypes.int32_t));
    badptr.contents;
- Have the crash reporter make a submission and exit firefox
- Verified crash and submission event files were in "/path/to/profile/crashes/events"
- Run firefox again and execute:
    Cu.import("resource://gre/modules/CrashManager.jsm");
    CrashManager.Singleton.runMaintenanceTasks();
- Verified crash event files were processed and deleted
- Restart firefox
- Verified results in FHR raw data:
    "org.mozilla.crashes.crashes": {
      "_v": 4,
      "main-crash": 1,
      "main-crash-submission-succeeded": 1
    },

Just ran through those steps on my development build nightly again and it WFM ^

Comment 41

[Kamil Jozwiak [:kjozwiak]]

So it looks like the crash is generating the two files but the submission file is not being processed or deleted from the events folder, example:

- installed the latest fx m-c using the following build:
--> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-11-03-02-01-mozilla-central/
- using the code in comment #40, I produced the fx crash (Submitted and Quit)
- crashing fx produced two files under "/profile/crashes/events"
--> (1) 13c7cf5f-19ee-4c79-a1a0-2dddb32fac85
--> (2) 13c7cf5f-19ee-4c79-a1a0-2dddb32fac85-submission
- Executed the runMaintenanceTasks(); code

Once you execute the runMaintenanceTasks(); code, only the first file is processed and removed and the submission file is left behind. I've tried this on a Win 8.1 VM instance and my personal Win 8.1 Desktop with the same results on both machines.

smacleod, would you be able to quickly try this on a none development build? Maybe there's a difference between the build you're using and the nightly m-c build that's available on the FTP.

Comment 42

[Kamil Jozwiak [:kjozwiak]]

Seems like this isn't working on Windows machines.. I've tried the following just to make sure:

- OSX 10.9.4 (Both files have been processed and removed from the events folder)
- Ubuntu 14.04 (Both files have been processed and removed from the events folder)
- Win 8.1, Win 7, Win Vista (Only the main crash event is being processed and removed from the events folder)

Comment 43

[Steven MacLeod [:smacleod]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #42)
> Seems like this isn't working on Windows machines.. I've tried the following
> just to make sure:
> 
> - OSX 10.9.4 (Both files have been processed and removed from the events
> folder)
> - Ubuntu 14.04 (Both files have been processed and removed from the events
> folder)
> - Win 8.1, Win 7, Win Vista (Only the main crash event is being processed
> and removed from the events folder)

Ah, the crashreporter is creating submission files with CRLF line endings on
windows which is stopping it from being processed properly.

When processing the event files we create the type by taking everything up to
the first LF, which on windows gives us "<crash-type>\x0D". Then when we
compare this with our event type strings no match is found and we return
EVENT_FILE_ERROR_UNKNOWN_EVENT; When we hit an unknown event type we just
leave the file.

Benjamin / Ted, any suggestions for how I should tackle opening the file stream [1]
as binary in a platform agnostic way (So we don't get any LF -> CRLF translation
shenanigans)?

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp?rev=00309d102efc#208

Comment 44

[Benjamin Smedberg]

Well, we have two options: we can either write unix-style ending on Windows, which is fine except it makes opening them in notepad harder. Or we can modify the CrashManager to recognize and accept windows line endings on windows.

To use unix-style endings, you'd have to specify ios_base::binary here: http://hg.mozilla.org/mozilla-central/annotate/340b19c14d3d/toolkit/crashreporter/client/crashreporter_win.cpp#l1465 and deal with other code which expects something else.

Comment 45

[Steven MacLeod [:smacleod]]

Attachment: Part 4 - Fix CRLF line endings on windows.

This fixes the line ending issues. I built and tested on OSX, ubuntu, and Win 7, verifying that it now works correctly on each platform.

I did however only build on windows with MSVC 2010, so I haven't tested the other two ifdef branch cases... It was enough of a hassle fixing my windows build for just one compiler. Benjamin, any suggestions here? Do we support the other compiler options still?

Comment 46

[Benjamin Smedberg]

Comment on attachment 8457453 [details] [diff] [review]
Part 4 - Fix CRLF line endings on windows.

I think we should just make the old-MSVC case an #error. We don't support building with those compilers any more.

The GCC (mingw) case, on the other hand, at least should work in theory, but it's not strongly supported. Jacek might want to know about and test this patch, but I don't think it should block landing.

Comment 47

[Steven MacLeod [:smacleod]]

Part 4: https://hg.mozilla.org/integration/fx-team/rev/a469473e6a43

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a469473e6a43

Comment 49

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following m-c build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-20-03-02-03-mozilla-central/

- ensured that once a crash occurs, quitting/submitting creates two separate entries under "/crash/events"
- ensured that once a crash occurs, restarting/submitting creates two separate entries under "/crash/events"
- ensured selecting not to submit, the "submission" entry isn't created under "/events/events" when restarting fx
- ensured selecting not to submit, the "submission" entry isn't created under "/events/events" when quitting fx
- ensured that executing runMaintenanceTasks(); removes both entries from "/crash/events"
- ensured "main-crash" is being incremented when appropriate
- ensured "main-crash-submission-succeeded" is being incremented when appropriate
- ensured that only the main crash event is created in "/crash/events" when the user doesn't submit the crash report
- set "MOZ_CRASHREPORTER_URL=http://localhost:9999/invalid" and ensured "main-crash-submission-failed" was incremented when appropriate
- ensured that the submitted crashes are appearing under about:crashes
- ensured that the crashes that haven't been submitted are not appearing under about:crashes

Also went through cases where each file was submitted separately using runMaintenanceTasks();, example:
- crash fx using the code from comment # 40
- once crashed, leave the crash reporter opened and re-launch fx (should only create a single file under "/crash/events")
- runMaintenanceTasks(); (should only increment "main-crash")
- select either restart/quit fx and submit the report (should only create a single file under "/crash/events")
- runMaintenanceTasks(); (should only increment "main-crash-submission-succeeded")

Went through all of the above test cases using:
- Win 8.1 x64
- Ubuntu 13.10 x64
- OSX 10.9.4 x64

Comment 50

[Jacek Caban]

(In reply to Benjamin Smedberg  [:bsmedberg] Away 19-July through 3-Aug from comment #46)
> Jacek might want to know about and test this patch, but I don't think it should block landing.

Sorry for the delay. mingw will use GCC code, which should be good enough. I will take care of that if it causes problems.