Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
28833002:
Enable SysInfo::AmountOfPhysicalMemory to be called from within the Linux sandbox. (Closed)
|
Created:
7 years, 2 months ago by rmcilroy Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, Jakob Kummerow Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionEnable SysInfo::AmountOfPhysicalMemory to be called from within the Linux sandbox.
Trigger caching of SysInfo::AmountOfPhysicalMemory in PreSandboxInit() to enable
it to be called by the renderer process after the sandbox is sealed.
BUG=312241
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231613
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : Use LazyInstance. #
Total comments: 10
Patch Set 5 : Address Comments #
Messages
Total messages: 19 (0 generated)
jln@chromium.org: Please review changes in content/zygote. jar@chromium.org: Please review changes in base
https://codereview.chromium.org/28833002/diff/70001/base/sys_info_linux.cc File base/sys_info_linux.cc (right): https://codereview.chromium.org/28833002/diff/70001/base/sys_info_linux.cc#ne... base/sys_info_linux.cc:33: if (!physical_memory_valid) { I'm hopeful this is done on only one thread... as this would not be thread safe. Can you verify that? DCHECK it?
https://chromiumcodereview.appspot.com/28833002/diff/70001/base/sys_info_linu... File base/sys_info_linux.cc (right): https://chromiumcodereview.appspot.com/28833002/diff/70001/base/sys_info_linu... base/sys_info_linux.cc:33: if (!physical_memory_valid) { On 2013/10/18 19:24:14, jar wrote: > I'm hopeful this is done on only one thread... as this would not be thread safe. > > Can you verify that? DCHECK it? Should we just use LazyInstance these things? It's a bit expensive, but none of these are hot.
https://chromiumcodereview.appspot.com/28833002/diff/70001/base/sys_info_linu... File base/sys_info_linux.cc (right): https://chromiumcodereview.appspot.com/28833002/diff/70001/base/sys_info_linu... base/sys_info_linux.cc:33: if (!physical_memory_valid) { On 2013/10/18 21:08:42, jln wrote: > On 2013/10/18 19:24:14, jar wrote: > > I'm hopeful this is done on only one thread... as this would not be thread > safe. > > > > Can you verify that? DCHECK it? > > Should we just use LazyInstance these things? It's a bit expensive, but none of > these are hot. +1. If in doubt about multi-threading, I'd rather see lazy instance. I don't think the cost is actually that high, and probably not large considering it is caching the answer, and replacing code that probably used to take longer to find the answer again and again.
On 2013/10/18 22:19:30, jar wrote: > https://chromiumcodereview.appspot.com/28833002/diff/70001/base/sys_info_linu... > File base/sys_info_linux.cc (right): > > https://chromiumcodereview.appspot.com/28833002/diff/70001/base/sys_info_linu... > base/sys_info_linux.cc:33: if (!physical_memory_valid) { > On 2013/10/18 21:08:42, jln wrote: > > On 2013/10/18 19:24:14, jar wrote: > > > I'm hopeful this is done on only one thread... as this would not be thread > > safe. > > > > > > Can you verify that? DCHECK it? > > > > Should we just use LazyInstance these things? It's a bit expensive, but none > of > > these are hot. > > +1. If in doubt about multi-threading, I'd rather see lazy instance. I don't > think the cost is actually that high, and probably not large considering it is > caching the answer, and replacing code that probably used to take longer to find > the answer again and again. Good plan. I think the threading would have been ok in the renderer, but I'm less sure about the browser, so I've gone with the LazyInstance idea. PTAL.
lgtm with small nits (you still need base/approval from jar)
https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc File base/sys_info_linux.cc (right): https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:36: NOTREACHED(); How about just "limit = 0" ? https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:40: DCHECK(static_cast<uint64>(limit) <= std::numeric_limits<size_t>::max()); The cast to unsigned is a bit strange and I don't particularly like that we'd truncate limit in release mode. How about: (if limit < 0 || limit > std::numeric_limits<size_t>::max()) limit = 0; DCHECK(limit > 0); That way we're consistently returning 0 on errors. https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:76: int64 GetPhysicalMemory() { Style: physical_memory() as it's a trivial accessor. (Same below) https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:91: std::string cpu_model_name_; Style: add DISALLOW_COPY_AND_ASSIGN?
+1 for the nits by jln ...and one more nit. https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc File base/sys_info_linux.cc (right): https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:71: cpu_model_name_ = CPUModelName(); nit: Given these three items are all constant... how about declaring them const, and then use member initializers?
https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc File base/sys_info_linux.cc (right): https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:36: NOTREACHED(); On 2013/10/21 23:27:28, jln wrote: > How about just "limit = 0" ? Done. https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:40: DCHECK(static_cast<uint64>(limit) <= std::numeric_limits<size_t>::max()); On 2013/10/21 23:27:28, jln wrote: > The cast to unsigned is a bit strange and I don't particularly like that we'd > truncate limit in release mode. > > How about: > (if limit < 0 || limit > std::numeric_limits<size_t>::max()) limit = 0; > DCHECK(limit > 0); > > That way we're consistently returning 0 on errors. Done. Are we sure we want to return 0 rather than truncating though? What about a 32bit binary running on a 64bit machine? The only use of this function in render_process_impl.cc assumes 0 means there is no limit, so I've updated sys_info.h to specify that returning 0 means there is no limit. Does this sound OK? https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:71: cpu_model_name_ = CPUModelName(); On 2013/10/21 23:54:14, jar wrote: > nit: Given these three items are all constant... how about declaring them const, > and then use member initializers? Done. https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:76: int64 GetPhysicalMemory() { On 2013/10/21 23:27:28, jln wrote: > Style: physical_memory() as it's a trivial accessor. > > (Same below) Done. https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... base/sys_info_linux.cc:91: std::string cpu_model_name_; On 2013/10/21 23:27:28, jln wrote: > Style: add DISALLOW_COPY_AND_ASSIGN? Done.
On 2013/10/23 14:51:33, rmcilroy wrote: > https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc > File base/sys_info_linux.cc (right): > > https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... > base/sys_info_linux.cc:36: NOTREACHED(); > On 2013/10/21 23:27:28, jln wrote: > > How about just "limit = 0" ? > > Done. > > https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... > base/sys_info_linux.cc:40: DCHECK(static_cast<uint64>(limit) <= > std::numeric_limits<size_t>::max()); > On 2013/10/21 23:27:28, jln wrote: > > The cast to unsigned is a bit strange and I don't particularly like that we'd > > truncate limit in release mode. > > > > How about: > > (if limit < 0 || limit > std::numeric_limits<size_t>::max()) limit = 0; > > DCHECK(limit > 0); > > > > That way we're consistently returning 0 on errors. > > Done. Are we sure we want to return 0 rather than truncating though? What about > a 32bit binary running on a 64bit machine? The only use of this function in > render_process_impl.cc assumes 0 means there is no limit, so I've updated > sys_info.h to specify that returning 0 means there is no limit. Does this sound > OK? > > https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... > base/sys_info_linux.cc:71: cpu_model_name_ = CPUModelName(); > On 2013/10/21 23:54:14, jar wrote: > > nit: Given these three items are all constant... how about declaring them > const, > > and then use member initializers? > > Done. > > https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... > base/sys_info_linux.cc:76: int64 GetPhysicalMemory() { > On 2013/10/21 23:27:28, jln wrote: > > Style: physical_memory() as it's a trivial accessor. > > > > (Same below) > > Done. > > https://codereview.chromium.org/28833002/diff/140001/base/sys_info_linux.cc#n... > base/sys_info_linux.cc:91: std::string cpu_model_name_; > On 2013/10/21 23:27:28, jln wrote: > > Style: add DISALLOW_COPY_AND_ASSIGN? > > Done. Ping? Still needs approval from Jar.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/28833002/200001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/28833002/200001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/28833002/200001
Message was sent while issue was closed.
Change committed as 231613
Message was sent while issue was closed.
On 2013/10/29 19:32:49, I haz the power (commit-bot) wrote: > Change committed as 231613 This prevented Blink from running layout tests: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%... 12:56:34.390 32717 worker/4 virtual/deferred/fast/images/2-comp.html crashed, (stderr lines): 12:56:34.390 32717 Xlib: extension "RANDR" missing on display ":9". 12:56:34.390 32717 [32734:32734:1029/125632:246485506719:ERROR:audio_manager_pulse.cc(249)] Failed to connect to the context. Error: Connection refused 12:56:34.390 32717 [32734:32734:1029/125632:246485624831:FATAL:thread_restrictions.cc(38)] Function marked as IO-only was called from a thread that disallows IO! If this thread really should be allowed to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() in this thread's startup. 12:56:34.390 32717 [0x7fb7d75c8f90] base::debug::StackTrace::StackTrace() 12:56:34.390 32717 [0x7fb7d7614f4f] logging::LogMessage::~LogMessage() 12:56:34.390 32717 [0x7fb7d769fe61] base::ThreadRestrictions::AssertIOAllowed() 12:56:34.390 32717 [0x7fb7d75f1b70] file_util::OpenFile() 12:56:34.390 32717 [0x7fb7d75ee926] base::ReadFileToString() 12:56:34.390 32717 [0x7fb7d768b67b] (anonymous namespace)::MaxSharedMemorySize() 12:56:34.390 32717 [0x7fb7d768bb5e] (anonymous namespace)::LazySysInfo::LazySysInfo() 12:56:34.390 32717 [0x7fb7d768bdf5] base::DefaultLazyInstanceTraits<>::New() 12:56:34.390 32717 [0x7fb7d768bd14] base::internal::LeakyLazyInstanceTraits<>::New() 12:56:34.390 32717 [0x7fb7d768bcb9] base::LazyInstance<>::Pointer() 12:56:34.390 32717 [0x7fb7d768bc56] base::LazyInstance<>::Get() 12:56:34.391 32717 [0x7fb7d768bbea] base::SysInfo::AmountOfPhysicalMemory() 12:56:34.391 32717 [0x7fb7ceec7537] base::SysInfo::AmountOfPhysicalMemoryMB() 12:56:34.391 32717 [0x7fb7cef98a39] content::RenderProcessHost::GetMaxRendererProcessCount() 12:56:34.391 32717 [0x7fb7cef9d9ba] content::RenderProcessHost::ShouldTryToUseExistingProcessHost() 12:56:34.391 32717 [0x7fb7cf0078e0] content::SiteInstanceImpl::GetProcess() 12:56:34.391 32717 [0x7fb7cefb02d8] content::RenderViewHostImpl::RenderViewHostImpl() 12:56:34.391 32717 [0x7fb7cefaf6df] content::RenderViewHostFactory::Create() 12:56:34.391 32717 [0x7fb7cf0734fe] content::RenderViewHostManager::Init() 12:56:34.391 32717 [0x7fb7cf07e601] content::WebContentsImpl::Init() 12:56:34.391 32717 [0x7fb7cf07b69b] content::WebContentsImpl::CreateWithOpener() 12:56:34.391 32717 [0x7fb7cf07a65f] content::WebContents::Create() 12:56:34.391 32717 [0x00000044fd43] content::Shell::CreateNewWindow() 12:56:34.391 32717 [0x00000045e7b5] content::WebKitTestController::PrepareForLayoutTest() 12:56:34.391 32717 [0x00000044ef19] ShellBrowserMain() 12:56:34.391 32717 [0x00000044cdb7] content::ShellMainDelegate::RunProcess() 12:56:34.391 32717 [0x7fb7cebb5b83] content::RunNamedProcessTypeMain() 12:56:34.391 32717 [0x7fb7cebb6d86] content::ContentMainRunnerImpl::Run() 12:56:34.391 32717 [0x7fb7cebb5047] content::ContentMain() 12:56:34.391 32717 [0x00000044c325] main 12:56:34.391 32717 [0x7fb7cbaab76d] __libc_start_main 12:56:34.391 32717 [0x00000044c239] <unknown> 12:56:34.391 32717
Message was sent while issue was closed.
Revert in progress here: https://codereview.chromium.org/51223003 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
