(Mac) Implement about:memory.

(Mac) Implement about:memory.

This implements about:memory on Mac. Moderately ridiculously, it calls
top (in logging mode) to obtain information about processes (ps offers
less information, and there aren't good -- or any, really -- APIs for
this on Mac).

Currently, we map the five available data (RPRVT, RSHRD, RSIZE, VPRVT, VSIZE) to the base::{CommittedKBytes, WorkingSetKBytes} structures which isn't entirely ideal. We then display the same about:memory page as Windows does, which may not be what we want either.

BUG=9653
TEST=Go to about:memory under a variety of conditions (with a variety of browsers loaded).

[viettrungluu-gmail, 2009-10-09 04:00:12]

It's a start, and Nico's task manager patch is gated on this....

[Amanda Walker, 2009-10-09 15:39:30]

LGTM with a couple of nits (the SIGPIPE question needs at least a TODO).  I
agree that parsing the output of top is unwieldy, but in the absence of a
supported API (and the repeated insistence from apple on darwin-dev@apple.com
that parsing the output of top and ps is how they want developers to get this
info), it's a reasonable initial implementation.

http://codereview.chromium.org/173261/diff/5001/5003
File base/process_util_posix.cc (right):

http://codereview.chromium.org/173261/diff/5001/5003#newcode603
Line 603: if (!success || exit_code != EXIT_SUCCESS)
Since we could close the child's stdout before it finishes if we've supplied an
output size limit, the child could also exit because it got a SIGPIPE when it
tried to write additional data.  We should probably treat this as a success,
since we got as much data as we asked for, even though the exit code may signify
a failure.

http://codereview.chromium.org/173261/diff/5001/5006
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/5001/5006#newcode348
Line 348: // do the opposite.
Add a brief explanation of why (mostly for the benefit of someone trying to rev
both implementations in the future).

[TVL, 2009-10-09 15:43:40]

some quick comments, I didn't read to closely incase the svn cp comment makes
sense, and that new diff would highlight existing code vs. new code.

http://codereview.chromium.org/173261/diff/5001/5006
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/5001/5006#newcode6
Line 6: // memory_details.cc (Windows-only); some merging is in order.
does it make sense to do a svn cp then so the history is linked and the changes
are obvious?

http://codereview.chromium.org/173261/diff/5001/5006#newcode314
Line 314: #endif
can you use chrome/common/chrome_constants?

[John Grabowski, 2009-10-09 16:52:49]

-1 on LGTM without a unit tests to 
  * confirm top output is what you expect
  * this function does anything useful at all (e.g. finds >0 Chrome processes)

http://codereview.chromium.org/173261/diff/5001/5002
File base/process_util.h (right):

http://codereview.chromium.org/173261/diff/5001/5002#newcode176
Line 176: bool SecureGetAppOutput(const CommandLine& cl,
Name makes me uncomfortable; it almost implies we've created a sandbox for
running the command line.  Can we call this BasicGetAppOutput() or something
like that?

http://codereview.chromium.org/173261/diff/5001/5003
File base/process_util_posix.cc (right):

http://codereview.chromium.org/173261/diff/5001/5003#newcode525
Line 525: static bool GetAppOutputInternal(const CommandLine& cl, char* const
envp[],
You nit test
You nit test

http://codereview.chromium.org/173261/diff/5001/5006
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/5001/5006#newcode81
Line 81: argv.push_back("/usr/bin/top");
Definitely required: a unit test to confirm the output format of top hasn't
changed.  Thus a dev can run the unit test and be pretty sure things will work.

How about a unit test that this function does anything useful at all?  Windows
just calls a system API so there is far less to test.

http://codereview.chromium.org/173261/diff/5001/5006#newcode311
Line 311: { google_browser_name.c_str(), L"Google Chrome", },
How do you make sure this doesn't pick up Chrome/Chromium processes which are
NOT part of this one?  E.g. I run Chromium.app but also have a
"Chromium.app/Contents/MacOS/Chromium --user-data-dir=/Blah" being run on the
same machine?

[viettrungluu-gmail, 2009-10-09 17:37:03]

I still want to make a unit test. I'll ping when that's done.

http://codereview.chromium.org/173261/diff/5001/5002
File base/process_util.h (right):

http://codereview.chromium.org/173261/diff/5001/5002#newcode176
Line 176: bool SecureGetAppOutput(const CommandLine& cl,
On 2009/10/09 16:52:49, John Grabowski wrote:
> Name makes me uncomfortable; it almost implies we've created a sandbox for
> running the command line.  Can we call this BasicGetAppOutput() or something
> like that?  

Fair enough, though I'm at a loss for a name. "Basic" just doesn't feel very
descriptive. Any other ideas? (Hmm. "Restricted"? Nah.)

http://codereview.chromium.org/173261/diff/5001/5003
File base/process_util_posix.cc (right):

http://codereview.chromium.org/173261/diff/5001/5003#newcode525
Line 525: static bool GetAppOutputInternal(const CommandLine& cl, char* const
envp[],
On 2009/10/09 16:52:49, John Grabowski wrote:
> You nit test
> You nit test

Sure, I'll get on it. You should also chant that at the author of the original
|GetAppOutput()|. ;-)

http://codereview.chromium.org/173261/diff/5001/5003#newcode603
Line 603: if (!success || exit_code != EXIT_SUCCESS)
On 2009/10/09 15:39:30, Amanda Walker wrote:
> Since we could close the child's stdout before it finishes if we've supplied
an
> output size limit, the child could also exit because it got a SIGPIPE when it
> tried to write additional data.  We should probably treat this as a success,
> since we got as much data as we asked for, even though the exit code may
signify
> a failure.

Right. Probably we should declare success if |!output_left && bytes_read > 0|.
Done.

http://codereview.chromium.org/173261/diff/5001/5006
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/5001/5006#newcode6
Line 6: // memory_details.cc (Windows-only); some merging is in order.
On 2009/10/09 15:43:40, TVL wrote:
> does it make sense to do a svn cp then so the history is linked and the
changes
> are obvious?

Oops, stale comment. It went stale with the Linux about:memory patch which
separated the platform-specific code. Sorry.

http://codereview.chromium.org/173261/diff/5001/5006#newcode81
Line 81: argv.push_back("/usr/bin/top");
On 2009/10/09 16:52:49, John Grabowski wrote:
> Definitely required: a unit test to confirm the output format of top hasn't
> changed.  Thus a dev can run the unit test and be pretty sure things will
work.
> 
> How about a unit test that this function does anything useful at all?  Windows
> just calls a system API so there is far less to test.

Sure, I'll get on it.

http://codereview.chromium.org/173261/diff/5001/5006#newcode311
Line 311: { google_browser_name.c_str(), L"Google Chrome", },
On 2009/10/09 16:52:49, John Grabowski wrote:
> How do you make sure this doesn't pick up Chrome/Chromium processes which are
> NOT part of this one?  E.g. I run Chromium.app but also have a
> "Chromium.app/Contents/MacOS/Chromium --user-data-dir=/Blah" being run on the
> same machine?

It picks it up, but is eliminated in
|MemoryDetails::CollectChildInfoOnUIThread()|, like on other platforms. Not so
elegant, I know. In general (on any platform, I think -- but definitely also on
Windows), |MemoryDetails| doesn't handle the possibility of multiple instances
of the same kind of browser very well.

http://codereview.chromium.org/173261/diff/5001/5006#newcode314
Line 314: #endif
On 2009/10/09 15:43:40, TVL wrote:
> can you use chrome/common/chrome_constants?

Yes, good idea. Done.

http://codereview.chromium.org/173261/diff/5001/5006#newcode348
Line 348: // do the opposite.
On 2009/10/09 15:39:30, Amanda Walker wrote:
> Add a brief explanation of why (mostly for the benefit of someone trying to
rev
> both implementations in the future).

Done.

[Amanda Walker, 2009-10-09 17:41:17]

On 2009/10/09 17:37:03, viettrungluu-gmail wrote:
> Fair enough, though I'm at a loss for a name. "Basic" just doesn't feel very
> descriptive. Any other ideas? (Hmm. "Restricted"? Nah.)

"Restricted" isn't bad, actually.  Or perhaps "Specific"?

[viettrungluu-gmail, 2009-10-09 22:37:04]

Okay, I renamed |ProcessInfoFromTop| to |ProcessInfoSnapshot| (mentioning top
seemed a bit specific) and split it off into its own bunch of files. (Possibly
the files should go in base/ though?) I also wrote a unit test for it, which
will verify that it can get output from top and at least pick up the processes
(it seems hard to check that the memory stats are being parsed correctly; maybe
I should make a mock-top).

I also renamed |SecureGetAppOutput()| to |GetAppOutputRestricted()| (a "suffix"
seems better to me). (It'd be tempting to just replace |GetAppOutput()| by
adding flags, but it's a cross-platform API.) I also added a unit test for it
(<red-face>it turns out |GetAppOutput()| itself had a unit test</red-face>).

Please re-review.... Thanks.

[TVL, 2009-10-12 16:53:15]

http://codereview.chromium.org/173261/diff/10008/9003
File base/process_util_posix.cc (right):

http://codereview.chromium.org/173261/diff/10008/9003#newcode533
Line 533: DCHECK(do_search_path || envp);
It took me a little to long to make sure this was right, would it be closer to
the comment to use '^' with one dcheck?
  DCHECK(do_search_path ^ envp);

http://codereview.chromium.org/173261/diff/10008/9004
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/173261/diff/10008/9004#newcode283
Line 283: argv.push_back("exit 0");   // argv[2]; "true"
"true"?

http://codereview.chromium.org/173261/diff/10008/9004#newcode289
Line 289: argv[2] = "exit 1";  // "false"
why updated argv, you are using CommandLine('/usr/bin/false') instead?

http://codereview.chromium.org/173261/diff/10008/9004#newcode301
Line 301: // Amount of output exactly greater than space allowed.
remove 'exactly' from comment?

http://codereview.chromium.org/173261/diff/10008/9004#newcode306
Line 306: // Amount of output exactly less than space allowed.
ditto?

http://codereview.chromium.org/173261/diff/10008/9007
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/10008/9007#newcode28
Line 28: class ProcessFilterTrue : public base::ProcessFilter {
you don't need this, the named iterator will give you everything if you don't
pass a filter.

http://codereview.chromium.org/173261/diff/10008/9007#newcode52
Line 52: static ProcessData g_process_template[MAX_BROWSERS];
all you use this for is arraysize within MemoryDetails::CollectProcessData,
can't that just use MAX_BROWSERS?

http://codereview.chromium.org/173261/diff/10008/9007#newcode107
Line 107: while(const ProcessEntry* process_entry =
process_it.NextProcessEntry())
space before ( (and below)

http://codereview.chromium.org/173261/diff/10008/9007#newcode111
Line 111: base::NamedProcessIterator subproc_it(L"Chromium Helper", &filter);
Need to use the constant for this to work for Chrome builds. A safer thing to do
would be to use a second filter that checks if the parent is the browser, this
way you aren't looking at the names at all for this loop.

Actually, the iterator on 102, should probably only be used for the non chrome
collections.  For the chrome ones, can you use a custom filter to
GetProcessCount and simply check if the process (or parent) is one of the ones
in child_info?  That way you don't have to worry about names, and you only
collect the ones you are told too?  (the filter wouldn't really be filtering as
much as it's looking at the data based and saving off the pids to then do you
collection on)

http://codereview.chromium.org/173261/diff/10008/9007#newcode151
Line 151: // this blank!)
file a bug?

http://codereview.chromium.org/173261/diff/10008/9007#newcode155
Line 155: FileVersionInfo::CreateFileVersionInfoForCurrentModule());
fyi - the code will collect other running Chromes/Chromiums which don't have to
be the same version number as the running on.

http://codereview.chromium.org/173261/diff/10008/9008
File chrome/browser/process_info_snapshot.h (right):

http://codereview.chromium.org/173261/diff/10008/9008#newcode5
Line 5: #include <map>
need include guards at the top/bottom

http://codereview.chromium.org/173261/diff/10008/9009
File chrome/browser/process_info_snapshot_mac.cc (right):

http://codereview.chromium.org/173261/diff/10008/9009#newcode53
Line 53: argv.push_back("-U");       // -U <uid> -- display processes of UID
only
this doesn't actually work on 10.6, -U is an alias for -user and only takes
names, not numbers.

http://codereview.chromium.org/173261/diff/10008/9009#newcode63
Line 63: if (!base::GetAppOutputRestricted(command_line, &output, max_procs *
200u)) {
why times 200?  should this just be +200 to allow for the headers?  maybe make a
constant about what it is for?

http://codereview.chromium.org/173261/diff/10008/9009#newcode78
Line 78: proc_info_entries_.erase(it);
seems like it would be more efficient to remove this and simply clear the map
after the loop?

http://codereview.chromium.org/173261/diff/10008/9009#newcode120
Line 120: bool ProcessInfoSnapshot::GetWorkingSetKBytesOfPID(
seems like it would be better to rework to core code to be able to collect both
(ie-add GetCommittedKBytesOfPID) of these in one call on the Mac, otherwise you
are spawning at lot of processes as part of the collection.

[Mark Mentovai, 2009-10-12 18:14:55]

http://codereview.chromium.org/173261/diff/10008/9003
File base/process_util_posix.cc (right):

http://codereview.chromium.org/173261/diff/10008/9003#newcode531
Line 531: // Either |do_search_path| should be false (x)or |envp| should be
null.
(x)or made no sense to me.  Do you mean or or xor?  Seems like you mean xor. 
Sometimes, it's better to just speak English.

"Either |do_search_path| should be false or |envp| should be NULL, but not
both."

[John Grabowski, 2009-10-12 20:20:41]

trung: it may appear like it, but we are not ganging up on you.  Collective
responsibility extends to reviewers; we would be remiss if we let anything slip
by.

http://codereview.chromium.org/173261/diff/10008/9002
File base/process_util.h (right):

http://codereview.chromium.org/173261/diff/10008/9002#newcode173
Line 173: // A secure version of |GetAppOutput()| which (a) clears the
environment, and
s/secure/restricted/g to reflect new func name

http://codereview.chromium.org/173261/diff/10008/9004
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/173261/diff/10008/9004#newcode274
Line 274: TEST_F(ProcessUtilTest, GetAppOutputRestricted) {
jrg <3 trung for unit tests

http://codereview.chromium.org/173261/diff/10008/9006
File chrome/browser/memory_details.h (right):

http://codereview.chromium.org/173261/diff/10008/9006#newcode114
Line 114: // process.
The Google C++ style guide recommends 
  void Foo(const string &in, string *out);
You don't appear to be following that here.

http://codereview.chromium.org/173261/diff/10008/9007
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/10008/9007#newcode28
Line 28: class ProcessFilterTrue : public base::ProcessFilter {
On 2009/10/12 16:53:15, TVL wrote:
> you don't need this, the named iterator will give you everything if you don't
> pass a filter.

Done.

http://codereview.chromium.org/173261/diff/10008/9007#newcode40
Line 40: CHROME_BROWSER = 0,
Add both CHROME and CHROMIUM

http://codereview.chromium.org/173261/diff/10008/9007#newcode52
Line 52: static ProcessData g_process_template[MAX_BROWSERS];
No need for global?  Why is local in MemoryDetails() not adequate?

http://codereview.chromium.org/173261/diff/10008/9007#newcode61
Line 61: // TODO(viettrungluu): This setup means that we can't detect both
Chrome and
Change that please; we'd like to see both.

http://codereview.chromium.org/173261/diff/10008/9007#newcode120
Line 120: // TODO(viettrungluu): properly get version/name info.
Seems important... please add or file bug as follow-up and ref bug # in here

http://codereview.chromium.org/173261/diff/10008/9007#newcode135
Line 135: ui_loop_->PostTask(FROM_HERE,
Why does this need to be on the main thread?
Why can't we get everything on this thread?

http://codereview.chromium.org/173261/diff/10008/9008
File chrome/browser/process_info_snapshot.h (right):

http://codereview.chromium.org/173261/diff/10008/9008#newcode52
Line 52: bool GetProcInfo(int pid,
Use of non-const ref args violates google C++ style

http://codereview.chromium.org/173261/diff/10008/9009
File chrome/browser/process_info_snapshot_mac.cc (right):

http://codereview.chromium.org/173261/diff/10008/9009#newcode53
Line 53: argv.push_back("-U");       // -U <uid> -- display processes of UID
only
On 2009/10/12 16:53:15, TVL wrote:
> this doesn't actually work on 10.6, -U is an alias for -user and only takes
> names, not numbers.

On next upload of code be sure to run unit tests on 10.6 to confirm this now
works.

[viettrungluu, 2009-10-14 05:33:05]

Work still in progress -- I've encountered some weirdness with |child_info| (as
in, it's empty).... Just thought I'd send this out before my replies got too
stale.

http://codereview.chromium.org/173261/diff/10008/9002
File base/process_util.h (right):

http://codereview.chromium.org/173261/diff/10008/9002#newcode173
Line 173: // A secure version of |GetAppOutput()| which (a) clears the
environment, and
On 2009/10/12 20:20:41, John Grabowski wrote:
> s/secure/restricted/g to reflect new func name
> 
> 

Done.

http://codereview.chromium.org/173261/diff/10008/9003
File base/process_util_posix.cc (right):

http://codereview.chromium.org/173261/diff/10008/9003#newcode531
Line 531: // Either |do_search_path| should be false (x)or |envp| should be
null.
On 2009/10/12 18:14:55, Mark Mentovai wrote:
> (x)or made no sense to me.  Do you mean or or xor?  Seems like you mean xor. 
> Sometimes, it's better to just speak English.
> 
> "Either |do_search_path| should be false or |envp| should be NULL, but not
> both."

Agreed (obviously a bad comment when I have to read the code to figure out what
I meant); changed.

http://codereview.chromium.org/173261/diff/10008/9003#newcode533
Line 533: DCHECK(do_search_path || envp);
On 2009/10/12 16:53:15, TVL wrote:
> It took me a little to long to make sure this was right, would it be closer to
> the comment to use '^' with one dcheck?
>   DCHECK(do_search_path ^ envp);

I changed it to:
  DCHECK(!do_search_path ^ !envp);
(which is what the comment was meant to say, i.e., "xor"). (This works, since we
end up taking the xor of two bools; C/C++ really should have a logical xor.)

http://codereview.chromium.org/173261/diff/10008/9004
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/173261/diff/10008/9004#newcode283
Line 283: argv.push_back("exit 0");   // argv[2]; "true"
On 2009/10/12 16:53:15, TVL wrote:
> "true"?

As in /usr/bin/true or /bin/true (I use /bin/sh since true may be in different
locations). I'll clarify the comment.

http://codereview.chromium.org/173261/diff/10008/9004#newcode289
Line 289: argv[2] = "exit 1";  // "false"
On 2009/10/12 16:53:15, TVL wrote:
> why updated argv, you are using CommandLine('/usr/bin/false') instead?

Oops. This fails (but not in a way which makes the unit test fail) on systems
where false is in /bin; it should be |argv|.

http://codereview.chromium.org/173261/diff/10008/9004#newcode301
Line 301: // Amount of output exactly greater than space allowed.
On 2009/10/12 16:53:15, TVL wrote:
> remove 'exactly' from comment?

Done. The root of all evil is not premature optimization, but overzealous
cutting-and-pasting.

http://codereview.chromium.org/173261/diff/10008/9004#newcode306
Line 306: // Amount of output exactly less than space allowed.
On 2009/10/12 16:53:15, TVL wrote:
> ditto?

Done.

http://codereview.chromium.org/173261/diff/10008/9006
File chrome/browser/memory_details.h (right):

http://codereview.chromium.org/173261/diff/10008/9006#newcode114
Line 114: // process.
On 2009/10/12 20:20:41, John Grabowski wrote:
> The Google C++ style guide recommends 
>   void Foo(const string &in, string *out);
> You don't appear to be following that here.

I'm not sure what in particular you're referring to here. They're actually all
inputs (with the state of the object being changed). I added back the name of
the variables, though (oops, WebKit style influence), and I changed the type of
the first one to |const std::vector<...>&|. (I'm keeping the |ProcessEntry| as a
pointer rather than a reference since |base::NamedProcessIterator| returns
pointers.)

http://codereview.chromium.org/173261/diff/10008/9007
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/10008/9007#newcode28
Line 28: class ProcessFilterTrue : public base::ProcessFilter {
On 2009/10/12 16:53:15, TVL wrote:
> you don't need this, the named iterator will give you everything if you don't
> pass a filter.

Okay, thanks.

http://codereview.chromium.org/173261/diff/10008/9007#newcode40
Line 40: CHROME_BROWSER = 0,
On 2009/10/12 20:20:41, John Grabowski wrote:
> Add both CHROME and CHROMIUM

The name of the constant is a little annoying (but cosmetic), but there are
problems when it comes to detecting other instances of Chrome/Chromium (see
below). Presumably, for the same reasons, I don't think the Windows and Linux
versions detect both Chrome and Chromium (or even other instances of the
browser).

http://codereview.chromium.org/173261/diff/10008/9007#newcode52
Line 52: static ProcessData g_process_template[MAX_BROWSERS];
On 2009/10/12 20:20:41, John Grabowski wrote:
> No need for global?  Why is local in MemoryDetails() not adequate?

Right.

http://codereview.chromium.org/173261/diff/10008/9007#newcode52
Line 52: static ProcessData g_process_template[MAX_BROWSERS];
On 2009/10/12 16:53:15, TVL wrote:
> all you use this for is arraysize within MemoryDetails::CollectProcessData,
> can't that just use MAX_BROWSERS?

You're right! I'm not sure how that train wreck got through. Actually, it's
cut-and-pasted straight from the Windows code. I'll file a bug.

http://codereview.chromium.org/173261/diff/10008/9007#newcode61
Line 61: // TODO(viettrungluu): This setup means that we can't detect both
Chrome and
On 2009/10/12 20:20:41, John Grabowski wrote:
> Change that please; we'd like to see both.

The problem is that, due to our multi-process architecture, gathering
information about all our helper processes is difficult (and we can only do so
easily for ourselves). Without being more sophisticated, we couldn't give good
numbers for both (at the same time). (We could look at process trees, i.e., PIDs
and PPIDs, but that's getting more complicated.) As it stands, the numbers even
for ourself aren't terrific (due to shared memory).

http://codereview.chromium.org/173261/diff/10008/9007#newcode84
Line 84: std::vector<ProcessMemoryInformation> child_info) {
Something seriously not kosher is going on. |child_info| is empty in the actual
call (from common code, in memory_details.cc), yet still somehow things kind of
work. I'm not sure yet whether this is Mac-specific or not; I'll investigate.
(If not, then it's only by a minor miracle that the code works on other
platforms.)

http://codereview.chromium.org/173261/diff/10008/9007#newcode107
Line 107: while(const ProcessEntry* process_entry =
process_it.NextProcessEntry())
On 2009/10/12 16:53:15, TVL wrote:
> space before ( (and below)

I've got to get my editor to warn me about this.

http://codereview.chromium.org/173261/diff/10008/9007#newcode111
Line 111: base::NamedProcessIterator subproc_it(L"Chromium Helper", &filter);
On 2009/10/12 16:53:15, TVL wrote:
> Need to use the constant for this to work for Chrome builds. A safer thing to
do
> would be to use a second filter that checks if the parent is the browser, this
> way you aren't looking at the names at all for this loop.
> 
> Actually, the iterator on 102, should probably only be used for the non chrome
> collections.  For the chrome ones, can you use a custom filter to
> GetProcessCount and simply check if the process (or parent) is one of the ones
> in child_info?  That way you don't have to worry about names, and you only
> collect the ones you are told too?  (the filter wouldn't really be filtering
as
> much as it's looking at the data based and saving off the pids to then do you
> collection on)

Working on this (when I ran into the strangeness with |child_info|)....

http://codereview.chromium.org/173261/diff/10008/9007#newcode120
Line 120: // TODO(viettrungluu): properly get version/name info.
On 2009/10/12 20:20:41, John Grabowski wrote:
> Seems important... please add or file bug as follow-up and ref bug # in here

Will do. Maybe the correct bug to file is "about:memory code needs to be
revamped", especially since its design is such that it also can't gather
information about multiple instances on Windows (off of which this code is
loosely based).

http://codereview.chromium.org/173261/diff/10008/9007#newcode135
Line 135: ui_loop_->PostTask(FROM_HERE,
On 2009/10/12 20:20:41, John Grabowski wrote:
> Why does this need to be on the main thread?
> Why can't we get everything on this thread?

This is explained in a comment in memory_details.cc: "The
RenderProcessHostIterator can only be accessed from the UI thread.". I'll add a
comment referring to that.

http://codereview.chromium.org/173261/diff/10008/9007#newcode151
Line 151: // this blank!)
On 2009/10/12 16:53:15, TVL wrote:
> file a bug?

Will do.

http://codereview.chromium.org/173261/diff/10008/9007#newcode155
Line 155: FileVersionInfo::CreateFileVersionInfoForCurrentModule());
On 2009/10/12 16:53:15, TVL wrote:
> fyi - the code will collect other running Chromes/Chromiums which don't have
to
> be the same version number as the running on.

Though theoretically that would have been filtered out in the common code (for
the child processes), which tosses out any |UNKNOWN_PROCESS|.... At least that
was the theory, though now I have to investigate why anything works at all.

http://codereview.chromium.org/173261/diff/10008/9008
File chrome/browser/process_info_snapshot.h (right):

http://codereview.chromium.org/173261/diff/10008/9008#newcode5
Line 5: #include <map>
On 2009/10/12 16:53:15, TVL wrote:
> need include guards at the top/bottom

Done.

http://codereview.chromium.org/173261/diff/10008/9008#newcode52
Line 52: bool GetProcInfo(int pid,
On 2009/10/12 20:20:41, John Grabowski wrote:
> Use of non-const ref args violates google C++ style

Makes sense; I'll change it.

[TVL, 2009-10-14 13:55:14]

fyi - your replies are probably in reference to local code changes, since some
of what you have "done" next to isn't changed in the last snapshot.  :)

[John Grabowski, 2009-10-14 18:33:14]

All your comments sound reasonable; let me know when you're ready for
re-review.
jrg


On Tue, Oct 13, 2009 at 10:33 PM, <viettrungluu@chromium.org> wrote:

> Work still in progress -- I've encountered some weirdness with |child_info|
> (as
> in, it's empty).... Just thought I'd send this out before my replies got
> too
> stale.
>
>
> http://codereview.chromium.org/173261/diff/10008/9002
> File base/process_util.h (right):
>
> http://codereview.chromium.org/173261/diff/10008/9002#newcode173
> Line 173: // A secure version of |GetAppOutput()| which (a) clears the
> environment, and
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> s/secure/restricted/g to reflect new func name
>>
>
>
>
> Done.
>
> http://codereview.chromium.org/173261/diff/10008/9003
> File base/process_util_posix.cc (right):
>
> http://codereview.chromium.org/173261/diff/10008/9003#newcode531
> Line 531: // Either |do_search_path| should be false (x)or |envp| should
> be null.
> On 2009/10/12 18:14:55, Mark Mentovai wrote:
>
>> (x)or made no sense to me.  Do you mean or or xor?  Seems like you
>>
> mean xor.
>
>> Sometimes, it's better to just speak English.
>>
>
>  "Either |do_search_path| should be false or |envp| should be NULL, but
>>
> not
>
>> both."
>>
>
> Agreed (obviously a bad comment when I have to read the code to figure
> out what I meant); changed.
>
> http://codereview.chromium.org/173261/diff/10008/9003#newcode533
> Line 533: DCHECK(do_search_path || envp);
> On 2009/10/12 16:53:15, TVL wrote:
>
>> It took me a little to long to make sure this was right, would it be
>>
> closer to
>
>> the comment to use '^' with one dcheck?
>>   DCHECK(do_search_path ^ envp);
>>
>
> I changed it to:
>  DCHECK(!do_search_path ^ !envp);
> (which is what the comment was meant to say, i.e., "xor"). (This works,
> since we end up taking the xor of two bools; C/C++ really should have a
> logical xor.)
>
> http://codereview.chromium.org/173261/diff/10008/9004
> File base/process_util_unittest.cc (right):
>
> http://codereview.chromium.org/173261/diff/10008/9004#newcode283
> Line 283: argv.push_back("exit 0");   // argv[2]; "true"
> On 2009/10/12 16:53:15, TVL wrote:
>
>> "true"?
>>
>
> As in /usr/bin/true or /bin/true (I use /bin/sh since true may be in
> different locations). I'll clarify the comment.
>
> http://codereview.chromium.org/173261/diff/10008/9004#newcode289
> Line 289: argv[2] = "exit 1";  // "false"
> On 2009/10/12 16:53:15, TVL wrote:
>
>> why updated argv, you are using CommandLine('/usr/bin/false') instead?
>>
>
> Oops. This fails (but not in a way which makes the unit test fail) on
> systems where false is in /bin; it should be |argv|.
>
> http://codereview.chromium.org/173261/diff/10008/9004#newcode301
> Line 301: // Amount of output exactly greater than space allowed.
> On 2009/10/12 16:53:15, TVL wrote:
>
>> remove 'exactly' from comment?
>>
>
> Done. The root of all evil is not premature optimization, but
> overzealous cutting-and-pasting.
>
> http://codereview.chromium.org/173261/diff/10008/9004#newcode306
> Line 306: // Amount of output exactly less than space allowed.
> On 2009/10/12 16:53:15, TVL wrote:
>
>> ditto?
>>
>
> Done.
>
> http://codereview.chromium.org/173261/diff/10008/9006
> File chrome/browser/memory_details.h (right):
>
> http://codereview.chromium.org/173261/diff/10008/9006#newcode114
> Line 114: // process.
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> The Google C++ style guide recommends
>>   void Foo(const string &in, string *out);
>> You don't appear to be following that here.
>>
>
> I'm not sure what in particular you're referring to here. They're
> actually all inputs (with the state of the object being changed). I
> added back the name of the variables, though (oops, WebKit style
> influence), and I changed the type of the first one to |const
> std::vector<...>&|. (I'm keeping the |ProcessEntry| as a pointer rather
> than a reference since |base::NamedProcessIterator| returns pointers.)
>
> http://codereview.chromium.org/173261/diff/10008/9007
> File chrome/browser/memory_details_mac.cc (right):
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode28
> Line 28: class ProcessFilterTrue : public base::ProcessFilter {
> On 2009/10/12 16:53:15, TVL wrote:
>
>> you don't need this, the named iterator will give you everything if
>>
> you don't
>
>> pass a filter.
>>
>
> Okay, thanks.
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode40
> Line 40: CHROME_BROWSER = 0,
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> Add both CHROME and CHROMIUM
>>
>
> The name of the constant is a little annoying (but cosmetic), but there
> are problems when it comes to detecting other instances of
> Chrome/Chromium (see below). Presumably, for the same reasons, I don't
> think the Windows and Linux versions detect both Chrome and Chromium (or
> even other instances of the browser).
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode52
> Line 52: static ProcessData g_process_template[MAX_BROWSERS];
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> No need for global?  Why is local in MemoryDetails() not adequate?
>>
>
> Right.
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode52
> Line 52: static ProcessData g_process_template[MAX_BROWSERS];
> On 2009/10/12 16:53:15, TVL wrote:
>
>> all you use this for is arraysize within
>>
> MemoryDetails::CollectProcessData,
>
>> can't that just use MAX_BROWSERS?
>>
>
> You're right! I'm not sure how that train wreck got through. Actually,
> it's cut-and-pasted straight from the Windows code. I'll file a bug.
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode61
> Line 61: // TODO(viettrungluu): This setup means that we can't detect
> both Chrome and
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> Change that please; we'd like to see both.
>>
>
> The problem is that, due to our multi-process architecture, gathering
> information about all our helper processes is difficult (and we can only
> do so easily for ourselves). Without being more sophisticated, we
> couldn't give good numbers for both (at the same time). (We could look
> at process trees, i.e., PIDs and PPIDs, but that's getting more
> complicated.) As it stands, the numbers even for ourself aren't terrific
> (due to shared memory).
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode84
> Line 84: std::vector<ProcessMemoryInformation> child_info) {
> Something seriously not kosher is going on. |child_info| is empty in the
> actual call (from common code, in memory_details.cc), yet still somehow
> things kind of work. I'm not sure yet whether this is Mac-specific or
> not; I'll investigate. (If not, then it's only by a minor miracle that
> the code works on other platforms.)
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode107
> Line 107: while(const ProcessEntry* process_entry =
> process_it.NextProcessEntry())
> On 2009/10/12 16:53:15, TVL wrote:
>
>> space before ( (and below)
>>
>
> I've got to get my editor to warn me about this.
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode111
> Line 111: base::NamedProcessIterator subproc_it(L"Chromium Helper",
> &filter);
> On 2009/10/12 16:53:15, TVL wrote:
>
>> Need to use the constant for this to work for Chrome builds. A safer
>>
> thing to do
>
>> would be to use a second filter that checks if the parent is the
>>
> browser, this
>
>> way you aren't looking at the names at all for this loop.
>>
>
>  Actually, the iterator on 102, should probably only be used for the
>>
> non chrome
>
>> collections.  For the chrome ones, can you use a custom filter to
>> GetProcessCount and simply check if the process (or parent) is one of
>>
> the ones
>
>> in child_info?  That way you don't have to worry about names, and you
>>
> only
>
>> collect the ones you are told too?  (the filter wouldn't really be
>>
> filtering as
>
>> much as it's looking at the data based and saving off the pids to then
>>
> do you
>
>> collection on)
>>
>
> Working on this (when I ran into the strangeness with |child_info|)....
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode120
> Line 120: // TODO(viettrungluu): properly get version/name info.
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> Seems important... please add or file bug as follow-up and ref bug #
>>
> in here
>
> Will do. Maybe the correct bug to file is "about:memory code needs to be
> revamped", especially since its design is such that it also can't gather
> information about multiple instances on Windows (off of which this code
> is loosely based).
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode135
> Line 135: ui_loop_->PostTask(FROM_HERE,
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> Why does this need to be on the main thread?
>> Why can't we get everything on this thread?
>>
>
> This is explained in a comment in memory_details.cc: "The
> RenderProcessHostIterator can only be accessed from the UI thread.".
> I'll add a comment referring to that.
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode151
> Line 151: // this blank!)
> On 2009/10/12 16:53:15, TVL wrote:
>
>> file a bug?
>>
>
> Will do.
>
> http://codereview.chromium.org/173261/diff/10008/9007#newcode155
> Line 155: FileVersionInfo::CreateFileVersionInfoForCurrentModule());
> On 2009/10/12 16:53:15, TVL wrote:
>
>> fyi - the code will collect other running Chromes/Chromiums which
>>
> don't have to
>
>> be the same version number as the running on.
>>
>
> Though theoretically that would have been filtered out in the common
> code (for the child processes), which tosses out any
> |UNKNOWN_PROCESS|.... At least that was the theory, though now I have to
> investigate why anything works at all.
>
> http://codereview.chromium.org/173261/diff/10008/9008
> File chrome/browser/process_info_snapshot.h (right):
>
> http://codereview.chromium.org/173261/diff/10008/9008#newcode5
> Line 5: #include <map>
> On 2009/10/12 16:53:15, TVL wrote:
>
>> need include guards at the top/bottom
>>
>
> Done.
>
> http://codereview.chromium.org/173261/diff/10008/9008#newcode52
> Line 52: bool GetProcInfo(int pid,
> On 2009/10/12 20:20:41, John Grabowski wrote:
>
>> Use of non-const ref args violates google C++ style
>>
>
> Makes sense; I'll change it.
>
>
> http://codereview.chromium.org/173261
>

[viettrungluu, 2009-10-15 22:28:30]

Argh. Apple completely revamped top for Snow Leopard, making it a very bad "API"
to use. (We could check versions, but the changes indicate that Apple considers
it to be a very mutable program, so it'll probably change again in the future.)

So ps it is (being POSIX standard, they can't change it). Unfortunately, ps only
gives two pieces of data: RSS and virtual memory size (the man page also
promises text size, but it's always zero!).

[TVL, 2009-10-15 22:44:18]

On Thu, Oct 15, 2009 at 6:28 PM, <viettrungluu@chromium.org> wrote:

> Argh. Apple completely revamped top for Snow Leopard, making it a very bad
> "API"
> to use. (We could check versions, but the changes indicate that Apple
> considers
> it to be a very mutable program, so it'll probably change again in the
> future.)
>
> So ps it is (being POSIX standard, they can't change it). Unfortunately, ps
> only
> gives two pieces of data: RSS and virtual memory size (the man page also
> promises text size, but it's always zero!).


on the plus side, you can collect the data for multiple pids in one run.  ;)
 chrome/test/chrome_process_util_mac.cc does this to get some easy parsing.


>
>
> http://codereview.chromium.org/173261
>

[viettrungluu, 2009-10-21 23:34:41]

I've rewritten this to use /bin/ps instead, and I think it's ready for
re-review. Do note that the stats for Chrome/Chromium now kind of suck though
since ps doesn't give any information about shared memory (I've filed a bug for
this).

[John Grabowski, 2009-10-22 20:57:06]

Mostly "add comments" but the NACL_PROCESS thing needs to be fixed.
Real close.

http://codereview.chromium.org/173261/diff/22001/23005
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/173261/diff/22001/23005#newcode211
Line 211: break;
Looks like you accidentally removed NACL_PROCESS.  Please add back in.

http://codereview.chromium.org/173261/diff/22001/23007
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/22001/23007#newcode88
Line 88: void MemoryDetails::CollectProcessData(
Might want to add a comment "runs in the FILE thread so doesn't cause jank".

http://codereview.chromium.org/173261/diff/22001/23007#newcode117
Line 117: // Get PIDs of helpers.
Safari on 10.6 can have helpers (e.g. out-of-proc Flash32).  Look for those? 
Generalize to have N names per browser?

http://codereview.chromium.org/173261/diff/22001/23007#newcode141
Line 141: // TODO(viettrungluu): properly get version/name info.
Why is this a TODO?  Fix for Beta?  File a bug and add # here?

http://codereview.chromium.org/173261/diff/22001/23008
File chrome/browser/process_info_snapshot.h (right):

http://codereview.chromium.org/173261/diff/22001/23008#newcode24
Line 24: // should be considered opaque).
Add brief comment explaining that top seems more useful but isn't portable
between 10.5 and 10.6 (and makes us nervous about the future).

http://codereview.chromium.org/173261/diff/22001/23010
File chrome/browser/process_info_snapshot_mac_unittest.cc (right):

http://codereview.chromium.org/173261/diff/22001/23010#newcode83
Line 83: }
Add a test which sanity checks your memory info.  E.g. compare our process
against a "/bin/sleep 1" and make sure it has a larger vsize.

[viettrungluu, 2009-10-23 02:14:49]

Somehow, I've messed things up and it won't upload properly anymore, so I'm
moving the patch here: <http://codereview.chromium.org/333008> (I can also
switch accounts at the same time). Sorry for the inconvenience.

http://codereview.chromium.org/173261/diff/22001/23005
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/173261/diff/22001/23005#newcode211
Line 211: break;
On 2009/10/22 20:57:06, John Grabowski wrote:
> Looks like you accidentally removed NACL_PROCESS.  Please add back in.

Oops. Bad merge. Fixed.

http://codereview.chromium.org/173261/diff/22001/23007
File chrome/browser/memory_details_mac.cc (right):

http://codereview.chromium.org/173261/diff/22001/23007#newcode32
Line 32: class ProcessFilterByPID : public base::ProcessFilter {
Oops. This class was no longer used. It's now gone.

http://codereview.chromium.org/173261/diff/22001/23007#newcode88
Line 88: void MemoryDetails::CollectProcessData(
On 2009/10/22 20:57:06, John Grabowski wrote:
> Might want to add a comment "runs in the FILE thread so doesn't cause jank".  

Done.

http://codereview.chromium.org/173261/diff/22001/23007#newcode117
Line 117: // Get PIDs of helpers.
On 2009/10/22 20:57:06, John Grabowski wrote:
> Safari on 10.6 can have helpers (e.g. out-of-proc Flash32).  Look for those? 
> Generalize to have N names per browser?

I'll leave the for the grand multi-platform refactoring (see TODO near the top
of the file). I'll add a specific note about this in the bug.

http://codereview.chromium.org/173261/diff/22001/23007#newcode141
Line 141: // TODO(viettrungluu): properly get version/name info.
On 2009/10/22 20:57:06, John Grabowski wrote:
> Why is this a TODO?  Fix for Beta?  File a bug and add # here?

It wasn't super-simple, and it isn't idiot-proof, but I've fixed this.

http://codereview.chromium.org/173261/diff/22001/23007#newcode184
Line 184: // this blank!)
This one was easy to fix, so I did.

http://codereview.chromium.org/173261/diff/22001/23008
File chrome/browser/process_info_snapshot.h (right):

http://codereview.chromium.org/173261/diff/22001/23008#newcode24
Line 24: // should be considered opaque).
On 2009/10/22 20:57:06, John Grabowski wrote:
> Add brief comment explaining that top seems more useful but isn't portable
> between 10.5 and 10.6 (and makes us nervous about the future).

Done.

http://codereview.chromium.org/173261/diff/22001/23010
File chrome/browser/process_info_snapshot_mac_unittest.cc (right):

http://codereview.chromium.org/173261/diff/22001/23010#newcode83
Line 83: }
On 2009/10/22 20:57:06, John Grabowski wrote:
> Add a test which sanity checks your memory info.  E.g. compare our process
> against a "/bin/sleep 1" and make sure it has a larger vsize.

I just hardcoded in some values which looked reasonable to me since spawning off
a process and measuring its memory use is tricky. (Moreover, on SL, 64-bit apps
-- like sleep -- have really large vsizes.)