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

Issue 1018953004: Add SeccompSupportDetector for Android. (Closed)

Created:
5 years, 9 months ago by Robert Sesek
Modified:
5 years, 9 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add SeccompSupportDetector for Android. This class will report to UMA the Android kernel version and the level of kernel support for seccomp-bpf sandboxing. BUG=468455 Committed: https://crrev.com/e59ea892a5fb3d9c0bfa97c8f1e567fd57a07130 Cr-Commit-Position: refs/heads/master@{#321451}

Patch Set 1 : #

Total comments: 22

Patch Set 2 : Address jln@ comments #

Patch Set 3 : Detect separately #

Total comments: 4

Patch Set 4 : DCHECK #

Total comments: 18

Patch Set 5 : Address comments #

Patch Set 6 : Fix use_seccomp_bpf==0 #

Total comments: 2

Patch Set 7 : GN and comment #

Patch Set 8 : More GN #

Unified diffs Side-by-side diffs Delta from patch set Stats (+300 lines, -1 line) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/browser/android/seccomp_support_detector.h View 1 2 3 4 5 6 1 chunk +46 lines, -0 lines 0 comments Download
A chrome/browser/android/seccomp_support_detector.cc View 1 2 3 4 5 1 chunk +127 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main_android.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main_android.cc View 1 2 3 4 3 chunks +10 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/chrome_utility.gypi View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/common/chrome_utility_messages.h View 1 2 2 chunks +16 lines, -0 lines 0 comments Download
M chrome/utility/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +6 lines, -1 line 0 comments Download
M chrome/utility/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.cc View 1 2 3 4 5 3 chunks +24 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 3 chunks +48 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (11 generated)
Robert Sesek
Julien: Mind taking a look before I send this out for OWNERS?
5 years, 9 months ago (2015-03-18 19:04:51 UTC) #3
jln (very slow on Chromium)
Looks good in general, but the ownership of the SeccompSupportDetector instance confused me. Am I ...
5 years, 9 months ago (2015-03-18 20:49:15 UTC) #5
Robert Sesek
Thanks Julien! https://codereview.chromium.org/1018953004/diff/20001/chrome/browser/android/seccomp_support_detector.cc File chrome/browser/android/seccomp_support_detector.cc (right): https://codereview.chromium.org/1018953004/diff/20001/chrome/browser/android/seccomp_support_detector.cc#newcode17 chrome/browser/android/seccomp_support_detector.cc:17: enum AndroidSeccompStatus { On 2015/03/18 20:49:15, jln ...
5 years, 9 months ago (2015-03-18 21:41:36 UTC) #6
Robert Sesek
PTAL. I changed utility process to send a message after each probing, that way if ...
5 years, 9 months ago (2015-03-18 22:30:32 UTC) #7
jln (very slow on Chromium)
lgtm
5 years, 9 months ago (2015-03-18 22:34:16 UTC) #8
jln (very slow on Chromium)
On 2015/03/18 22:30:32, Robert Sesek wrote: > PTAL. I changed utility process to send a ...
5 years, 9 months ago (2015-03-18 22:35:06 UTC) #9
jln (very slow on Chromium)
lgtm, but the complexity of our IPC system makes it somewhat difficult to be confident ...
5 years, 9 months ago (2015-03-19 00:12:12 UTC) #10
Robert Sesek
Thanks for the review. https://codereview.chromium.org/1018953004/diff/80001/chrome/browser/android/seccomp_support_detector.cc File chrome/browser/android/seccomp_support_detector.cc (right): https://codereview.chromium.org/1018953004/diff/80001/chrome/browser/android/seccomp_support_detector.cc#newcode126 chrome/browser/android/seccomp_support_detector.cc:126: UMA_HISTOGRAM_ENUMERATION("Android.SeccompStatus.Syscall", On 2015/03/19 00:12:12, jln ...
5 years, 9 months ago (2015-03-19 00:18:41 UTC) #11
Robert Sesek
+dtrainor for chrome/browser/android/ +thestig for the rest of chrome/ +isherman for histograms.xml
5 years, 9 months ago (2015-03-19 00:19:04 UTC) #13
Ilya Sherman
https://codereview.chromium.org/1018953004/diff/100001/chrome/browser/android/seccomp_support_detector.cc File chrome/browser/android/seccomp_support_detector.cc (right): https://codereview.chromium.org/1018953004/diff/100001/chrome/browser/android/seccomp_support_detector.cc#newcode71 chrome/browser/android/seccomp_support_detector.cc:71: UMA_HISTOGRAM_CUSTOM_ENUMERATION( nit: Maybe a sparse enum would be simpler ...
5 years, 9 months ago (2015-03-19 00:37:44 UTC) #14
Lei Zhang
https://codereview.chromium.org/1018953004/diff/100001/chrome/browser/android/seccomp_support_detector.cc File chrome/browser/android/seccomp_support_detector.cc (right): https://codereview.chromium.org/1018953004/diff/100001/chrome/browser/android/seccomp_support_detector.cc#newcode32 chrome/browser/android/seccomp_support_detector.cc:32: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, BrowserThread::PostBlockingPoolTask https://codereview.chromium.org/1018953004/diff/100001/chrome/browser/android/seccomp_support_detector.cc#newcode83 chrome/browser/android/seccomp_support_detector.cc:83: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); DCHECK_CURRENTLY_ON() is the ...
5 years, 9 months ago (2015-03-19 00:42:23 UTC) #15
David Trainor- moved to gerrit
chrome/browser/android lgtm with thestig@'s nits.
5 years, 9 months ago (2015-03-19 05:54:35 UTC) #16
Robert Sesek
Fix use_seccomp_bpf==0
5 years, 9 months ago (2015-03-19 13:56:42 UTC) #17
Robert Sesek
Thanks for the reviews, all comments addressed. https://codereview.chromium.org/1018953004/diff/100001/chrome/browser/android/seccomp_support_detector.cc File chrome/browser/android/seccomp_support_detector.cc (right): https://codereview.chromium.org/1018953004/diff/100001/chrome/browser/android/seccomp_support_detector.cc#newcode32 chrome/browser/android/seccomp_support_detector.cc:32: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, ...
5 years, 9 months ago (2015-03-19 14:57:30 UTC) #18
Lei Zhang
I'm not sure what the Android GN build status is, but look for the GN ...
5 years, 9 months ago (2015-03-19 18:29:13 UTC) #19
Lei Zhang
https://codereview.chromium.org/1018953004/diff/140001/chrome/browser/android/seccomp_support_detector.h File chrome/browser/android/seccomp_support_detector.h (right): https://codereview.chromium.org/1018953004/diff/140001/chrome/browser/android/seccomp_support_detector.h#newcode25 chrome/browser/android/seccomp_support_detector.h:25: // Called on the FILE thread. This reads the ...
5 years, 9 months ago (2015-03-19 18:32:04 UTC) #20
Robert Sesek
On 2015/03/19 18:29:13, Lei Zhang wrote: > I'm not sure what the Android GN build ...
5 years, 9 months ago (2015-03-19 19:37:16 UTC) #21
Lei Zhang
https://codereview.chromium.org/1018953004/diff/160001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1018953004/diff/160001/chrome/chrome_browser.gypi#newcode3429 chrome/chrome_browser.gypi:3429: 'sources': [ '<@(chrome_browser_android_sources)' ], This is chrome/browser/BUILD.gn line 596. ...
5 years, 9 months ago (2015-03-19 19:46:36 UTC) #22
Robert Sesek
On 2015/03/19 19:46:36, Lei Zhang wrote: > https://codereview.chromium.org/1018953004/diff/160001/chrome/chrome_browser.gypi > File chrome/chrome_browser.gypi (right): > > https://codereview.chromium.org/1018953004/diff/160001/chrome/chrome_browser.gypi#newcode3429 ...
5 years, 9 months ago (2015-03-19 20:02:36 UTC) #24
Lei Zhang
lgtm
5 years, 9 months ago (2015-03-19 20:43:56 UTC) #25
Ilya Sherman
histograms LGTM, thanks
5 years, 9 months ago (2015-03-19 21:37:12 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1018953004/200001
5 years, 9 months ago (2015-03-19 22:02:40 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/50880)
5 years, 9 months ago (2015-03-19 22:10:16 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1018953004/200001
5 years, 9 months ago (2015-03-19 22:23:30 UTC) #35
commit-bot: I haz the power
Committed patchset #8 (id:200001)
5 years, 9 months ago (2015-03-19 22:28:07 UTC) #36
commit-bot: I haz the power
5 years, 9 months ago (2015-03-19 22:29:03 UTC) #37
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/e59ea892a5fb3d9c0bfa97c8f1e567fd57a07130
Cr-Commit-Position: refs/heads/master@{#321451}

Powered by Google App Engine
This is Rietveld 408576698