Closed Bug 1048091 Opened 5 years ago Closed 5 years ago

Support for multiple upload files in Breakpad

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: dmajor, Assigned: dmajor)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch Windows only so far (obsolete) — Splinter Review
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+
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)
We probably need a change on the Socorro collector side to support this, FYI.
> 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+
Attached patch Only the breakpad files (obsolete) — Splinter Review
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)
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)
https://hg.mozilla.org/mozilla-central/rev/1be81104d7d9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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
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.)
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?
Attachment #8473358 - Attachment is obsolete: true
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)
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.