1048091 - Support for multiple upload files in Breakpad

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(Away)]

Separating this out from bug 1007534. The gist is that we'll want to upload a .memory.json.gz in addition to a .dmp file.

Comment 1

[(Away)]

Attachment: Windows only so far

Patch originally from bsmedberg with tweaks by me. f? Ted on the Windows piece before I spend time replicating this on the other platforms.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 8466856 [details] [diff] [review]
Windows only so far

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

Looks pretty plausible. Obviously we'll want to land this upstream. Patches for other platforms should actually be easier, as they aren't reinventing the multipart-POST wheel:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc#128
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter_osx.mm#576

Comment 3

[(Away)]

Attachment: Patch for Windows and Linux

Here are the Breakpad pieces for Windows and Linux, along with the minimum boilerplate to keep the clients compiling. I'll plumb the parameter through more nicely in bug 1007534.

AFAICT there are no changes needed for Mac, at least not on the Breakpad side.

Comment 4

[(Away)]

Tested on Windows: https://crash-stats.mozilla.com/report/index/3abe3576-f8c0-48be-a253-d23ab2140806

Comment 5

[(not currently active) Ted Mielczarek]

We probably need a change on the Socorro collector side to support this, FYI.

Comment 6

[(Away)]

> We probably need a change on the Socorro collector side to support this, FYI.
Does bug 1026059 cover what you had in mind?

Comment 7

[(not currently active) Ted Mielczarek]

Yes it does, thanks.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8468172 [details] [diff] [review]
Patch for Windows and Linux

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

This looks good. If you can give me a patch of just the Breakpad bits I'll land that in Breakpad SVN for you.

::: toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc
@@ +124,5 @@
>                   CURLFORM_COPYCONTENTS, iter->second.c_str(),
>                   CURLFORM_END);
>  
> +  // Add form files.
> +  for (iter = files.begin(); iter != files.end(); ++iter)

braces

Comment 9

[(Away)]

Attachment: Only the breakpad files

Here's just the google-breakpad directory (and the requested braces).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8473358 [details] [diff] [review]
Only the breakpad files

This flag is causing the bug to show up on our checkin-needed queries. Since it's obviously not expediting anything, I'm clearing it to reduce the noise at least.

Comment 11

[(Away)]

Want to keep this on your radar. Guess I'll have to use some other flag :-)

Comment 12

[(not currently active) Ted Mielczarek]

Thanks for the reminder, landed upstream:
https://code.google.com/p/google-breakpad/source/detail?r=1367

Comment 13

[(Away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be81104d7d9

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1be81104d7d9

Comment 15

[primiano]

Hmm r1367 seems to have broken Breakpad on Linux with the following:

src/tools/linux/symupload/minidump_upload.cc: In function ‘void Start(Options*)’:
src/tools/linux/symupload/minidump_upload.cc:76:48: error: no matching function for call to ‘google_breakpad::HTTPUpload::SendRequest(std::string&, std::map<std::basic_string<char>, std::basic_string<char> >&, std::string&, const char [21], std::string&, std::string&, const char [1], std::string*, NULL, std::string*)’
src/tools/linux/symupload/minidump_upload.cc:76:48: note: candidate is:
./src/common/linux/http_upload.h:61:15: note: static bool google_breakpad::HTTPUpload::SendRequest(const string&, const std::map<std::basic_string<char>, std::basic_string<char> >&, const std::map<std::basic_string<char>, std::basic_string<char> >&, const string&, const string&, const string&, std::string*, long int*, std::string*)
./src/common/linux/http_upload.h:61:15: note:   candidate expects 9 arguments, 10 provided
make: *** [src/tools/linux/symupload/minidump_upload.o] Error 1

Comment 16

[Mark Mentovai]

I’m backing this out upstream because of the build break. In the future, can we do the code review on breakpad.appspot.com?

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Mark Mentovai from comment #16)
> I’m backing this out upstream because of the build break. In the future, can
> we do the code review on breakpad.appspot.com?

We can, but sometimes it's a hassle to get someone else to review the changes, and it's an extra step that usually falls on me. Sorry about the build bustage, we don't build that tool. (Wish we had some CI for Breakpad.)

Comment 18

[(Away)]

Since the signature of those functions changed, callers will need some trivial fixup, similar to attachment 8468172 [details] [diff] [review].

Sorry for the bustage, I had scanned the google-breakpad repo to look for potential platforms that Mozilla's build wouldn't catch, but I must have missed that tools directory.

I can provide an updated patch that addresses minidump_upload and any other callers I can find. Do I need a separate appspot review?

Comment 19

[(Away)]

Attachment: Breakpad files (v2)

Comment 20

[(Away)]

I've built this successfully on Linux, and grep found one more callsite in the Windows tools, but since this is not my usual environment, could I ask you to double-check that this builds?

Comment 21

[primiano]

I have no way to test on windows.
I applied attachment 8485564 [details] [diff] [review] on Linux and it seems to compile and pass the tests.

Comment 22

[(not currently active) Ted Mielczarek]

I tested the updated patch on Linux and Windows, it built fine and passed tests so I re-landed it:
https://chromium.googlesource.com/breakpad/breakpad/+/cf1087c403de8d496f3f2a919327edaa30385731