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

Issue 154653002: Breakpad coverage for chrome_elf start up (Closed)

Created:
6 years, 10 months ago by Cait (Slow)
Modified:
6 years, 10 months ago
CC:
chromium-reviews, tfarina, caitkp+watch_chromium.org, robertshield, csharp
Visibility:
Public.

Description

Breakpad coverage for chrome_elf start up Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252870

Patch Set 1 #

Total comments: 8

Patch Set 2 : More breakpad, less base #

Total comments: 10

Patch Set 3 : Address comments and catch crashes in blacklist intercept code #

Total comments: 8

Patch Set 4 : Reg key checks #

Total comments: 14

Patch Set 5 : Remove base dependency and clean up logic a bit #

Patch Set 6 : Missed some nits #

Total comments: 33

Patch Set 7 : Use Environment Vars to get program dir #

Total comments: 56

Patch Set 8 : Tests and Greg's comments #

Total comments: 58

Patch Set 9 : Add policy checks and headlessness #

Total comments: 48

Patch Set 10 : More clean up #

Total comments: 10

Patch Set 11 : #

Total comments: 2

Patch Set 12 : rebase and last nits #

Patch Set 13 : Fix tests on chromium builds #

Unified diffs Side-by-side diffs Delta from patch set Stats (+724 lines, -40 lines) Patch
M chrome_elf/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M chrome_elf/blacklist.gypi View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M chrome_elf/blacklist/blacklist_interceptions.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +46 lines, -19 lines 0 comments Download
A chrome_elf/breakpad.h View 1 2 3 4 5 6 7 1 chunk +34 lines, -0 lines 0 comments Download
A chrome_elf/breakpad.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +180 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf.gyp View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +45 lines, -3 lines 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 2 2 chunks +8 lines, -2 lines 0 comments Download
A chrome_elf/chrome_elf_util.h View 1 2 3 4 5 6 7 8 1 chunk +27 lines, -0 lines 0 comments Download
A chrome_elf/chrome_elf_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +193 lines, -0 lines 0 comments Download
A chrome_elf/chrome_elf_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +186 lines, -0 lines 0 comments Download
M chrome_elf/create_file/chrome_create_file.h View 1 2 3 1 chunk +0 lines, -4 lines 0 comments Download
M chrome_elf/create_file/chrome_create_file.cc View 1 2 3 2 chunks +1 line, -5 lines 0 comments Download
M chrome_elf/create_file/chrome_create_file_unittest.cc View 1 2 3 1 chunk +0 lines, -6 lines 0 comments Download

Messages

Total messages: 31 (0 generated)
Cait (Slow)
Siggi -- PTAL and let me know if I'm on the right track for the ...
6 years, 10 months ago (2014-02-04 22:04:27 UTC) #1
Sigurður Ásgeirsson
looks good so far :) https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc#newcode141 chrome_elf/breakpad.cc:141: // TODO(caitkp): Is terminating ...
6 years, 10 months ago (2014-02-05 13:58:32 UTC) #2
Cait (Slow)
+Robert, PTAL at breakpad.cc/h to make sure I didn't remove anything vital while gutting all ...
6 years, 10 months ago (2014-02-07 15:32:02 UTC) #3
robertshield
https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc#newcode73 chrome_elf/breakpad.cc:73: // Get the alternate dump directory. We use the ...
6 years, 10 months ago (2014-02-07 20:03:07 UTC) #4
Cait (Slow)
PTAL. This addresses the comments in the previous patch, and adds breakpad support in blacklist ...
6 years, 10 months ago (2014-02-10 18:13:20 UTC) #5
robertshield
https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist.gypi File chrome_elf/blacklist.gypi (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist.gypi#newcode26 chrome_elf/blacklist.gypi:26: # TODO(caitkp): Why do we need this here? possibly ...
6 years, 10 months ago (2014-02-10 18:43:13 UTC) #6
Cait (Slow)
https://codereview.chromium.org/154653002/diff/140001/chrome_elf/breakpad.h File chrome_elf/breakpad.h (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/breakpad.h#newcode13 chrome_elf/breakpad.h:13: // that the user has agreed to crash dump ...
6 years, 10 months ago (2014-02-10 20:42:55 UTC) #7
Cait (Slow)
+Greg to confirm RegKey-checking logic is correct (chrome_elf_util.cc is where said logic lives). Thanks!
6 years, 10 months ago (2014-02-11 21:29:53 UTC) #8
robertshield
Looks awesome! A few comments, also there may have been some comments left unaddressed from ...
6 years, 10 months ago (2014-02-11 22:26:58 UTC) #9
Cait (Slow)
PTAL -- I've clean up the registry checking logic a bit, reinvented a few wheels, ...
6 years, 10 months ago (2014-02-12 19:15:41 UTC) #10
robertshield
https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_util.cc File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_util.cc#newcode54 chrome_elf/chrome_elf_util.cc:54: bool found = wcsncmp(exe_path, L"C:\\Program Files", 15) == 0; ...
6 years, 10 months ago (2014-02-12 21:12:37 UTC) #11
Cait (Slow)
Still todo: sort out what pipe we should be using. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc#newcode6 ...
6 years, 10 months ago (2014-02-13 00:03:19 UTC) #12
grt (UTC plus 2)
some comments. a bunch of the files have the old chunk mismatch error, so i ...
6 years, 10 months ago (2014-02-13 03:58:25 UTC) #13
Cait (Slow)
getting there... https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#newcode1 chrome_elf/breakpad.cc:1: // Copyright (c) 2014 The Chromium Authors. ...
6 years, 10 months ago (2014-02-14 01:17:01 UTC) #14
grt (UTC plus 2)
https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_util.cc File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_util.cc#newcode50 chrome_elf/chrome_elf_util.cc:50: return ERROR_SUCCESS == result; On 2014/02/14 01:17:02, Cait Phillips ...
6 years, 10 months ago (2014-02-14 16:37:31 UTC) #15
Cait (Slow)
https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#newcode10 chrome_elf/breakpad.cc:10: #include <string> On 2014/02/14 16:37:33, grt wrote: > not ...
6 years, 10 months ago (2014-02-14 23:12:03 UTC) #16
grt (UTC plus 2)
looking good https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/blacklist_interceptions.cc File chrome_elf/blacklist/blacklist_interceptions.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/blacklist_interceptions.cc#newcode221 chrome_elf/blacklist/blacklist_interceptions.cc:221: GetNtDllExportByName("NtQuerySection")); nit: either 4-space indent here, or ...
6 years, 10 months ago (2014-02-17 20:49:38 UTC) #17
Cait (Slow)
PTAL -- thanks for yet another round of comments, Greg :) https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/blacklist_interceptions.cc File chrome_elf/blacklist/blacklist_interceptions.cc (right): ...
6 years, 10 months ago (2014-02-18 23:03:11 UTC) #18
grt (UTC plus 2)
https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc#newcode144 chrome_elf/breakpad.cc:144: base::string16 pipe_name = kGoogleUpdatePipeName; nix base::string16 so that pipe_name ...
6 years, 10 months ago (2014-02-19 15:55:36 UTC) #19
Cait (Slow)
https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc#newcode144 chrome_elf/breakpad.cc:144: base::string16 pipe_name = kGoogleUpdatePipeName; On 2014/02/19 15:55:36, grt wrote: ...
6 years, 10 months ago (2014-02-19 20:47:58 UTC) #20
grt (UTC plus 2)
lgtm!
6 years, 10 months ago (2014-02-20 15:11:50 UTC) #21
robertshield
Awesome! LGTM too. https://codereview.chromium.org/154653002/diff/1050001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/1050001/chrome_elf/breakpad.cc#newcode64 chrome_elf/breakpad.cc:64: wchar_t* sid_string; nit: = NULL https://codereview.chromium.org/154653002/diff/1050001/chrome_elf/chrome_elf_util.cc ...
6 years, 10 months ago (2014-02-20 16:44:55 UTC) #22
Cait (Slow)
+mark@ for breakpad/ owners approval of DEPS change (chrome_elf now depends on breakpad/src/client). background: this ...
6 years, 10 months ago (2014-02-20 19:01:09 UTC) #23
Mark Mentovai
LGTM
6 years, 10 months ago (2014-02-20 19:27:00 UTC) #24
Cait (Slow)
The CQ bit was checked by caitkp@chromium.org
6 years, 10 months ago (2014-02-20 19:28:27 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/154653002/1130001
6 years, 10 months ago (2014-02-20 19:35:28 UTC) #26
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-20 22:10:10 UTC) #27
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=267248
6 years, 10 months ago (2014-02-20 22:10:11 UTC) #28
Cait (Slow)
The CQ bit was checked by caitkp@chromium.org
6 years, 10 months ago (2014-02-23 18:28:02 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/154653002/1370001
6 years, 10 months ago (2014-02-23 18:28:17 UTC) #30
commit-bot: I haz the power
6 years, 10 months ago (2014-02-24 02:12:11 UTC) #31
Message was sent while issue was closed.
Change committed as 252870

Powered by Google App Engine
This is Rietveld 408576698