Pass StatsTable shared memory via global descriptors on Posix rather than using named shared memory.

Pass StatsTable shared memory via global descriptors on Posix rather than using named shared memory.

This is required for to enable chrome://stats Android where there is no
/dev/shm.  This also provides the added advantage of not requiring the
--no-sandbox command line flag with the --enable-stats-table on Posix.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228903

[rmcilroy, 2013-08-20 20:39:31]

jam@chromium.org: Please review changes in ipc/ and content/
jar@chromium.org: Please review changes in base/metrics

If you are ok with this approach, I am happy to do something similar for Windows
if you can point me in the right direction for sharing HANDLE's in a manner
similar to GlobalDescriptors::.  If so, I think we can remove
SharedMemory::CreateNamed(...) since this is the only place it's used.

[jar (doing other things), 2013-08-21 01:09:46]

Do we make much (any?) use of stats tables? ... or has most of that code been
ripped out?

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc
File base/metrics/stats_table.cc (right):

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:141: Private(SharedMemory* shared_memory)
nit: explicit

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:178: if (!priv->shared_memory_->Map(size))
Your code can return a null in several cases (line 203, or line 214), so you
need to handle such.

if (!priv->shared_memory || !priv->shared_memory_->Map(size))

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:205: } else {
nit: no need for "else" and indent, since you returned in line 204.

Less important nit: When both clauses of an if return... it is more readable to
list the shorter block one up front.

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:599: return impl_->shared_memory()->handle();
Shouldn't you handle impl_ == NULL?

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:212: if ((stats_table =
base::StatsTable::current()) != NULL) {
nit: initialize during the definition, rather than in an if() statement.

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:215:
stats_table->GetSharedMemoryHandle()));
nit: indent: align under first argument.

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:239: if ((stats_table =
base::StatsTable::current()) != NULL) {
nit: initialize in definition, rather than during if().

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:242:
stats_table->GetSharedMemoryHandle()));
nit: indent

[rmcilroy, 2013-08-21 10:36:55]

On 2013/08/21 01:09:46, jar wrote:
> Do we make much (any?) use of stats tables? ... or has most of that code been
> ripped out?

I'm wanting to use it to view the V8 counters. I'm not sure if there are many
other uses of it.

[rmcilroy, 2013-08-21 10:37:16]

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc
File base/metrics/stats_table.cc (right):

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:141: Private(SharedMemory* shared_memory)
On 2013/08/21 01:09:46, jar wrote:
> nit: explicit

Done.

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:178: if (!priv->shared_memory_->Map(size))
On 2013/08/21 01:09:46, jar wrote:
> Your code can return a null in several cases (line 203, or line 214), so you
> need to handle such.
> 
> if (!priv->shared_memory || !priv->shared_memory_->Map(size))

Good point, done.

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:205: } else {
On 2013/08/21 01:09:46, jar wrote:
> nit: no need for "else" and indent, since you returned in line 204.
> 
> Less important nit: When both clauses of an if return... it is more readable
to
> list the shorter block one up front.

Done.

https://codereview.chromium.org/22911027/diff/1/base/metrics/stats_table.cc#n...
base/metrics/stats_table.cc:599: return impl_->shared_memory()->handle();
On 2013/08/21 01:09:46, jar wrote:
> Shouldn't you handle impl_ == NULL?

Done.

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:212: if ((stats_table =
base::StatsTable::current()) != NULL) {
On 2013/08/21 01:09:46, jar wrote:
> nit: initialize during the definition, rather than in an if() statement.

Done.

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:215:
stats_table->GetSharedMemoryHandle()));
On 2013/08/21 01:09:46, jar wrote:
> nit: indent: align under first argument.

I was copying line 210 above.  I've now changed both lines to indent with first
arg.

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:239: if ((stats_table =
base::StatsTable::current()) != NULL) {
On 2013/08/21 01:09:46, jar wrote:
> nit: initialize in definition, rather than during if().

Done.

https://codereview.chromium.org/22911027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:242:
stats_table->GetSharedMemoryHandle()));
On 2013/08/21 01:09:46, jar wrote:
> nit: indent

Done.

[jar (doing other things), 2013-08-22 00:29:38]

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.cc
File base/metrics/stats_table.cc (right):

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.c...
base/metrics/stats_table.cc:109: int max_threads, int max_counters);
nit: fix indentation.  Chromium standard for declarations and definitions that
have to wrap is to put one arg per line.

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.c...
base/metrics/stats_table.cc:169: };
nit: Probably NO_DEFAULT_COPY_OR_ASSIGN(...)

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.c...
base/metrics/stats_table.cc:183: scoped_ptr<Private> priv(new
Private(shared_memory.release()));
nit: Style: Don't abbreviate in names.  Sadly, the word "private" is a reserved
word... so perhaps you can choose a better name for the class, and then for the
instance.

Possible choices (you can think of others that are better):
Internal
Helper

[rmcilroy, 2013-08-22 10:03:55]

Thanks for the review. PTAL.

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.cc
File base/metrics/stats_table.cc (right):

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.c...
base/metrics/stats_table.cc:109: int max_threads, int max_counters);
On 2013/08/22 00:29:38, jar wrote:
> nit: fix indentation.  Chromium standard for declarations and definitions that
> have to wrap is to put one arg per line.

Done.

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.c...
base/metrics/stats_table.cc:169: };
On 2013/08/22 00:29:38, jar wrote:
> nit: Probably NO_DEFAULT_COPY_OR_ASSIGN(...)

Done (DISALLOW_COPY_AND_ASSIGN)

https://codereview.chromium.org/22911027/diff/7001/base/metrics/stats_table.c...
base/metrics/stats_table.cc:183: scoped_ptr<Private> priv(new
Private(shared_memory.release()));
On 2013/08/22 00:29:38, jar wrote:
> nit: Style: Don't abbreviate in names.  Sadly, the word "private" is a
reserved
> word... so perhaps you can choose a better name for the class, and then for
the
> instance.
> 
> Possible choices (you can think of others that are better):
> Internal
> Helper

Done (went with Internal)

[rmcilroy, 2013-08-22 10:03:57]

Thanks for the review. PTAL.

[jar (doing other things), 2013-08-23 00:24:03]

base.* LGTM   

There is a nit below, and you'll need Jam's review for content.

https://codereview.chromium.org/22911027/diff/17001/content/browser/child_pro...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/22911027/diff/17001/content/browser/child_pro...
content/browser/child_process_launcher.cc:215: files_to_register.push_back(
nit: <sadly>: When the body for an if() extends across more than one line (lines
215-217), you need to put curlies around it.  This means you'll need to wrap 214
some more to make room for the curly (sorry).

https://codereview.chromium.org/22911027/diff/17001/content/browser/child_pro...
content/browser/child_process_launcher.cc:241:
base::SharedMemory::IsHandleValid(stats_table->GetSharedMemoryHandle()))
nit: add curlies here too.

[rmcilroy, 2013-08-23 17:03:30]

Thanks Jar.

Jam, could you have a look at content/ please.

https://codereview.chromium.org/22911027/diff/17001/content/browser/child_pro...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/22911027/diff/17001/content/browser/child_pro...
content/browser/child_process_launcher.cc:215: files_to_register.push_back(
On 2013/08/23 00:24:04, jar wrote:
> nit: <sadly>: When the body for an if() extends across more than one line
(lines
> 215-217), you need to put curlies around it.  This means you'll need to wrap
214
> some more to make room for the curly (sorry).

Dang, thought that might be the case :).  Done

https://codereview.chromium.org/22911027/diff/17001/content/browser/child_pro...
content/browser/child_process_launcher.cc:241:
base::SharedMemory::IsHandleValid(stats_table->GetSharedMemoryHandle()))
On 2013/08/23 00:24:04, jar wrote:
> nit: add curlies here too.

Done.

[jam, 2013-08-25 16:22:24]

https://codereview.chromium.org/22911027/diff/30001/content/browser/child_pro...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/22911027/diff/30001/content/browser/child_pro...
content/browser/child_process_launcher.cc:241: base::StatsTable* stats_table =
base::StatsTable::current();
we need to have one copy of this code, not two, and also same for lines 235-240
which are also duplicated. up to you whether this is done by combining the
ifdefs for android and posix and adding android ifdefs inside of that, or using
a helper function.

[jam, 2013-08-25 16:27:20]

https://codereview.chromium.org/22911027/diff/30001/ipc/ipc_descriptors.h
File ipc/ipc_descriptors.h (right):

https://codereview.chromium.org/22911027/diff/30001/ipc/ipc_descriptors.h#new...
ipc/ipc_descriptors.h:12: kStatsTableSharedMemFd = 100,
this seems a bit hacky. please copy what chrome/content use, i.e. how content
defines  kContentIPCDescriptorMax and chrome begins after that. so here we
should have  kIPCDescriptorMax that content keys off, since the code assumed so
far that IPC layer only had one fd

[rmcilroy, 2013-08-27 14:13:06]

Thanks for the review, PTAL.

https://codereview.chromium.org/22911027/diff/30001/content/browser/child_pro...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/22911027/diff/30001/content/browser/child_pro...
content/browser/child_process_launcher.cc:241: base::StatsTable* stats_table =
base::StatsTable::current();
On 2013/08/25 16:22:25, jam wrote:
> we need to have one copy of this code, not two, and also same for lines
235-240
> which are also duplicated. up to you whether this is done by combining the
> ifdefs for android and posix and adding android ifdefs inside of that, or
using
> a helper function.

Done (combined ifdefs).

https://codereview.chromium.org/22911027/diff/30001/ipc/ipc_descriptors.h
File ipc/ipc_descriptors.h (right):

https://codereview.chromium.org/22911027/diff/30001/ipc/ipc_descriptors.h#new...
ipc/ipc_descriptors.h:12: kStatsTableSharedMemFd = 100,
On 2013/08/25 16:27:20, jam wrote:
> this seems a bit hacky. please copy what chrome/content use, i.e. how content
> defines  kContentIPCDescriptorMax and chrome begins after that. so here we
> should have  kIPCDescriptorMax that content keys off, since the code assumed
so
> far that IPC layer only had one fd

You're right, I had meant to ask where ipc descriptors were being registered and
didn't know of content_descriptors.h.

Done.

[jam, 2013-08-27 16:22:12]

lgtm

[commit-bot: I haz the power, 2013-08-28 09:39:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/22911027/40001

[commit-bot: I haz the power, 2013-08-28 12:34:17]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-28 12:59:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/22911027/40001

[commit-bot: I haz the power, 2013-08-28 15:34:38]

Failed to trigger a try job on linux_chromeos
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-28 15:39:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/22911027/64001

[commit-bot: I haz the power, 2013-08-28 17:01:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/22911027/64001

[commit-bot: I haz the power, 2013-08-28 21:37:39]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[rmcilroy, 2013-08-29 16:43:01]

Brad: Could you have a look at the changes in components/nacl here please.  This
change adds a new ipc descriptor which upsets some of the hard-coded assumptions
in NaclForkDelegate.  I'm not familiar enough with the code to know whether
these changes are right, but they do get nacl working again.

A couple of points:
 - I had to change kNaClSandboxDescriptor, but that makes the requirement that
it match kMagicSandboxIPCDescriptor false (and changing
kMagicSandboxIPCDescriptor to be 6 crashes everything).  Instead, I think the
requirement might actually be that kNaClSandboxDescriptor matches
kSandboxIPCChannel + base::GlobalDescriptors::kBaseDescriptor (see line 264 of
child_process_launcher.cc).  Does this sound like a possibility?
 - This CL also makes the size of child_fds variable depending upon whether the
stats_table is initialized or not. This makes the hard-coded indices into the
array, defined in nacl_helper_linux.h, vary depending upon the size of fd array.
 I've changed this to a macro to get around this, but am not very happy about
it.  Do you know if there is any way to avoid hard coding these indexes instead?

[rmcilroy, 2013-09-02 15:58:23]

Brad (Nelson), could you please have a look at the changes in the nacl code (it
seems Brad Chen has moved teams).  Please see the message above for some
comments about this change.

[bradn, 2013-09-03 16:43:01]

Sorry for the slow reply, was out last Friday.

I'm getting strange errors when trying to view the diffs: 
error: old chunk mismatch

Could you reupload?

Not sure offhand with the file descriptors. My recollection was that 3 gets
baked in more places that it should, but a lot of this predates me being
actively on nacl. I can track down some better reviewers for you but can we get
the patch upload sorted out first, so I've got full context? Seems to be
something wrong with patch 9.

[rmcilroy, 2013-09-04 17:20:19]

On 2013/09/03 16:43:01, bradn wrote:
> Sorry for the slow reply, was out last Friday.
> 
> I'm getting strange errors when trying to view the diffs: 
> error: old chunk mismatch
> 
> Could you reupload?
> 
> Not sure offhand with the file descriptors. My recollection was that 3 gets
> baked in more places that it should, but a lot of this predates me being
> actively on nacl. I can track down some better reviewers for you but can we
get
> the patch upload sorted out first, so I've got full context? Seems to be
> something wrong with patch 9.

Weird, I've re-uploaded.  Let me know if you have problems with patch set 10.

I've also noticed that OutOfProcessPPAPITest.TrueTypeFont fails with this patch
(see
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/9757/ste...).
 I'm having a look into this, but if you have any ideas why this might be
failing that would be great?

[rmcilroy, 2013-09-11 09:29:14]

On 2013/09/04 17:20:19, rmcilroy wrote:
> On 2013/09/03 16:43:01, bradn wrote:
> > Sorry for the slow reply, was out last Friday.
> > 
> > I'm getting strange errors when trying to view the diffs: 
> > error: old chunk mismatch
> > 
> > Could you reupload?
> > 
> > Not sure offhand with the file descriptors. My recollection was that 3 gets
> > baked in more places that it should, but a lot of this predates me being
> > actively on nacl. I can track down some better reviewers for you but can we
> get
> > the patch upload sorted out first, so I've got full context? Seems to be
> > something wrong with patch 9.
> 
> Weird, I've re-uploaded.  Let me know if you have problems with patch set 10.
> 
> I've also noticed that OutOfProcessPPAPITest.TrueTypeFont fails with this
patch
> (see
>
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/9757/ste...).
>  I'm having a look into this, but if you have any ideas why this might be
> failing that would be great?

Ping?

[bradn, 2013-09-11 18:36:12]

+mseaborn for nacl part

[Mark Seaborn, 2013-09-12 00:00:27]

Can you fill out BUG=, please?  If there's no issue to reference, at least put
"BUG=none".

I'm missing some background here.  What information gets put into the stats
table?  What stats specifically?  I grepped for source files that #include
stats_table.h but it wasn't obvious.  Is this only used by renderers?  If so,
maybe we don't want to plumb this through to NaCl processes?

https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/na...
File components/nacl/common/nacl_helper_linux.h (right):

https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/na...
components/nacl/common/nacl_helper_linux.h:46: #define
kNaClDummyFDIndex(fd_array_size) (fd_array_size == 3 ? 1 : 2)
This should be an inline function rather than a macro -- functions are safer.

Also it should be renamed to remove the "k" prefix because it's not a constant
any more.

(But this assumes we want to plumb StatsTable through to NaCl processes -- see
other comment.)

[rmcilroy, 2013-09-12 17:04:09]

On 2013/09/12 00:00:27, Mark Seaborn wrote:
> Can you fill out BUG=, please?  If there's no issue to reference, at least put
> "BUG=none".
> 
> I'm missing some background here.  What information gets put into the stats
> table?  What stats specifically?  I grepped for source files that #include
> stats_table.h but it wasn't obvious.  Is this only used by renderers?  If so,
> maybe we don't want to plumb this through to NaCl processes?
> 
>
https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/na...
> File components/nacl/common/nacl_helper_linux.h (right):
> 
>
https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/na...
> components/nacl/common/nacl_helper_linux.h:46: #define
> kNaClDummyFDIndex(fd_array_size) (fd_array_size == 3 ? 1 : 2)
> This should be an inline function rather than a macro -- functions are safer.
> 
> Also it should be renamed to remove the "k" prefix because it's not a constant
> any more.
> 
> (But this assumes we want to plumb StatsTable through to NaCl processes -- see
> other comment.)

You can search for StatsCounter to find where counters are allocated that use
the stats table.  It is used by the browser and renderer, but also seems to be
used for npapi processes (content/child/npapi/plugin_lib.cc).  I think it would
be difficult to figure out in child_process_launcher.cc whether we are launching
a nacl process in order to special case nacl to avoid sending the file
descriptor (plus I think it might be a bad idea in case nacl is indirectly using
code in base, etc, which tries to increment StatsCounters, but I don't know nacl
enough to know whether this is a concern).  In any case, changes to NaCl will be
required due to the changes of content_descriptors.h.

[rmcilroy, 2013-09-12 17:04:55]

https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/na...
File components/nacl/common/nacl_helper_linux.h (right):

https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/na...
components/nacl/common/nacl_helper_linux.h:46: #define
kNaClDummyFDIndex(fd_array_size) (fd_array_size == 3 ? 1 : 2)
On 2013/09/12 00:00:28, Mark Seaborn wrote:
> This should be an inline function rather than a macro -- functions are safer.
> 
> Also it should be renamed to remove the "k" prefix because it's not a constant
> any more.
> 
> (But this assumes we want to plumb StatsTable through to NaCl processes -- see
> other comment.)

Done.

[jar (doing other things), 2013-09-12 17:11:40]

I believe it is also the case that stats tables are NOT enabled by default.
 They would represent too much of a security attack surface (or at least
they used to), and I don't believe they were hardened.  I think you need to
use command line flags to turn them on.

They can be used to gather stats from multiple processes, and can
accumulate data across multiple re-starts (since they use shared memory...
which the stats viewer may "keep alive" as Chrome comes and goes.



On Thu, Sep 12, 2013 at 10:04 AM, <rmcilroy@chromium.org> wrote:

> On 2013/09/12 00:00:27, Mark Seaborn wrote:
>
>> Can you fill out BUG=, please?  If there's no issue to reference, at
>> least put
>> "BUG=none".
>>
>
>  I'm missing some background here.  What information gets put into the
>> stats
>> table?  What stats specifically?  I grepped for source files that #include
>> stats_table.h but it wasn't obvious.  Is this only used by renderers?  If
>> so,
>> maybe we don't want to plumb this through to NaCl processes?
>>
>
>
> https://codereview.chromium.**org/22911027/diff/51001/**
>
components/nacl/common/nacl_**helper_linux.h<https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/nacl_helper_linux.h>
>
>> File components/nacl/common/nacl_**helper_linux.h (right):
>>
>
>
> https://codereview.chromium.**org/22911027/diff/51001/**
>
components/nacl/common/nacl_**helper_linux.h#newcode46<https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/nacl_helper_linux.h#newcode46>
>
>> components/nacl/common/nacl_**helper_linux.h:46: #define
>> kNaClDummyFDIndex(fd_array_**size) (fd_array_size == 3 ? 1 : 2)
>> This should be an inline function rather than a macro -- functions are
>> safer.
>>
>
>  Also it should be renamed to remove the "k" prefix because it's not a
>> constant
>> any more.
>>
>
>  (But this assumes we want to plumb StatsTable through to NaCl processes
>> -- see
>> other comment.)
>>
>
> You can search for StatsCounter to find where counters are allocated that
> use
> the stats table.  It is used by the browser and renderer, but also seems
> to be
> used for npapi processes (content/child/npapi/plugin_**lib.cc).  I think
> it would
> be difficult to figure out in child_process_launcher.cc whether we are
> launching
> a nacl process in order to special case nacl to avoid sending the file
> descriptor (plus I think it might be a bad idea in case nacl is indirectly
> using
> code in base, etc, which tries to increment StatsCounters, but I don't
> know nacl
> enough to know whether this is a concern).  In any case, changes to NaCl
> will be
> required due to the changes of content_descriptors.h.
>
>
>
https://codereview.chromium.**org/22911027/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmcilroy, 2013-09-23 08:34:28]

Ping - Mark?

On 2013/09/12 17:11:40, jar wrote:
> I believe it is also the case that stats tables are NOT enabled by default.
>  They would represent too much of a security attack surface (or at least
> they used to), and I don't believe they were hardened.  I think you need to
> use command line flags to turn them on.
> 
> They can be used to gather stats from multiple processes, and can
> accumulate data across multiple re-starts (since they use shared memory...
> which the stats viewer may "keep alive" as Chrome comes and goes.
> 
> 
> 
> On Thu, Sep 12, 2013 at 10:04 AM, <mailto:rmcilroy@chromium.org> wrote:
> 
> > On 2013/09/12 00:00:27, Mark Seaborn wrote:
> >
> >> Can you fill out BUG=, please?  If there's no issue to reference, at
> >> least put
> >> "BUG=none".
> >>
> >
> >  I'm missing some background here.  What information gets put into the
> >> stats
> >> table?  What stats specifically?  I grepped for source files that #include
> >> stats_table.h but it wasn't obvious.  Is this only used by renderers?  If
> >> so,
> >> maybe we don't want to plumb this through to NaCl processes?
> >>
> >
> >
> > https://codereview.chromium.**org/22911027/diff/51001/**
> >
>
components/nacl/common/nacl_**helper_linux.h<https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/nacl_helper_linux.h>
> >
> >> File components/nacl/common/nacl_**helper_linux.h (right):
> >>
> >
> >
> > https://codereview.chromium.**org/22911027/diff/51001/**
> >
>
components/nacl/common/nacl_**helper_linux.h#newcode46<https://codereview.chromium.org/22911027/diff/51001/components/nacl/common/nacl_helper_linux.h#newcode46>
> >
> >> components/nacl/common/nacl_**helper_linux.h:46: #define
> >> kNaClDummyFDIndex(fd_array_**size) (fd_array_size == 3 ? 1 : 2)
> >> This should be an inline function rather than a macro -- functions are
> >> safer.
> >>
> >
> >  Also it should be renamed to remove the "k" prefix because it's not a
> >> constant
> >> any more.
> >>
> >
> >  (But this assumes we want to plumb StatsTable through to NaCl processes
> >> -- see
> >> other comment.)
> >>
> >
> > You can search for StatsCounter to find where counters are allocated that
> > use
> > the stats table.  It is used by the browser and renderer, but also seems
> > to be
> > used for npapi processes (content/child/npapi/plugin_**lib.cc).  I think
> > it would
> > be difficult to figure out in child_process_launcher.cc whether we are
> > launching
> > a nacl process in order to special case nacl to avoid sending the file
> > descriptor (plus I think it might be a bad idea in case nacl is indirectly
> > using
> > code in base, etc, which tries to increment StatsCounters, but I don't
> > know nacl
> > enough to know whether this is a concern).  In any case, changes to NaCl
> > will be
> > required due to the changes of content_descriptors.h.
> >
> >
> >
>
https://codereview.chromium.**org/22911027/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2013-09-23 16:31:31]

On 23 September 2013 01:34, <rmcilroy@chromium.org> wrote:

> Ping - Mark?
>
> On 2013/09/12 17:11:40, jar wrote:
>
>> I believe it is also the case that stats tables are NOT enabled by
>> default.
>>   They would represent too much of a security attack surface (or at least
>> they used to), and I don't believe they were hardened.  I think you need
>> to
>> use command line flags to turn them on.
>>
>
>  They can be used to gather stats from multiple processes, and can
>> accumulate data across multiple re-starts (since they use shared memory...
>> which the stats viewer may "keep alive" as Chrome comes and goes.
>>
>
OK.  If the stats tables aren't enabled by default, how do they get tested
on the bots?  Is there a test that runs NaCl with "--enable-stats-table"?
Presumably there must be if that's how you found that NaCl's code path
needed updating for the stats tables?

Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmcilroy, 2013-09-23 19:26:26]

> OK.  If the stats tables aren't enabled by default, how do they get tested
> on the bots?  Is there a test that runs NaCl with "--enable-stats-table"?
> Presumably there must be if that's how you found that NaCl's code path
> needed updating for the stats tables?

I believe all chrome tests which are initialized via ChromeTestSuite::Initialize
(see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/c...
where the stats table is explicitly initialised).  I believe this occurs for
NaCl tests too (I could at least prevent the errors I was seeing previously by
commenting out this line)?

[Mark Seaborn, 2013-09-23 21:33:29]

On 23 September 2013 12:26, <rmcilroy@chromium.org> wrote:

>
>  OK.  If the stats tables aren't enabled by default, how do they get tested
>> on the bots?  Is there a test that runs NaCl with "--enable-stats-table"?
>> Presumably there must be if that's how you found that NaCl's code path
>> needed updating for the stats tables?
>>
>
> I believe all chrome tests which are initialized via
> ChromeTestSuite::Initialize
> (see
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/test/base/chrome_**test_suite.cc&q=stats%**
>
20package:%5Echromium$%20file:**(%5Esrc/chrome/test/)&dr=C&l=**285<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/chrome_test_suite.cc&q=stats%20package:%5Echromium$%20file:(%5Esrc/chrome/test/)&dr=C&l=285>
> where the stats table is explicitly initialised).  I believe this occurs
> for
> NaCl tests too (I could at least prevent the errors I was seeing
> previously by
> commenting out this line)?
>

Hmm.  That makes me uneasy, because it probably means that browser_tests
only ever tests the "--enable-stats-table" case, while the case we actually
deploy (where stats tables are disabled) is never tested by browser_tests.
Maybe the only testing of the stats-tables-disabled case occurs via the
nacl_integration step on the bots (which is deprecated).

So code like "fd_array_size == 3 ? 1 : 2" might easily get broken by future
changes if only one of its two code paths is tested on the bots.

You've probably noticed that the FD-handling code for nacl_helper is a bit
of a mess.  The origin of the problem is that the normal Chrome code path
(from child_process_launcher.cc) passes around an FD mapping (from FD
number to FD), but at the point where this is passed to nacl_helper
(ForkWithRealPid() in zygote_linux.cc), it gets converted to an array of
FDs.  The mapping keys are thrown away and so the ordering of the mapping's
entries become significant.

I can have a go at cleaning this up so that you don't need to change
nacl_helper_linux.{cc,h}...

Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmcilroy_google, 2013-09-24 08:22:43]

> Hmm.  That makes me uneasy, because it probably means that browser_tests
only ever tests the "--enable-stats-table" case, while the case we actually
deploy (where stats tables are disabled) is never tested by browser_tests.
Maybe the only testing of the stats-tables-disabled case occurs via the
nacl_integration step on the bots (which is deprecated).
>
> So code like "fd_array_size == 3 ? 1 : 2" might easily get broken by
future changes if only one of its two code paths is tested on the bots.
>
> You've probably noticed that the FD-handling code for nacl_helper is a
bit of a mess.  The origin of the problem is that the normal Chrome code
path (from child_process_launcher.cc) passes around an FD mapping (from FD
number to FD), but at the point where this is passed to nacl_helper
(ForkWithRealPid() in zygote_linux.cc), it gets converted to an array of
FDs.  The mapping keys are thrown away and so the ordering of the mapping's
entries become significant.
>
> I can have a go at cleaning this up so that you don't need to change
nacl_helper_linux.{cc,h}.

Sure, I would much prefer that this was cleaned up properly so that changes
like this didn't need to modify nacl_helper_linux. If you are willing to do
this yourself that would be great, otherwise if you let me know what you
had in mind please let me know and I can try and put together a cl.

Cheers
Ross

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2013-09-24 22:31:01]

On 24 September 2013 01:22, Ross McIlroy <rmcilroy@google.com> wrote:

> > Hmm.  That makes me uneasy, because it probably means that browser_tests
> only ever tests the "--enable-stats-table" case, while the case we actually
> deploy (where stats tables are disabled) is never tested by browser_tests.
> Maybe the only testing of the stats-tables-disabled case occurs via the
> nacl_integration step on the bots (which is deprecated).
> >
> > So code like "fd_array_size == 3 ? 1 : 2" might easily get broken by
> future changes if only one of its two code paths is tested on the bots.
> >
> > You've probably noticed that the FD-handling code for nacl_helper is a
> bit of a mess.  The origin of the problem is that the normal Chrome code
> path (from child_process_launcher.cc) passes around an FD mapping (from FD
> number to FD), but at the point where this is passed to nacl_helper
> (ForkWithRealPid() in zygote_linux.cc), it gets converted to an array of
> FDs.  The mapping keys are thrown away and so the ordering of the mapping's
> entries become significant.
> >
> > I can have a go at cleaning this up so that you don't need to change
> nacl_helper_linux.{cc,h}.
>
> Sure, I would much prefer that this was cleaned up properly so that
> changes like this didn't need to modify nacl_helper_linux. If you are
> willing to do this yourself that would be great, otherwise if you let me
> know what you had in mind please let me know and I can try and put together
> a cl.
>
I've prepared a cleanup: https://codereview.chromium.org/24449002/  I'm
running it through the trybots now (though they seem to be slow to respond
today), and I'll send it out for review when it passes.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2013-10-12 00:02:16]

On 24 September 2013 15:30, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 24 September 2013 01:22, Ross McIlroy <rmcilroy@google.com> wrote:
>
>> Sure, I would much prefer that this was cleaned up properly so that
>> changes like this didn't need to modify nacl_helper_linux. If you are
>> willing to do this yourself that would be great, otherwise if you let me
>> know what you had in mind please let me know and I can try and put together
>> a cl.
>
> I've prepared a cleanup: https://codereview.chromium.org/24449002/  I'm
> running it through the trybots now (though they seem to be slow to respond
> today), and I'll send it out for review when it passes.
>

I've committed that change, so you shouldn't need any NaCl-related changes
in this change any more.

Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmcilroy, 2013-10-14 08:26:03]

> I've committed that change, so you shouldn't need any NaCl-related changes
> in this change any more.

Awesome, thanks Mark! I've removed the nacl changes and will give it another go.

[rmcilroy, 2013-10-14 10:16:26]

On 2013/10/14 08:26:03, rmcilroy wrote:
> > I've committed that change, so you shouldn't need any NaCl-related changes
> > in this change any more.
> 
> Awesome, thanks Mark! I've removed the nacl changes and will give it another
go.

Mark: This does require one change in NaCl code to change the
kNaClSandboxDescriptor based on the ipc/content_descriptors.h.  Could you please
review the changes in nacl_helper_linux.h.

[Mark Seaborn, 2013-10-14 22:01:49]

https://codereview.chromium.org/22911027/diff/141001/components/nacl/common/n...
File components/nacl/common/nacl_helper_linux.h (right):

https://codereview.chromium.org/22911027/diff/141001/components/nacl/common/n...
components/nacl/common/nacl_helper_linux.h:34: // kSandboxIPCChannel +
base::GlobalDescriptors::kBaseDescriptor
Would you mind removing kNaClSandboxDescriptor from this file and changing
components/nacl/zygote/nacl_fork_delegate_linux.cc to use kSandboxIPCChannel +
base::GlobalDescriptors::kBaseDescriptor instead of kNaClSandboxDescriptor?

It's just a small cleanup now, but I think it's better to clean that up than
document that two parts of the code have to be kept in sync manually.

https://codereview.chromium.org/22911027/diff/141001/components/nacl/common/n...
components/nacl/common/nacl_helper_linux.h:40: #define kNaClBrowserFDIndex 0
BTW, it looks like you haven't rebased, because I removed this block in trunk. 
Presumably you've only tested this via the trybots and not locally, which is why
this works.

[rmcilroy, 2013-10-15 18:47:49]

https://codereview.chromium.org/22911027/diff/141001/components/nacl/common/n...
File components/nacl/common/nacl_helper_linux.h (right):

https://codereview.chromium.org/22911027/diff/141001/components/nacl/common/n...
components/nacl/common/nacl_helper_linux.h:34: // kSandboxIPCChannel +
base::GlobalDescriptors::kBaseDescriptor
On 2013/10/14 22:01:50, Mark Seaborn wrote:
> Would you mind removing kNaClSandboxDescriptor from this file and changing
> components/nacl/zygote/nacl_fork_delegate_linux.cc to use kSandboxIPCChannel +
> base::GlobalDescriptors::kBaseDescriptor instead of kNaClSandboxDescriptor?
> 
> It's just a small cleanup now, but I think it's better to clean that up than
> document that two parts of the code have to be kept in sync manually.

Done.

https://codereview.chromium.org/22911027/diff/141001/components/nacl/common/n...
components/nacl/common/nacl_helper_linux.h:40: #define kNaClBrowserFDIndex 0
On 2013/10/14 22:01:50, Mark Seaborn wrote:
> BTW, it looks like you haven't rebased, because I removed this block in trunk.

> Presumably you've only tested this via the trybots and not locally, which is
why
> this works.

Strange, I thought I had synced.  It did work on the trybots though (and I
thought locally too...). Done.

[Mark Seaborn, 2013-10-15 20:59:45]

LGTM, thanks

https://codereview.chromium.org/22911027/diff/152001/components/nacl/common/n...
File components/nacl/common/nacl_helper_linux.h (right):

https://codereview.chromium.org/22911027/diff/152001/components/nacl/common/n...
components/nacl/common/nacl_helper_linux.h:29: #endif  //
COMPONENTS_NACL_COMMON_NACL_HELPER_LINUX_H_
Nit: keep an empty line before this #endif

[commit-bot: I haz the power, 2013-10-16 09:12:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/22911027/162001

[commit-bot: I haz the power, 2013-10-16 11:59:30]

Change committed as 228903