Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(646)

Issue 8120007: Update Breakpad to r842 and fix the Breakpad bootstrap service leak and -10810 errors (Closed)

Created:
9 years, 2 months ago by Mark Mentovai
Modified:
9 years, 2 months ago
Reviewers:
Avi (use Gerrit)
CC:
chromium-reviews, darin-cc_chromium.org, pam+watch_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

Update Breakpad to r842, picking up: ------------------------------------------------------------------------ r842 | mark@chromium.org | 2011-10-03 15:54:28 -0400 (Mon, 03 Oct 2011) | 11 lines Use a bootstrap subset port for the inspector, tying the subset to the lifetime of the task to be monitored, the invoking task. This allows the bootstrap server (in launchd) to automatically clean up the Mach server registration when the task being monitored exits, avoiding leaks of com.Breakpad.Inspector(pid) ports in "launchctl bslist". BUG=chromium:28547 TEST=Handler should still crash catches, but inspector ports should no longer show up in "launchctl bslist". They should show up under a subset port in "launchctl bstree" instead. "launchctl bstree" must be invoked as root. Review URL: http://breakpad.appspot.com/306001 ------------------------------------------------------------------------ This gives each Breakpad instance its own bootstrap subset port as a subset of the process' bootstrap port, and places the Breakpad inspector port into the bootstrap subset port. This completely eliminates leaks of on-demand Mach services advertised via the bootstrap server. This also reverts r34318 and r34534, "temporary" (21-month) hacks to mitigate the leak. The temporary hacks were never completely effective against Breakpad ports leaking from child processes. DestructCrashReporter at process shutdown was deemed unnecessary and is being removed. As this was the last caller to that function, the implementation is removed as well. This is addressed in Breakpad rather than Chrome because a mechanism to fix it without changing the process' bootstrap port was discovered. It was determined that the rohitfork port is already immune to leaks of the sort that the Breakpad inspector port experiences. A previous attempt to fix this bug was r103089, but that caused bug 98550. This strategy is safer. The forked Breakpad module.cc is updated to match upstream r835, which allows the unforking of the CFI-disabling code. A new CFI-disabling "-c" option to dump_syms is now available and used by the dump_product_syms script. BUG=28547 TEST=1. "launchctl bslist" should no longer show on-demand com.Breakpad.Inspector ports (in Breakpad-enabled builds with Breakpad on) ports. 2. "launchctl bstree" (as root) should reveal a bootstrap subset for each process as a child of the per-user/per-session bootstrap namespace if crash reporting is on. One com.Breakpad.Inspector port should show up as a child of each bootstrap subset. As each process exits (even if mercilessly killed by "kill -9"), the associated subsets and inspector ports should disappear. 3. Breakpad reports should be generated on crashes. For example, about:crash and about:inducebrowsercrashforrealz should each cause a minidump to be written in ~/Library/Application Support/Google/Chrome/Crash Reports when Breakpad is enabled. This tests that the Breakpad ports are functioning properly. 4. The browser process should be able to access child process data. Window:Task Manager should show valid values for the Memory, CPU, and Network columns for all child processes. This tests that the rohitfork port is functioning properly. Note that this version of the change does not change the handling of rohitfork ports at all, but this test case was useful in a previous version of this patch. 5. Test case from bug 98550: Have a link in a web browser window (in a different browser) and drag it into Chrome. Expect the drag operation to operate properly in both the tab strip and in web content. See bug 98550 for details. 6. Unreported test case that failed in r103089: browser relaunch should work. Visit chrome://flags, change some flags to get the "Relaunch Now" button at the bottom (you can put the flags back to how you found them initially), and click "Relaunch Now". The browser should close and then be relaunched. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103778

Patch Set 1 #

Total comments: 1

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -207 lines) Patch
M DEPS View 1 chunk +1 line, -1 line 0 comments Download
M breakpad/breakpad.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M breakpad/pending/src/common/module.cc View 14 chunks +47 lines, -46 lines 0 comments Download
M chrome/app/breakpad_mac.h View 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/app/breakpad_mac.mm View 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/app/breakpad_mac_stubs.mm View 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/app/chrome_main.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/renderer/chrome_render_process_observer.cc View 3 chunks +0 lines, -140 lines 0 comments Download
M chrome/tools/build/mac/dump_product_syms View 1 chunk +5 lines, -2 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Mark Mentovai
9 years, 2 months ago (2011-10-03 20:20:19 UTC) #1
Avi (use Gerrit)
lgtm with note http://codereview.chromium.org/8120007/diff/1/breakpad/pending/src/common/module.cc File breakpad/pending/src/common/module.cc (right): http://codereview.chromium.org/8120007/diff/1/breakpad/pending/src/common/module.cc#newcode41 breakpad/pending/src/common/module.cc:41: #include <iostream> is this necessary? it ...
9 years, 2 months ago (2011-10-03 20:26:58 UTC) #2
Mark Mentovai
9 years, 2 months ago (2011-10-03 20:28:32 UTC) #3
Avi wrote:
> lgtm with note
> 
>
http://codereview.chromium.org/8120007/diff/1/breakpad/pending/src/common/mod...
> File breakpad/pending/src/common/module.cc (right):
> 
>
http://codereview.chromium.org/8120007/diff/1/breakpad/pending/src/common/mod...
> breakpad/pending/src/common/module.cc:41: #include <iostream>
> is this necessary? it introduces a static constructor. iosfwd?

This is in upstream code that we’ve temporarily forked. It doesn’t become part
of the application, it only becomes part of the dump_syms tool.

(The dump_syms tool also uses RTTI. Yuck, but the exception’s already been
granted.)

Powered by Google App Engine
This is Rietveld 408576698