Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676023003:
breakpad: Fix crash_reporter upload success handling on Chrome OS (Closed)
|
Created:
3 years, 10 months ago by James Cook Modified:
3 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbreakpad: Fix crash_reporter upload success handling on Chrome OS
Chrome OS incorrectly prints this message to the ui log after each crash:
"System crash-reporter failed to process crash report."
This makes it confusing to work on crash reporting, as crash_reporter is
actually succeeding.
The problem was how breakpad listens for the crash id / success string.
A helper process reads the crash id/status string from a pipe. That pipe
was connected to fd 3 via sys_dup2(). This worked on Linux because wget was
configured to write status to /dev/fd/3. This failed on Chrome OS because
crash_reporter writes status to stdout (fd 1).
This CL wires crash_reporter's output to stdout using sys_dup2(). It also
eliminates the dup2() code on Linux, because wget can write directly to
the pipe to the helper process.
I also pulled out some helper functions because the existing functions are
too long, and also added some docs and error checking.
BUG=688248
TEST=Navigate to chrome:inducebrowsercrashforrealz on chromebook, look at
/var/log/ui/ui.LATEST, see "Crash dump received by crash_reporter" log line.
Review-Url: https://codereview.chromium.org/2676023003
Cr-Commit-Position: refs/heads/master@{#448724}
Committed: https://chromium.googlesource.com/chromium/src/+/bdead22a98f5f12e9ebc0d9356b5521f5e0be793
Patch Set 1 #Patch Set 2 : fix unused var #
Total comments: 6
Patch Set 3 : review comments #
Total comments: 2
Patch Set 4 : add trailing \n #Messages
Total messages: 32 (23 generated)
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jamescook@chromium.org changed reviewers: + vapier@chromium.org
vapier, can you take a look at this? I would send it to thestig@, but he's OOO for a while.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
i agree passing down uploader_status_fd makes more sense, but i think it was cleaner to have a single func to call rather than splitting into two and then #ifdef-ing the call site https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1247: const char* dumpfile, indent is off here https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1251: // Configure wget to write to file descriptor 3. Do this before other work does it really need to dup ? can't it write to /dev/fd/%(status_fd)i directly ? https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1489: // Attempts to close all open file descriptors other than stdin, stdout and this move doesn't seem related ?
vapier, please take another look. https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1247: const char* dumpfile, On 2017/02/06 17:17:44, vapier wrote: > indent is off here Deleted this block. https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1251: // Configure wget to write to file descriptor 3. Do this before other work On 2017/02/06 17:17:44, vapier wrote: > does it really need to dup ? can't it write to /dev/fd/%(status_fd)i directly ? Changed to write to /dev/fd/<foo> directly. I extracted the int-to-string work into a separate function since it's fiddly and we do it in 3 places. https://codereview.chromium.org/2676023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1489: // Attempts to close all open file descriptors other than stdin, stdout and On 2017/02/06 17:17:44, vapier wrote: > this move doesn't seem related ? No, but HandleCrashDump() is 400 lines long, so I'm trying to clean it up a little bit. I find this file difficult to read.
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Oh, and I've tested this manually on both on Linux and Chrome OS.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2676023003/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2676023003/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1205: const char err[] = "dup2 failed"; this should have a trailing \n right ?
Description was changed from ========== breakpad: Fix crash_reporter upload success handling on Chrome OS Chrome OS incorrectly prints this message to the ui log after each crash: "System crash-reporter failed to process crash report." This makes it confusing to work on crash reporting, as crash_reporter is actually succeeding. The problem was how breakpad listens for the crash id / success string. A helper process reads the crash id/status string from a pipe. That pipe was connected to fd 3 via sys_dup2(). This worked on Linux because wget was configured to write status to /dev/fd/3. This failed on Chrome OS because crash_reporter writes status to stdout (fd 1). This probably wasn't noticed because the magic number 3 was far away in the code from the string /dev/fd/3. I refactored things to put the constants together, extracted some helper functions and added some docs and error checking. BUG=688248 TEST=Navigate to chrome:inducebrowsercrashforrealz on chromebook, look at /var/log/ui/ui.LATEST, see "Crash dump received by crash_reporter" log line. ========== to ========== breakpad: Fix crash_reporter upload success handling on Chrome OS Chrome OS incorrectly prints this message to the ui log after each crash: "System crash-reporter failed to process crash report." This makes it confusing to work on crash reporting, as crash_reporter is actually succeeding. The problem was how breakpad listens for the crash id / success string. A helper process reads the crash id/status string from a pipe. That pipe was connected to fd 3 via sys_dup2(). This worked on Linux because wget was configured to write status to /dev/fd/3. This failed on Chrome OS because crash_reporter writes status to stdout (fd 1). This CL wires crash_reporter's output to stdout using sys_dup2(). It also eliminates the dup2() code on Linux, because wget can write directly to the pipe to the helper process. I also pulled out some helper functions because the existing functions are too long, and also added some docs and error checking. BUG=688248 TEST=Navigate to chrome:inducebrowsercrashforrealz on chromebook, look at /var/log/ui/ui.LATEST, see "Crash dump received by crash_reporter" log line. ==========
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jamescook@chromium.org changed reviewers: + scottmg@chromium.org
Scott, can I get OWNERS? I would send it to thestig@, but he's OOO for a while. https://codereview.chromium.org/2676023003/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2676023003/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:1205: const char err[] = "dup2 failed"; On 2017/02/07 04:53:09, vapier wrote: > this should have a trailing \n right ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jamescook@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vapier@chromium.org Link to the patchset: https://codereview.chromium.org/2676023003/#ps60001 (title: "add trailing \n")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486499632529830,
"parent_rev": "f3ae9f99eb8b04497ec2778d256102a61848eaec", "commit_rev":
"bdead22a98f5f12e9ebc0d9356b5521f5e0be793"}
Message was sent while issue was closed.
Description was changed from ========== breakpad: Fix crash_reporter upload success handling on Chrome OS Chrome OS incorrectly prints this message to the ui log after each crash: "System crash-reporter failed to process crash report." This makes it confusing to work on crash reporting, as crash_reporter is actually succeeding. The problem was how breakpad listens for the crash id / success string. A helper process reads the crash id/status string from a pipe. That pipe was connected to fd 3 via sys_dup2(). This worked on Linux because wget was configured to write status to /dev/fd/3. This failed on Chrome OS because crash_reporter writes status to stdout (fd 1). This CL wires crash_reporter's output to stdout using sys_dup2(). It also eliminates the dup2() code on Linux, because wget can write directly to the pipe to the helper process. I also pulled out some helper functions because the existing functions are too long, and also added some docs and error checking. BUG=688248 TEST=Navigate to chrome:inducebrowsercrashforrealz on chromebook, look at /var/log/ui/ui.LATEST, see "Crash dump received by crash_reporter" log line. ========== to ========== breakpad: Fix crash_reporter upload success handling on Chrome OS Chrome OS incorrectly prints this message to the ui log after each crash: "System crash-reporter failed to process crash report." This makes it confusing to work on crash reporting, as crash_reporter is actually succeeding. The problem was how breakpad listens for the crash id / success string. A helper process reads the crash id/status string from a pipe. That pipe was connected to fd 3 via sys_dup2(). This worked on Linux because wget was configured to write status to /dev/fd/3. This failed on Chrome OS because crash_reporter writes status to stdout (fd 1). This CL wires crash_reporter's output to stdout using sys_dup2(). It also eliminates the dup2() code on Linux, because wget can write directly to the pipe to the helper process. I also pulled out some helper functions because the existing functions are too long, and also added some docs and error checking. BUG=688248 TEST=Navigate to chrome:inducebrowsercrashforrealz on chromebook, look at /var/log/ui/ui.LATEST, see "Crash dump received by crash_reporter" log line. Review-Url: https://codereview.chromium.org/2676023003 Cr-Commit-Position: refs/heads/master@{#448724} Committed: https://chromium.googlesource.com/chromium/src/+/bdead22a98f5f12e9ebc0d9356b5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bdead22a98f5f12e9ebc0d9356b5... |
