Convert renderer JS memory usage reporting to use Mojo.

Convert renderer JS memory usage reporting to use Mojo.

As a result, removes the
NOTIFICATION_RENDERER_V8_HEAP_STATS_COMPUTED notification.

BUG=268984, 487927
Committed: https://crrev.com/77099c24ca1783490e38a2e2d44c7a524c50531b
Cr-Commit-Position: refs/heads/master@{#330692}

[Anand Mistry (off Chromium), 2015-05-13 03:58:58]

amistry@chromium.org changed reviewers:
+ afakhry@chromium.org, nick@chromium.org

[Anand Mistry (off Chromium), 2015-05-13 03:58:58]

This is a follow-up to my utility process reporting change. I figured there's no
point in having two mechanisms to report JS memory usage. This removes quite a
bit of plumbing as well as the IPC messages. It does increase the runtime cost
of about:memory-internals since the renderer has to create a new Mojo service
instance, but since that page is rare and the collection is user-initiated, I
thought it would be fine.

[ncarter (slow), 2015-05-14 06:28:13]

Nice.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/chrome_n...
File chrome/browser/chrome_notification_types.h (left):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_notification_types.h:262:
NOTIFICATION_RENDERER_V8_HEAP_STATS_COMPUTED,
:)

Make sure you mention BUG=268984 (Kill NotificationService) in your CL comment.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
File chrome/browser/process_resource_usage.cc (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
chrome/browser/process_resource_usage.cc:53: void
ProcessResourceUsage::OnConnectionError() {
Does it make sense to zero the |stats_| when the child process dies?

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
File chrome/browser/process_resource_usage.h (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
chrome/browser/process_resource_usage.h:55: void OnConnectionError() override;
Making this method private doesn't really hide it from clients of
ProcessResourceUsage -- because it's public in the base class. So I'd say these
ought to be public as well.

Since the paradigm here is "ProcessResourceUsage is implemented in terms of
mojo::ErrorHandler" instead of "ProcessResourceUsage is a kind of
mojo::ErrorHandler", you could hide these methods from clients of
ProcessResourceUsage by using composition instead of inheritance.

Generally, when I'm tempted to change the access specifier in the derived class
to be different from that in a base interface class, an alternative is to move
the interface implementation into a helper class of which the parent class has a
private member, and the helper class keeps a pointer back to the parent class if
needed. If you need to privately implement more than one interface, this can
become PIMPL, where all the data members from the parent type move into the
helper type.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc:161:
resource_usage_reporters_.clear();
Invalidating the weak pointers here seems like a good idea.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc:179:
DCHECK(iter != resource_usage_reporters_.end());
This code was pretty dicey to begin with, so we can wind up in this callback in
some pretty weird situations:

1. It's possible |content| is not a valid WC pointer when the refresh completes
-- the tab could have been closed.
2. The resource_usage_reporters_ map might have been cleared between Refresh and
now -- the user could have hit the button again.
3. Because of [2] it's possible that we get back here with an entry in
resource_usage_reporters_[content] that is a different object from the one
currently invoking the callback.

Although it sounds like a pretty quick race that would be hard to lose, the v8
stats will actually return after an arbitrary delay if we're executing an
expensive js function. I just confirmed this in the console using "for (var iii
= 0; iii < 3000000; iii++) {}" in one tab while trying to collecting
memory-internals in another.

The old code seemed to somehow be immune to crashes because, although it
maintained a set of WebContents pointers, it didn't ever dereference them
anytime after the set was first built. Request() happens during the same message
loop task as all the AddWebContents calls.

Some possible ways forward (short of totally reworking this class, which I'd be
okay with too):
 - Don't retain any WebContents pointers after Request().
 - Have IsClean() be weak_factory_.HasWeakPtrs()
 - Have Clear() do weak_factory_.InvalidateWeakPtrs()
 - Probably kill off |web_contents_| altogether.
 - Bind the ProcessResourceUsage* to the OnRefreshDone callback.
 - Possibly -- if it's not too weird -- the ProcessResourceUsage objects can be
bound as Owned() or scoped_ptr<> instead of keeping them in a ScopedVector or
whatever resource_usage_reporters_ becomes.

Not particularly in love with that last one, but I do think it would work.

[Anand Mistry (off Chromium), 2015-05-14 07:30:41]

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/chrome_n...
File chrome/browser/chrome_notification_types.h (left):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_notification_types.h:262:
NOTIFICATION_RENDERER_V8_HEAP_STATS_COMPUTED,
On 2015/05/14 06:28:12, ncarter wrote:
> :)
> 
> Make sure you mention BUG=268984 (Kill NotificationService) in your CL
comment.

Done.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
File chrome/browser/process_resource_usage.cc (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
chrome/browser/process_resource_usage.cc:53: void
ProcessResourceUsage::OnConnectionError() {
On 2015/05/14 06:28:12, ncarter wrote:
> Does it make sense to zero the |stats_| when the child process dies?

I honestly have no idea. Maybe we want to keep the last known information. Maybe
expose an "IsAlive()" instead, but right now, I don't think there's a use for
it. Since this only affects the task manager (memory-internals will create a new
one for each update), this question is, what do you want to see in the task
manager?

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
File chrome/browser/process_resource_usage.h (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
chrome/browser/process_resource_usage.h:55: void OnConnectionError() override;
On 2015/05/14 06:28:12, ncarter wrote:
> Making this method private doesn't really hide it from clients of
> ProcessResourceUsage -- because it's public in the base class. So I'd say
these
> ought to be public as well.
> 
> Since the paradigm here is "ProcessResourceUsage is implemented in terms of
> mojo::ErrorHandler" instead of "ProcessResourceUsage is a kind of
> mojo::ErrorHandler", you could hide these methods from clients of
> ProcessResourceUsage by using composition instead of inheritance.
> 
> Generally, when I'm tempted to change the access specifier in the derived
class
> to be different from that in a base interface class, an alternative is to move
> the interface implementation into a helper class of which the parent class has
a
> private member, and the helper class keeps a pointer back to the parent class
if
> needed. If you need to privately implement more than one interface, this can
> become PIMPL, where all the data members from the parent type move into the
> helper type.

Moved to a private helper class. I'd rather not go PIMPL just yet, but it would
be useful if I want to remove the exposed dependency on Mojo, which would be
nice. However, that's more work than I think it's worth at this point.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc:161:
resource_usage_reporters_.clear();
On 2015/05/14 06:28:12, ncarter wrote:
> Invalidating the weak pointers here seems like a good idea.

Done.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc:179:
DCHECK(iter != resource_usage_reporters_.end());
On 2015/05/14 06:28:12, ncarter wrote:
> This code was pretty dicey to begin with, so we can wind up in this callback
in
> some pretty weird situations:
> 
> 1. It's possible |content| is not a valid WC pointer when the refresh
completes
> -- the tab could have been closed.

I've considered this, which is why |content| is only used as a key. But the
assumption is that Clear() hasn't been called and the usage reporter hasn't been
cleared. Invalidating the weak_ptr in Clear() removes this possibility.

> 2. The resource_usage_reporters_ map might have been cleared between Refresh
and
> now -- the user could have hit the button again.

Invalidating the weak_ptr fixes this.

> 3. Because of [2] it's possible that we get back here with an entry in
> resource_usage_reporters_[content] that is a different object from the one
> currently invoking the callback.
> 
> Although it sounds like a pretty quick race that would be hard to lose, the v8
> stats will actually return after an arbitrary delay if we're executing an
> expensive js function. I just confirmed this in the console using "for (var
iii
> = 0; iii < 3000000; iii++) {}" in one tab while trying to collecting
> memory-internals in another.
> 
> The old code seemed to somehow be immune to crashes because, although it
> maintained a set of WebContents pointers, it didn't ever dereference them
> anytime after the set was first built. Request() happens during the same
message
> loop task as all the AddWebContents calls.
> 
> Some possible ways forward (short of totally reworking this class, which I'd
be
> okay with too):
>  - Don't retain any WebContents pointers after Request().
>  - Have IsClean() be weak_factory_.HasWeakPtrs()
>  - Have Clear() do weak_factory_.InvalidateWeakPtrs()
>  - Probably kill off |web_contents_| altogether.
>  - Bind the ProcessResourceUsage* to the OnRefreshDone callback.
>  - Possibly -- if it's not too weird -- the ProcessResourceUsage objects can
be
> bound as Owned() or scoped_ptr<> instead of keeping them in a ScopedVector or
> whatever resource_usage_reporters_ becomes.
> 
> Not particularly in love with that last one, but I do think it would work.

[ncarter (slow), 2015-05-14 13:59:59]

lgtm

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
File chrome/browser/process_resource_usage.cc (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/process_...
chrome/browser/process_resource_usage.cc:53: void
ProcessResourceUsage::OnConnectionError() {
On 2015/05/14 07:30:41, Anand Mistry wrote:
> On 2015/05/14 06:28:12, ncarter wrote:
> > Does it make sense to zero the |stats_| when the child process dies?
> 
> I honestly have no idea. Maybe we want to keep the last known information.
Maybe
> expose an "IsAlive()" instead, but right now, I don't think there's a use for
> it. Since this only affects the task manager (memory-internals will create a
new
> one for each update), this question is, what do you want to see in the task
> manager?

You can leave it as is. The task manager removes rows as soon as it knows
they've crashed, so I agree it's a moot point.

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc:179:
DCHECK(iter != resource_usage_reporters_.end());
On 2015/05/14 07:30:41, Anand Mistry wrote:
> On 2015/05/14 06:28:12, ncarter wrote:
> > This code was pretty dicey to begin with, so we can wind up in this callback
> in
> > some pretty weird situations:
> > 
> > 1. It's possible |content| is not a valid WC pointer when the refresh
> completes
> > -- the tab could have been closed.
> 
> I've considered this, which is why |content| is only used as a key. But the
> assumption is that Clear() hasn't been called and the usage reporter hasn't
been
> cleared. Invalidating the weak_ptr in Clear() removes this possibility.
> 
> > 2. The resource_usage_reporters_ map might have been cleared between Refresh
> and
> > now -- the user could have hit the button again.
> 
> Invalidating the weak_ptr fixes this.
> 
> > 3. Because of [2] it's possible that we get back here with an entry in
> > resource_usage_reporters_[content] that is a different object from the one
> > currently invoking the callback.
> > 
> > Although it sounds like a pretty quick race that would be hard to lose, the
v8
> > stats will actually return after an arbitrary delay if we're executing an
> > expensive js function. I just confirmed this in the console using "for (var
> iii
> > = 0; iii < 3000000; iii++) {}" in one tab while trying to collecting
> > memory-internals in another.
> > 
> > The old code seemed to somehow be immune to crashes because, although it
> > maintained a set of WebContents pointers, it didn't ever dereference them
> > anytime after the set was first built. Request() happens during the same
> message
> > loop task as all the AddWebContents calls.
> > 
> > Some possible ways forward (short of totally reworking this class, which I'd
> be
> > okay with too):
> >  - Don't retain any WebContents pointers after Request().
> >  - Have IsClean() be weak_factory_.HasWeakPtrs()
> >  - Have Clear() do weak_factory_.InvalidateWeakPtrs()
> >  - Probably kill off |web_contents_| altogether.
> >  - Bind the ProcessResourceUsage* to the OnRefreshDone callback.
> >  - Possibly -- if it's not too weird -- the ProcessResourceUsage objects can
> be
> > bound as Owned() or scoped_ptr<> instead of keeping them in a ScopedVector
or
> > whatever resource_usage_reporters_ becomes.
> > 
> > Not particularly in love with that last one, but I do think it would work.
> 

OK. I'll try to come back here myself and do something to remove the
|web_contents| pointers.

Since you've fixed 487927, make sure you mention it in the CL.

[Anand Mistry (off Chromium), 2015-05-14 21:37:15]

amistry@chromium.org changed reviewers:
+ jam@chromium.org

[Anand Mistry (off Chromium), 2015-05-14 21:37:16]

jam@chromium.org: For //chrome, and since you're a reviewer on the change this
depends on.

[Anand Mistry (off Chromium), 2015-05-14 23:26:04]

amistry@chromium.org changed reviewers:
+ tsepez@chromium.org

[Anand Mistry (off Chromium), 2015-05-14 23:26:05]

tsepez@chromium.org: For render_messages.h

[Tom Sepez, 2015-05-14 23:30:57]

RS LGTM on deleting code.

[Anand Mistry (off Chromium), 2015-05-19 04:28:38]

jam: Ping!

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc (right):

https://codereview.chromium.org/1081323003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc:179:
DCHECK(iter != resource_usage_reporters_.end());
On 2015/05/14 13:59:58, ncarter wrote:
> On 2015/05/14 07:30:41, Anand Mistry wrote:
> > On 2015/05/14 06:28:12, ncarter wrote:
> > > This code was pretty dicey to begin with, so we can wind up in this
callback
> > in
> > > some pretty weird situations:
> > > 
> > > 1. It's possible |content| is not a valid WC pointer when the refresh
> > completes
> > > -- the tab could have been closed.
> > 
> > I've considered this, which is why |content| is only used as a key. But the
> > assumption is that Clear() hasn't been called and the usage reporter hasn't
> been
> > cleared. Invalidating the weak_ptr in Clear() removes this possibility.
> > 
> > > 2. The resource_usage_reporters_ map might have been cleared between
Refresh
> > and
> > > now -- the user could have hit the button again.
> > 
> > Invalidating the weak_ptr fixes this.
> > 
> > > 3. Because of [2] it's possible that we get back here with an entry in
> > > resource_usage_reporters_[content] that is a different object from the one
> > > currently invoking the callback.
> > > 
> > > Although it sounds like a pretty quick race that would be hard to lose,
the
> v8
> > > stats will actually return after an arbitrary delay if we're executing an
> > > expensive js function. I just confirmed this in the console using "for
(var
> > iii
> > > = 0; iii < 3000000; iii++) {}" in one tab while trying to collecting
> > > memory-internals in another.
> > > 
> > > The old code seemed to somehow be immune to crashes because, although it
> > > maintained a set of WebContents pointers, it didn't ever dereference them
> > > anytime after the set was first built. Request() happens during the same
> > message
> > > loop task as all the AddWebContents calls.
> > > 
> > > Some possible ways forward (short of totally reworking this class, which
I'd
> > be
> > > okay with too):
> > >  - Don't retain any WebContents pointers after Request().
> > >  - Have IsClean() be weak_factory_.HasWeakPtrs()
> > >  - Have Clear() do weak_factory_.InvalidateWeakPtrs()
> > >  - Probably kill off |web_contents_| altogether.
> > >  - Bind the ProcessResourceUsage* to the OnRefreshDone callback.
> > >  - Possibly -- if it's not too weird -- the ProcessResourceUsage objects
can
> > be
> > > bound as Owned() or scoped_ptr<> instead of keeping them in a ScopedVector
> or
> > > whatever resource_usage_reporters_ becomes.
> > > 
> > > Not particularly in love with that last one, but I do think it would work.
> > 
> 
> OK. I'll try to come back here myself and do something to remove the
> |web_contents| pointers.
> 
> Since you've fixed 487927, make sure you mention it in the CL.

So, I've done some more testing and it turns out I haven't fixed this bug.
Although it's logically fixed, there's an implementation bug in how Mojo
interacts with render processes. Since this is part of the fix, I'll leave the
bug line. But the bug won't be fixed. Not until I fix the core issue. That said,
memory-internals won't hang forever. If you close the crashed tab, it recovers.

[jam, 2015-05-19 16:07:29]

lgtm

[Anand Mistry (off Chromium), 2015-05-20 01:51:40]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[Anand Mistry (off Chromium), 2015-05-20 01:51:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
nick@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1081323003/#ps80001
(title: "Rebase and remove obsolete comment.")

[commit-bot: I haz the power, 2015-05-20 01:52:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1081323003/80001

[Anand Mistry (off Chromium), 2015-05-20 03:01:30]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-05-20 03:01:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
nick@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1081323003/#ps100001
(title: "Rebase and fix tests.")

[commit-bot: I haz the power, 2015-05-20 03:01:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1081323003/100001

[commit-bot: I haz the power, 2015-05-20 03:49:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-20 03:49:59]

Patchset 6 (id:??) landed as
https://crrev.com/77099c24ca1783490e38a2e2d44c7a524c50531b
Cr-Commit-Position: refs/heads/master@{#330692}