Support for multiple upload files in Breakpad

RESOLVED FIXED in mozilla34

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8466856 [details] [diff] [review]
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.
Attachment #8466856 - Flags: feedback?(ted)
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
Attachment #8466856 - Flags: feedback?(ted) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8468172 [details] [diff] [review]
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.
Attachment #8466856 - Attachment is obsolete: true
Attachment #8468172 - Flags: review?(ted)
(Assignee)

Comment 4

3 years ago
Tested on Windows: https://crash-stats.mozilla.com/report/index/3abe3576-f8c0-48be-a253-d23ab2140806
We probably need a change on the Socorro collector side to support this, FYI.
(Assignee)

Comment 6

3 years ago
> We probably need a change on the Socorro collector side to support this, FYI.
Does bug 1026059 cover what you had in mind?
Yes it does, thanks.
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
Attachment #8468172 - Flags: review?(ted) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8473358 [details] [diff] [review]
Only the breakpad files

Here's just the google-breakpad directory (and the requested braces).
Attachment #8473358 - Flags: checkin?(ted)
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.
Attachment #8473358 - Flags: checkin?(ted)
(Assignee)

Comment 11

3 years ago
Want to keep this on your radar. Guess I'll have to use some other flag :-)
Flags: needinfo?(ted)
Thanks for the reminder, landed upstream:
https://code.google.com/p/google-breakpad/source/detail?r=1367
Flags: needinfo?(ted)
(Assignee)

Comment 13

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1be81104d7d9
https://hg.mozilla.org/mozilla-central/rev/1be81104d7d9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Comment 15

3 years ago
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

3 years ago
I’m backing this out upstream because of the build break. In the future, can we do the code review on breakpad.appspot.com?
(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.)
(Assignee)

Comment 18

3 years ago
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?
(Assignee)

Comment 19

3 years ago
Created attachment 8485564 [details] [diff] [review]
Breakpad files (v2)
Attachment #8473358 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
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?
Flags: needinfo?(ted)

Comment 21

3 years ago
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.
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
Flags: needinfo?(ted)
You need to log in before you can comment on or make changes to this bug.