Stats for the renderers are all broken, because of this change:...

Stats for the renderers are all broken, because of this change:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/app/chrome_dll_main.cc?r1=6627&r2=6626&pathrev=6627
line 236
We are appending the pid to the name of the shared memory object, 
which causes each renderer to get its own uninitialized copy of the 
stats counters, breaking the sharing. So then the about:stats code
can't find the counters created and updated by these processes. This
includes all the V8 counters and timers.

BUG=8311

[pink (ping after 24hrs), 2009-03-03 15:45:10]

I'd like jrg to confirm that cleaning up shmem segments when the app goes away
is now totally solid and fixed. The problem we had before was that they
wouldn't, and we'd get stuck trying to re-use a horked shmem segment the next
time we started so the app could never be re-launched.

If jrg says it's ok, I'm ok with it. LGTM.

[John Grabowski, 2009-03-03 19:11:21]

On 2009/03/03 15:45:10, pink wrote:
> I'd like jrg to confirm that cleaning up shmem segments when the app goes away
> is now totally solid and fixed. The problem we had before was that they
> wouldn't, and we'd get stuck trying to re-use a horked shmem segment the next
> time we started so the app could never be re-launched.
> 
> If jrg says it's ok, I'm ok with it. LGTM.

It'll be cleaned up in the sense that the in-core shmem will be gone.  However,
for the use here, there will still be a backing file on disk that persists. 
Thus, if Chromium crashes and is launched again, stats will continue.  

Is that acceptable?  If yes, please remove the TODO(port) comment.  If no, we'll
need an call somewhere to shared_memory_.Delete(chrome::kStatsFilename).  (We
can't just add it to StatsTable since I don't think we know there if it's in the
browser or renderer).

If you like, let me know if that is needed and I'll do it.


Dave / Mike, is there a way to know this is

[John Grabowski, 2009-03-03 19:12:19]

On 2009/03/03 15:45:10, pink wrote:
> I'd like jrg to confirm that cleaning up shmem segments when the app goes away
> is now totally solid and fixed. The problem we had before was that they
> wouldn't, and we'd get stuck trying to re-use a horked shmem segment the next
> time we started so the app could never be re-launched.
> 
> If jrg says it's ok, I'm ok with it. LGTM.

It'll be cleaned up in the sense that the in-core shmem will be gone.  However,
for the use here, there will still be a backing file on disk that persists. 
Thus, if Chromium crashes and is launched again, stats will continue.  

Is that acceptable?  If yes, please remove the TODO(port) comment.  If no, we'll
need an call somewhere to shared_memory_.Delete(chrome::kStatsFilename).  (We
can't just add it to StatsTable since I don't think we know there if it's in the
browser or renderer).

If you like, let me know if that is needed and I'll do it.


Dave / Mike, is there a way to know this is

[John Grabowski, 2009-03-03 19:21:06]

> On Tue, Mar 3, 2009 at 11:15 AM, Mike Pinkerton <pinkerton@chromium.org>
wrote:
> The problem we were running into before was this:
> 
> - app starts up with shmem for stats and it somehow gets corrupted
> - app crashes and can't clean it up
> - next launch of the app was DOA because stats table was corrupted.
> Reboot required.
> 
> I understand my "fix" probably wasn't correct (sorry!) but how do we
> get around this fail case?

Add an extra arg to StatsTable's constructor, called "bool master".
Set it to true when called from ChromeMain().
If set to true, StatsTable can call SharedMemory.Delete(name) before doing
anything else.
Do NOT set master to true when called from RendererMain().

Seem reasonable?

[DaveMoore, 2009-03-05 11:11:41]

I've taken the most direct approach to get the counters working under Windows
again. Instead of using the current pid as part of the name I use the browser's
pid. This should avoid the corruption problem as well as ensure that if you have
multiple copies of Chrome running they won't share their stats counters.

Since Windows doesn't need the Delete() and I can't test Mac I've left the rest
as is. If we're leaking the share files a separate bug should be created for
that.

An alternative approach to explicitly calling Delete() would be to create the
memory as an unnamed share and then pass the memory handle to the renderer
process.

Also, the same change that broke renderer stats counters broke the external
stats_viewer application. This does *not* fix that. I'll create a separate bug
about that...I think we'll just be able to remove that application as I don't
believe it's used. After all, it's been broken for 3 months and I didn't hear
anyone scream.

[John Grabowski, 2009-03-05 18:04:00]

> I've taken the most direct approach to get the counters working under Windows
> again. Instead of using the current pid as part of the name I use the
browser's
> pid. This should avoid the corruption problem as well as ensure that if you
have
> multiple copies of Chrome running they won't share their stats counters.

Seems like a good idea. 

> Since Windows doesn't need the Delete() and I can't test Mac I've left the
rest
> as is. If we're leaking the share files a separate bug should be created for
> that.

Dave, you absolutely can test Mac (and Linux).  Make the change, write a unit
test, then upload to the try server.  In general, ignoring non-Windows will be
problematic.

LGTM for checkin, but be sure to file the bug.