Convert ChromeViewHostMsg_UpdatedCacheStats to use mojo.

Convert ChromeViewHostMsg_UpdatedCacheStats to use mojo.

BUG=577685
Review-Url: https://codereview.chromium.org/2643183002
Cr-Commit-Position: refs/heads/master@{#468234}
Committed: https://chromium.googlesource.com/chromium/src/+/beff0a104dbf869114f66db3efc2d52094050971

[nigeltao1, 2017-01-20 06:00:21]

nigeltao@chromium.org changed reviewers:
+ sammc@chromium.org

[Sam McNally, 2017-01-24 04:06:15]

https://codereview.chromium.org/2643183002/diff/1/chrome/browser/cache_stats_...
File chrome/browser/cache_stats_recorder.h (right):

https://codereview.chromium.org/2643183002/diff/1/chrome/browser/cache_stats_...
chrome/browser/cache_stats_recorder.h:23: int render_process_id_;
const

https://codereview.chromium.org/2643183002/diff/1/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_thread_observer.cc (right):

https://codereview.chromium.org/2643183002/diff/1/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_thread_observer.cc:121:
chrome::mojom::CacheStatsRecorderPtr cache_stats_recorder;
This one feels like the connection should be reused.

[nigeltao1, 2017-02-02 05:20:59]

https://codereview.chromium.org/2643183002/diff/1/chrome/browser/cache_stats_...
File chrome/browser/cache_stats_recorder.h (right):

https://codereview.chromium.org/2643183002/diff/1/chrome/browser/cache_stats_...
chrome/browser/cache_stats_recorder.h:23: int render_process_id_;
On 2017/01/24 04:06:15, Sam McNally wrote:
> const

Done.

https://codereview.chromium.org/2643183002/diff/1/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_thread_observer.cc (right):

https://codereview.chromium.org/2643183002/diff/1/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_thread_observer.cc:121:
chrome::mojom::CacheStatsRecorderPtr cache_stats_recorder;
On 2017/01/24 04:06:15, Sam McNally wrote:
> This one feels like the connection should be reused.

Done.

[nigeltao1, 2017-02-02 05:21:38]

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

[commit-bot: I haz the power, 2017-02-02 05:22:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 06:57:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 06:57:02]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-02-02 19:43:39]

lgtm

https://codereview.chromium.org/2643183002/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_thread_observer.cc (right):

https://codereview.chromium.org/2643183002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_thread_observer.cc:121: if
(!cache_stats_recorder_.is_bound())
if (!cache_stats_recorder_) {

[nigeltao1, 2017-02-03 01:47:37]

nigeltao@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[nigeltao1, 2017-02-03 01:47:38]

R=sky,tsepez for OWNERS.

https://codereview.chromium.org/2643183002/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_thread_observer.cc (right):

https://codereview.chromium.org/2643183002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_thread_observer.cc:121: if
(!cache_stats_recorder_.is_bound())
On 2017/02/02 19:43:39, Sam McNally wrote:
> if (!cache_stats_recorder_) {

Done.

[sky, 2017-02-03 16:31:44]

https://codereview.chromium.org/2643183002/diff/40001/chrome/browser/cache_st...
File chrome/browser/cache_stats_recorder.cc (right):

https://codereview.chromium.org/2643183002/diff/40001/chrome/browser/cache_st...
chrome/browser/cache_stats_recorder.cc:27:
web_cache::WebCacheManager::GetInstance()->ObserveStats(render_process_id_,
Is there any concern that by the time this function is called the renderer
process id may no longer be valid? Do we at all care about recycling? I'm
wondering if CacheStatsRecorders should be destroyed if the renderer process is
destroyed.

[Tom Sepez, 2017-02-03 20:05:53]

mojom LGTM

[sky, 2017-02-03 23:46:13]

Also, do we have test coverage of this?

[nigeltao1, 2017-02-07 21:32:57]

On 2017/02/03 23:46:13, sky wrote:
> Also, do we have test coverage of this?

IIUC, we don't. See https://codereview.chromium.org/2674343003/ where I
commented out the old-school IPC message send, in an otherwise clean branch, and
all the tests still pass.

I manually tested this commit by printf'ing the cache capacity / size numbers
for the old-school IPC before the commit and for the new-school Mojo RPC after
the commit, and browsing a few sites.

If you want me to add new test coverage, I can look into it. Do you know who to
ask for Chromium expertise around cache stats?

[sky, 2017-02-07 23:12:41]

I'm not sure either. I suggest looking at the blame log for who added the IPC.

  -Scott

On Tue, Feb 7, 2017 at 1:32 PM,  <nigeltao@chromium.org> wrote:
> On 2017/02/03 23:46:13, sky wrote:
>> Also, do we have test coverage of this?
>
> IIUC, we don't. See https://codereview.chromium.org/2674343003/ where I
> commented out the old-school IPC message send, in an otherwise clean branch,
> and
> all the tests still pass.
>
> I manually tested this commit by printf'ing the cache capacity / size
> numbers
> for the old-school IPC before the commit and for the new-school Mojo RPC
> after
> the commit, and browsing a few sites.
>
> If you want me to add new test coverage, I can look into it. Do you know who
> to
> ask for Chromium expertise around cache stats?
>
> https://codereview.chromium.org/2643183002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nigeltao1, 2017-02-16 02:50:50]

On 2017/02/03 16:31:44, sky wrote:
>
https://codereview.chromium.org/2643183002/diff/40001/chrome/browser/cache_st...
> File chrome/browser/cache_stats_recorder.cc (right):
> 
>
https://codereview.chromium.org/2643183002/diff/40001/chrome/browser/cache_st...
> chrome/browser/cache_stats_recorder.cc:27:
> web_cache::WebCacheManager::GetInstance()->ObserveStats(render_process_id_,
> Is there any concern that by the time this function is called the renderer
> process id may no longer be valid? Do we at all care about recycling? I'm
> wondering if CacheStatsRecorders should be destroyed if the renderer process
is
> destroyed.

The CacheStatsRecorder is bound as a mojo::StrongBinding, which means that the
CacheStatsRecorder is destroyed on connection error on the underlying message
pipe, including the connection close when the renderer process is destroyed.

I'm not 100% certain whether the connection close observation happens just
before or just after the actual PID becomes recycled. If just after, then during
that window, technically, there's a possibility of a renderer PID being re-used,
but I suspect that it's extremely unlikely in practice.

Dropping in some temporary log messages suggests, though, that the
CacheStatsRecorder destructor is called (on the UI thread) before the
RenderProcessHostImpl destructor is called.

sammc: can you sanity check what I just wrote.

[nigeltao1, 2017-02-16 02:53:48]

On 2017/02/07 23:12:41, sky wrote:
> I'm not sure either. I suggest looking at the blame log for who added the IPC.
> 
>   -Scott
> 
> On Tue, Feb 7, 2017 at 1:32 PM,  <mailto:nigeltao@chromium.org> wrote:
> > On 2017/02/03 23:46:13, sky wrote:
> >> Also, do we have test coverage of this?
> >
> > IIUC, we don't. See https://codereview.chromium.org/2674343003/ where I
> > commented out the old-school IPC message send, in an otherwise clean branch,
> > and
> > all the tests still pass.
> >
> > I manually tested this commit by printf'ing the cache capacity / size
> > numbers
> > for the old-school IPC before the commit and for the new-school Mojo RPC
> > after
> > the commit, and browsing a few sites.
> >
> > If you want me to add new test coverage, I can look into it. Do you know who
> > to
> > ask for Chromium expertise around cache stats?
> >
> > https://codereview.chromium.org/2643183002/

As for adding test coverage, I asked (on a separate mail thread, CC'ing you
(sky)) such people, but got no leads. I have tested this manually, by dropping
in some printf's around the old IPC and new Mojo RPCs, at both the sender and
receiver ends, and the message contents look the same to me.

[Sam McNally, 2017-02-19 22:49:13]

On 2017/02/16 02:50:50, nigeltao1 wrote:
> On 2017/02/03 16:31:44, sky wrote:
> >
>
https://codereview.chromium.org/2643183002/diff/40001/chrome/browser/cache_st...
> > File chrome/browser/cache_stats_recorder.cc (right):
> > 
> >
>
https://codereview.chromium.org/2643183002/diff/40001/chrome/browser/cache_st...
> > chrome/browser/cache_stats_recorder.cc:27:
> > web_cache::WebCacheManager::GetInstance()->ObserveStats(render_process_id_,
> > Is there any concern that by the time this function is called the renderer
> > process id may no longer be valid? Do we at all care about recycling? I'm
> > wondering if CacheStatsRecorders should be destroyed if the renderer process
> is
> > destroyed.
> 
> The CacheStatsRecorder is bound as a mojo::StrongBinding, which means that the
> CacheStatsRecorder is destroyed on connection error on the underlying message
> pipe, including the connection close when the renderer process is destroyed.
> 
> I'm not 100% certain whether the connection close observation happens just
> before or just after the actual PID becomes recycled. If just after, then
during
> that window, technically, there's a possibility of a renderer PID being
re-used,
> but I suspect that it's extremely unlikely in practice.
> 
> Dropping in some temporary log messages suggests, though, that the
> CacheStatsRecorder destructor is called (on the UI thread) before the
> RenderProcessHostImpl destructor is called.
> 
> sammc: can you sanity check what I just wrote.

WebCacheManager already deals with unexpected renderer IDs so late IPCs aren't a
problem during normal renderer shutdown. It could be a problem if the renderer
crashes and restarts; in that case I believe the ID is reused. Making this
interface channel-associated would avoid this risk.

[nigeltao1, 2017-03-13 04:09:05]

On 2017/02/19 22:49:13, Sam McNally wrote:
> Making this interface channel-associated would avoid this risk.

OK, I've made this an associated interface. I'm not totally confident that it's
legit, so sammc, please review carefully. Two things to focus on:

In chrome/browser/chrome_content_browser_client.cc, I've called the
AddAssociatedInterfaceForIOThread method on the IPC::ChannelProxy. The
ForIOThread part doesn't quite feel right, but AFAICT, there isn't a separate
AddAssociatedInterface method that doesn't say ForIOThread, and I didn't find
any other precedent for chrome/browser code binding, instead of sending to, an
associated interface.

In chrome/browser/cache_stats_recorder.cc, I'm therefore not sure if I need to
do some dance to get back onto the UI thread.

[nigeltao1, 2017-03-13 04:50:41]

nigeltao@chromium.org changed reviewers:
+ rockot@chromium.org

[nigeltao1, 2017-03-13 04:54:32]

On 2017/03/13 04:09:05, nigeltao1 wrote:
> In chrome/browser/chrome_content_browser_client.cc, I've called the
> AddAssociatedInterfaceForIOThread method on the IPC::ChannelProxy. The
> ForIOThread part doesn't quite feel right, but AFAICT, there isn't a separate
> AddAssociatedInterface method that doesn't say ForIOThread,

rockot, do you have any suggestions, given that you wrote
https://codereview.chromium.org/2653973002 "Remove associated interface
registration from ChannelProxy"?

[Ken Rockot(use gerrit already), 2017-03-13 17:19:53]

On 2017/03/13 at 04:54:32, nigeltao wrote:
> On 2017/03/13 04:09:05, nigeltao1 wrote:
> > In chrome/browser/chrome_content_browser_client.cc, I've called the
> > AddAssociatedInterfaceForIOThread method on the IPC::ChannelProxy. The
> > ForIOThread part doesn't quite feel right, but AFAICT, there isn't a
separate
> > AddAssociatedInterface method that doesn't say ForIOThread,
> 
> rockot, do you have any suggestions, given that you wrote
https://codereview.chromium.org/2653973002 "Remove associated interface
registration from ChannelProxy"?

What you're doing is exactly correct.

As a message that was handled by a BrowserMessageFilter with no relevant
override from the impl of OverrideThreadForMessage, we know this IPC is
currently handled on the IO thread. It follows then that you want the associated
interface binder for this interface to be registered on the IO thread, and
that's precisely the purpose of AddAssociatedInterfaceForIOThread.

I'd like to see if we can start to find sane ways to avoid using associated
interfaces in most cases like this, e.g., where there's only one relatively
simple ordering dependency like the lifetime of a specific renderer rather than
complex ordering dependencies against numerous other events. That said, I still
think this is fine for now.

[Ken Rockot(use gerrit already), 2017-03-13 17:19:58]

https://codereview.chromium.org/2643183002/diff/60001/chrome/browser/cache_st...
File chrome/browser/cache_stats_recorder.cc (right):

https://codereview.chromium.org/2643183002/diff/60001/chrome/browser/cache_st...
chrome/browser/cache_stats_recorder.cc:28:
web_cache::WebCacheManager::GetInstance()->ObserveStats(render_process_id_,
IIUC this is clearly already happening on the IO thread today, so I'd say no.

[nigeltao1, 2017-03-17 05:23:25]

On 2017/03/13 17:19:53, Ken Rockot wrote:
> As a message that was handled by a BrowserMessageFilter with no relevant
> override from the impl of OverrideThreadForMessage, we know this IPC is
> currently handled on the IO thread.

There *is* an override from the impl of OverrideThreadForMessage. If you look at
the diff for chrome/browser/renderer_host/chrome_render_message_filter.cc in
this CL, the old code, in ChromeRenderMessageFilter::OverrideThreadForMessage,
explicitly said "case ChromeViewHostMsg_UpdatedCacheStats::ID: *thread =
BrowserThread::UI;".

[Ken Rockot(use gerrit already), 2017-03-17 05:55:17]

Yikes, I misread it. So then this isn't the right thing to do.

Channel-associated interface requests coming in on the browser side with no
IO thread handler are going to be routed by default to
RenderProcessHostImpl::OnAssociatedInterfaceRequest on the UI thread.

If I were you, I'd add an AssociatedInterfaceRegistryImpl
<https://cs.chromium.org/chromium/src/content/common/associated_interface_regi...>
member
to RPHI, expose a way for ContentBrowserClient to register interfaces on
it, and use that registry to bind incoming requests in
OnAssociatedInterfaceRequest.

This is exactly what RenderThreadImpl does for the renderer-side
equivalent, i.e. main-thread Channel-associated interface binding.

On Thu, Mar 16, 2017 at 10:23 PM, <nigeltao@chromium.org> wrote:

> On 2017/03/13 17:19:53, Ken Rockot wrote:
> > As a message that was handled by a BrowserMessageFilter with no relevant
> > override from the impl of OverrideThreadForMessage, we know this IPC is
> > currently handled on the IO thread.
>
> There *is* an override from the impl of OverrideThreadForMessage. If you
> look at
> the diff for chrome/browser/renderer_host/chrome_render_message_filter.cc
> in
> this CL, the old code, in ChromeRenderMessageFilter::
> OverrideThreadForMessage,
> explicitly said "case ChromeViewHostMsg_UpdatedCacheStats::ID: *thread =
> BrowserThread::UI;".
>
>
> https://codereview.chromium.org/2643183002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nigeltao1, 2017-03-18 04:50:21]

On 2017/03/17 05:55:17, Ken Rockot wrote:
> If I were you, I'd add an AssociatedInterfaceRegistryImpl
>
<https://cs.chromium.org/chromium/src/content/common/associated_interface_regi...>
> member
> to RPHI, expose a way for ContentBrowserClient to register interfaces on
> it, and use that registry to bind incoming requests in
> OnAssociatedInterfaceRequest.

Done.

[nigeltao1, 2017-03-18 04:50:55]

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

[commit-bot: I haz the power, 2017-03-18 04:51:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 05:36:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 05:36:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nigeltao1, 2017-03-18 07:22:21]

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

[commit-bot: I haz the power, 2017-03-18 07:22:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 08:08:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 08:08:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nigeltao1, 2017-03-19 00:43:06]

Huh, some of the tests are failing with:

[18867:18867:0318/005553.514584:FATAL:associated_interface_registry_impl.cc(34)]
Check failed: result.second.
#0 0x000003559367 base::debug::StackTrace::StackTrace()
#1 0x000003570e9a logging::LogMessage::~LogMessage()
#2 0x000001c838a8 content::AssociatedInterfaceRegistryImpl::AddInterface()
#3 0x0000036811cc content::AssociatedInterfaceRegistry::AddInterface<>()
#4 0x000003680e64 ChromeContentBrowserClient::ExposeInterfacesToRenderer()
#5 0x0000023c3900 content::RenderProcessHostImpl::RegisterMojoInterfaces()
#6 0x0000023c15d4 content::RenderProcessHostImpl::Init()

The AssociatedInterfaceRegistryImpl code is:

void AssociatedInterfaceRegistryImpl::AddInterface(const std::string& name,
                                                   const Binder& binder) {
  auto result = interfaces_.insert(std::make_pair(name, binder));
  DCHECK(result.second);
}

The AssociatedInterfaceRegistry code is:

// Templated helper for AddInterface() above.
template <typename Interface>
void AddInterface(const InterfaceBinder<Interface>& binder) {
  AddInterface(Interface::Name_,
               base::Bind(&BindInterface<Interface>, binder));
}

Why would the binder (result.second) be false? Is it some missing header file
for C++ template magic?? I'll poke at it some more the next time I'm in the
office, but in the meantime, if you have any suggestions, or if something in the
patch looks wrong, please share.

[Ken Rockot(use gerrit already), 2017-03-20 16:00:57]

On 2017/03/19 at 00:43:06, nigeltao wrote:
> Huh, some of the tests are failing with:
> 
>
[18867:18867:0318/005553.514584:FATAL:associated_interface_registry_impl.cc(34)]
Check failed: result.second.
> #0 0x000003559367 base::debug::StackTrace::StackTrace()
> #1 0x000003570e9a logging::LogMessage::~LogMessage()
> #2 0x000001c838a8 content::AssociatedInterfaceRegistryImpl::AddInterface()
> #3 0x0000036811cc content::AssociatedInterfaceRegistry::AddInterface<>()
> #4 0x000003680e64 ChromeContentBrowserClient::ExposeInterfacesToRenderer()
> #5 0x0000023c3900 content::RenderProcessHostImpl::RegisterMojoInterfaces()
> #6 0x0000023c15d4 content::RenderProcessHostImpl::Init()
> 
> The AssociatedInterfaceRegistryImpl code is:
> 
> void AssociatedInterfaceRegistryImpl::AddInterface(const std::string& name,
>                                                    const Binder& binder) {
>   auto result = interfaces_.insert(std::make_pair(name, binder));
>   DCHECK(result.second);
> }
> 
> The AssociatedInterfaceRegistry code is:
> 
> // Templated helper for AddInterface() above.
> template <typename Interface>
> void AddInterface(const InterfaceBinder<Interface>& binder) {
>   AddInterface(Interface::Name_,
>                base::Bind(&BindInterface<Interface>, binder));
> }
> 
> Why would the binder (result.second) be false? Is it some missing header file
for C++ template magic?? I'll poke at it some more the next time I'm in the
office, but in the meantime, if you have any suggestions, or if something in the
patch looks wrong, please share.

If insertion fails it's because the key is already in the map.

[nigeltao1, 2017-03-23 05:52:18]

On 2017/03/20 16:00:57, Ken Rockot wrote:
> If insertion fails it's because the key is already in the map.

Ah, the problem doesn't manifest in a quick, manual kicking of the tyres
(including hitting Control-R to re-load pages), but browser tests such as
CrashRecoveryBrowserTest.LoadInNewTab deliberately navigate to
"chrome://crash/". The subsequent page reload leads to a second call to
content::RenderProcessHostImpl::RegisterMojoInterfaces on the same
*RenderProcessHostImpl. For the record, the top of the two stack traces to
ChromeContentBrowserClient::ExposeInterfacesToRenderer:

#0 0x7f4ee9a374fb base::debug::StackTrace::StackTrace()
#1 0x7f4ee9a35b8c base::debug::StackTrace::StackTrace()
#2 0x000002bba89a ChromeContentBrowserClient::ExposeInterfacesToRenderer()
#3 0x7f4ee18b9d1b content::RenderProcessHostImpl::RegisterMojoInterfaces()
#4 0x7f4ee18b6ecf content::RenderProcessHostImpl::Init()
#5 0x7f4ee13891da content::RenderFrameHostManager::InitRenderView()
#6 0x7f4ee138043d content::RenderFrameHostManager::ReinitializeRenderFrame()
#7 0x7f4ee137f023 content::RenderFrameHostManager::Navigate()
#8 0x7f4ee1328324 content::NavigatorImpl::NavigateToEntry()
#9 0x7f4ee13297e8 content::NavigatorImpl::NavigateToPendingEntry()
#10 0x7f4ee12fadda
content::NavigationControllerImpl::NavigateToPendingEntryInternal()
#11 0x7f4ee12f3277 content::NavigationControllerImpl::NavigateToPendingEntry()
#12 0x7f4ee12f36c5 content::NavigationControllerImpl::LoadEntry()
#13 0x7f4ee12f551c content::NavigationControllerImpl::LoadURLWithParams()
#14 0x000005c19734 (anonymous namespace)::LoadURLInContents()
#15 0x000005c183ea chrome::Navigate()

#0 0x7f4ee9a374fb base::debug::StackTrace::StackTrace()
#1 0x7f4ee9a35b8c base::debug::StackTrace::StackTrace()
#2 0x000002bba89a ChromeContentBrowserClient::ExposeInterfacesToRenderer()
#3 0x7f4ee18b9d1b content::RenderProcessHostImpl::RegisterMojoInterfaces()
#4 0x7f4ee18b6ecf content::RenderProcessHostImpl::Init()
#5 0x7f4ee13891da content::RenderFrameHostManager::InitRenderView()
#6 0x7f4ee138043d content::RenderFrameHostManager::ReinitializeRenderFrame()
#7 0x7f4ee137f023 content::RenderFrameHostManager::Navigate()
#8 0x7f4ee1328324 content::NavigatorImpl::NavigateToEntry()
#9 0x7f4ee13297e8 content::NavigatorImpl::NavigateToPendingEntry()
#10 0x7f4ee12fadda
content::NavigationControllerImpl::NavigateToPendingEntryInternal()
#11 0x7f4ee12f3277 content::NavigationControllerImpl::NavigateToPendingEntry()
#12 0x7f4ee12f2b6b content::NavigationControllerImpl::Reload()
#13 0x000005c035d1 chrome::(anonymous namespace)::ReloadInternal()
#14 0x000005c034dc chrome::Reload()

The first line of content::RenderProcessHostImpl::RegisterMojoInterfaces
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...)
creates a new service_manager::InterfaceRegistry as a local variable and towards
the end of that function, std::move's it elsewhere:

void RenderProcessHostImpl::RegisterMojoInterfaces() {
  auto registry = base::MakeUnique<service_manager::InterfaceRegistry>(
      service_manager::mojom::kServiceManager_ConnectorSpec);

  ... // lots of individual bindings.

  GetContentClient()->browser()->ExposeInterfacesToRenderer(registry.get(),
                                                            this);

  ServiceManagerConnection* service_manager_connection =
      BrowserContext::GetServiceManagerConnectionFor(browser_context_);
  std::unique_ptr<ConnectionFilterImpl> connection_filter(
      new ConnectionFilterImpl(child_connection_->child_identity(),
                               std::move(registry)));
  connection_filter_controller_ = connection_filter->controller();
  connection_filter_id_ = service_manager_connection->AddConnectionFilter(
      std::move(connection_filter));
}

In other words, every call to
content::RenderProcessHostImpl::RegisterMojoInterfaces leads to a new
service_manager::InterfaceRegistry. In comparison, the way this CL is set up,
every call to the same content::RenderProcessHostImpl::RegisterMojoInterfaces
method uses the same *AssociatedInterfaceRegistryImpl, since that AIRI is a
field of the RenderProcessHostImpl class, hence the duplicate keys and then the
DCHECK failure.

I can think of several workarounds, but none of them seem satisfactory.

One is to guard the call to associated_registry->AddInterface at the top of
ChromeContentBrowserClient::ExposeInterfacesToRenderer. Instead of an
unconditional call, surround it by "if (associated_registry->NotBound(name) {
... }". But there isn't any such NotBound method on
content::AssociatedInterfaceRegistry*, and I suspect that we don't want one:
this doesn't seem the right place to solve duplicate bindings.

Two is to swap in a new AssociatedInterfaceRegistryImpl to replace the existing
one in content::RenderProcessHostImpl, but that would mean (silently) losing any
existing bindings. There aren't any other ones now, but there might be in the
future, and this seems precarious. Also, this doesn't un-bind the old
AssociatedInterfaceRegistryImpl. Could this lead to a memory leak or dangling
pointer??

Three is to be called back somehow whenever either the render process crashes or
a RPHI is otherwise re-used, and have a corresponding RemoveInterface call for
every previous AddInterface call. But again, that seems precarious.

Four is to add an ExposeAssociatedInterfacesToRenderer method, like
ExposeInterfacesToRenderer but for *associated* interfaces, to
content::ContentBrowserClient. But the caveat is that EAITR would be called only
once, but EITR would be called once per content::RenderProcessHostImpl::Init
call. Once again, this subtle distinction seems precarious.

There might very well be a simpler solution that escapes me right now. Any
suggestions?

[Ken Rockot(use gerrit already), 2017-03-24 19:38:52]

On 2017/03/23 05:52:18, nigeltao1 wrote:
> On 2017/03/20 16:00:57, Ken Rockot wrote:
> > If insertion fails it's because the key is already in the map.
> 
> Ah, the problem doesn't manifest in a quick, manual kicking of the tyres
> (including hitting Control-R to re-load pages), but browser tests such as
> CrashRecoveryBrowserTest.LoadInNewTab deliberately navigate to
> "chrome://crash/". The subsequent page reload leads to a second call to
> content::RenderProcessHostImpl::RegisterMojoInterfaces on the same
> *RenderProcessHostImpl. For the record, the top of the two stack traces to
> ChromeContentBrowserClient::ExposeInterfacesToRenderer:
> 
> #0 0x7f4ee9a374fb base::debug::StackTrace::StackTrace()
> #1 0x7f4ee9a35b8c base::debug::StackTrace::StackTrace()
> #2 0x000002bba89a ChromeContentBrowserClient::ExposeInterfacesToRenderer()
> #3 0x7f4ee18b9d1b content::RenderProcessHostImpl::RegisterMojoInterfaces()
> #4 0x7f4ee18b6ecf content::RenderProcessHostImpl::Init()
> #5 0x7f4ee13891da content::RenderFrameHostManager::InitRenderView()
> #6 0x7f4ee138043d content::RenderFrameHostManager::ReinitializeRenderFrame()
> #7 0x7f4ee137f023 content::RenderFrameHostManager::Navigate()
> #8 0x7f4ee1328324 content::NavigatorImpl::NavigateToEntry()
> #9 0x7f4ee13297e8 content::NavigatorImpl::NavigateToPendingEntry()
> #10 0x7f4ee12fadda
> content::NavigationControllerImpl::NavigateToPendingEntryInternal()
> #11 0x7f4ee12f3277 content::NavigationControllerImpl::NavigateToPendingEntry()
> #12 0x7f4ee12f36c5 content::NavigationControllerImpl::LoadEntry()
> #13 0x7f4ee12f551c content::NavigationControllerImpl::LoadURLWithParams()
> #14 0x000005c19734 (anonymous namespace)::LoadURLInContents()
> #15 0x000005c183ea chrome::Navigate()
> 
> #0 0x7f4ee9a374fb base::debug::StackTrace::StackTrace()
> #1 0x7f4ee9a35b8c base::debug::StackTrace::StackTrace()
> #2 0x000002bba89a ChromeContentBrowserClient::ExposeInterfacesToRenderer()
> #3 0x7f4ee18b9d1b content::RenderProcessHostImpl::RegisterMojoInterfaces()
> #4 0x7f4ee18b6ecf content::RenderProcessHostImpl::Init()
> #5 0x7f4ee13891da content::RenderFrameHostManager::InitRenderView()
> #6 0x7f4ee138043d content::RenderFrameHostManager::ReinitializeRenderFrame()
> #7 0x7f4ee137f023 content::RenderFrameHostManager::Navigate()
> #8 0x7f4ee1328324 content::NavigatorImpl::NavigateToEntry()
> #9 0x7f4ee13297e8 content::NavigatorImpl::NavigateToPendingEntry()
> #10 0x7f4ee12fadda
> content::NavigationControllerImpl::NavigateToPendingEntryInternal()
> #11 0x7f4ee12f3277 content::NavigationControllerImpl::NavigateToPendingEntry()
> #12 0x7f4ee12f2b6b content::NavigationControllerImpl::Reload()
> #13 0x000005c035d1 chrome::(anonymous namespace)::ReloadInternal()
> #14 0x000005c034dc chrome::Reload()
> 
> The first line of content::RenderProcessHostImpl::RegisterMojoInterfaces
>
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...)
> creates a new service_manager::InterfaceRegistry as a local variable and
towards
> the end of that function, std::move's it elsewhere:
> 
> void RenderProcessHostImpl::RegisterMojoInterfaces() {
>   auto registry = base::MakeUnique<service_manager::InterfaceRegistry>(
>       service_manager::mojom::kServiceManager_ConnectorSpec);
> 
>   ... // lots of individual bindings.
> 
>   GetContentClient()->browser()->ExposeInterfacesToRenderer(registry.get(),
>                                                             this);
> 
>   ServiceManagerConnection* service_manager_connection =
>       BrowserContext::GetServiceManagerConnectionFor(browser_context_);
>   std::unique_ptr<ConnectionFilterImpl> connection_filter(
>       new ConnectionFilterImpl(child_connection_->child_identity(),
>                                std::move(registry)));
>   connection_filter_controller_ = connection_filter->controller();
>   connection_filter_id_ = service_manager_connection->AddConnectionFilter(
>       std::move(connection_filter));
> }
> 
> In other words, every call to
> content::RenderProcessHostImpl::RegisterMojoInterfaces leads to a new
> service_manager::InterfaceRegistry. In comparison, the way this CL is set up,
> every call to the same content::RenderProcessHostImpl::RegisterMojoInterfaces
> method uses the same *AssociatedInterfaceRegistryImpl, since that AIRI is a
> field of the RenderProcessHostImpl class, hence the duplicate keys and then
the
> DCHECK failure.
> 
> I can think of several workarounds, but none of them seem satisfactory.
> 
> One is to guard the call to associated_registry->AddInterface at the top of
> ChromeContentBrowserClient::ExposeInterfacesToRenderer. Instead of an
> unconditional call, surround it by "if (associated_registry->NotBound(name) {
> ... }". But there isn't any such NotBound method on
> content::AssociatedInterfaceRegistry*, and I suspect that we don't want one:
> this doesn't seem the right place to solve duplicate bindings.
> 
> Two is to swap in a new AssociatedInterfaceRegistryImpl to replace the
existing
> one in content::RenderProcessHostImpl, but that would mean (silently) losing
any
> existing bindings. There aren't any other ones now, but there might be in the
> future, and this seems precarious. Also, this doesn't un-bind the old
> AssociatedInterfaceRegistryImpl. Could this lead to a memory leak or dangling
> pointer??

Not sure why this is precarious. It seems like exactly the same situation as
InterfaceRegistry. I would just do the same thing InterfaceRegistry is doing,
and
for convenience add an AIRI* arg to ExposeInterfacesToRenderer. Am I missing
some
reason why this isn't sufficient?

I'm also not sure what you mean by un-binding the old registry or the notion
that it would silently lose existing bindings. The registry does not own any
bindings and is not itself bound to anything. It's just a map of callbacks.

> 
> Three is to be called back somehow whenever either the render process crashes
or
> a RPHI is otherwise re-used, and have a corresponding RemoveInterface call for
> every previous AddInterface call. But again, that seems precarious.
> 
> Four is to add an ExposeAssociatedInterfacesToRenderer method, like
> ExposeInterfacesToRenderer but for *associated* interfaces, to
> content::ContentBrowserClient. But the caveat is that EAITR would be called
only
> once, but EITR would be called once per content::RenderProcessHostImpl::Init
> call. Once again, this subtle distinction seems precarious.
> 
> There might very well be a simpler solution that escapes me right now. Any
> suggestions?

[nigeltao1, 2017-03-31 04:23:42]

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

[commit-bot: I haz the power, 2017-03-31 04:23:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 04:26:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 04:26:39]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[nigeltao1, 2017-03-31 04:39:26]

On 2017/03/24 19:38:52, Ken Rockot wrote:
> On 2017/03/23 05:52:18, nigeltao1 wrote:
> > Two is to swap in a new AssociatedInterfaceRegistryImpl to replace the
> existing
> > one in content::RenderProcessHostImpl, but that would mean (silently) losing
> any
> > existing bindings. There aren't any other ones now, but there might be in
the
> > future, and this seems precarious. Also, this doesn't un-bind the old
> > AssociatedInterfaceRegistryImpl. Could this lead to a memory leak or
dangling
> > pointer??
> 
> Not sure why this is precarious. It seems like exactly the same situation as
> InterfaceRegistry. I would just do the same thing InterfaceRegistry is doing,
> and
> for convenience add an AIRI* arg to ExposeInterfacesToRenderer. Am I missing
> some
> reason why this isn't sufficient?
> 
> I'm also not sure what you mean by un-binding the old registry or the notion
> that it would silently lose existing bindings. The registry does not own any
> bindings and is not itself bound to anything. It's just a map of callbacks.

Ah, an AssociatedInterfaceRegistryImpl is lighter than I feared.

I've coded this approach. PTAL. Still, I said precarious because the flow in
render_process_host_impl.cc is now:

RenderProcessHostImpl::RegisterMojoInterfaces() {
  auto registry = `make a new service_manager::InterfaceRegistry`;

  etc

  // This line is new in this CL.
  associated_registry_.reset(new AssociatedInterfaceRegistryImpl());

  GetContentClient()->browser()->ExposeInterfacesToRenderer(etc);

  `do stuff with registry, a ServiceManagerConnection* and
      ConnectionFilterImpl*, involving std::move(registry)`;
}

The key point being that associated_registry_ is a member field of
RenderProcessHostImpl, but the (non-associated) registry is not.

Suppose that a future maintainer needs to bind an associated interface (let's
call it Foo), sees the existing associated_registry_ member field, and calls
AddInterface on it, not necessarily during the RegisterMojoInterfaces method.

This might work almost always, but on rare occasions (e.g. on the renderer
process crashing), RegisterMojoInterfaces might be called twice. The second
call resets the AssociatedInterfaceRegistryImpl to a new (empty) map of
callbacks, which loses the Foo mapping.

The risk here is subtle and non-obvious, which is why I said precarious. Does
that make sense?

I suppose that, instead of being a member field, I could try to copy/paste the
`do stuff with registry, a ServiceManagerConnection* etc` and apply it to the
associated_registry, but I don't really grok what that code is doing.

[nigeltao1, 2017-03-31 04:54:39]

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

[commit-bot: I haz the power, 2017-03-31 04:55:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 08:20:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 08:20:45]

Dry run: This issue passed the CQ dry run.

[nigeltao1, 2017-04-05 22:33:45]

On 2017/03/31 04:39:26, nigeltao1 wrote:
> I've coded this approach. PTAL.

rockot: ping.

[nigeltao1, 2017-04-06 01:37:32]

On 2017/04/05 22:33:45, nigeltao1 wrote:
> On 2017/03/31 04:39:26, nigeltao1 wrote:
> > I've coded this approach. PTAL.
> 
> rockot: ping.

Ah, you're OOO. Sorry.



Still, another question for rockot or tsepez...

My change to content/public/browser/content_browser_client.h adds
AssociatedInterfaceRegistry* associated_registry
parameter to ExposeInterfacesToRenderer, mimicking the existing
service_manager::InterfaceRegistry* registry
argument.

In the meantime, ben landed https://codereview.chromium.org/2766263009 "Convert
content ConnectionFilter to OnBindInterface" which changed that argument from
service_manager::InterfaceRegistry* registry
to
service_manager::BinderRegistry* registry

Do I need to do something similar for the associated flavor? I'm trying to read
CL 2766263009 but I'm feeling lost and would appreciate some guidance.

[Ken Rockot(use gerrit already), 2017-04-10 21:23:48]

On 2017/04/06 at 01:37:32, nigeltao wrote:
> On 2017/04/05 22:33:45, nigeltao1 wrote:
> > On 2017/03/31 04:39:26, nigeltao1 wrote:
> > > I've coded this approach. PTAL.
> > 
> > rockot: ping.
> 
> Ah, you're OOO. Sorry.
> 
> 
> 
> Still, another question for rockot or tsepez...
> 
> My change to content/public/browser/content_browser_client.h adds
> AssociatedInterfaceRegistry* associated_registry
> parameter to ExposeInterfacesToRenderer, mimicking the existing
> service_manager::InterfaceRegistry* registry
> argument.
> 
> In the meantime, ben landed https://codereview.chromium.org/2766263009
"Convert content ConnectionFilter to OnBindInterface" which changed that
argument from
> service_manager::InterfaceRegistry* registry
> to
> service_manager::BinderRegistry* registry
> 
> Do I need to do something similar for the associated flavor? I'm trying to
read CL 2766263009 but I'm feeling lost and would appreciate some guidance.

LGTM!

No need to do anything similar. Associated interface registry, unlikely
InterfaceRegistry, is a content-only stopgap concept used to support the
existence of Channel-associated interfaces. Any broader efforts to fit content
interface usage into the more general mold of updated service manager client
libraries (which is ultimately the intent of Ben's recent changes) are
irrelevant insofar as Channel-associated interface usage is concerned.

[Ken Rockot(use gerrit already), 2017-04-11 22:20:36]

https://codereview.chromium.org/2643183002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/140001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2092: } else if
(associated_registry_->CanBindRequest(interface_name)) {
Optional nit: You could add a binder for RouteProvider to associated_registry_
and get rid of the first branch here.

[nigeltao1, 2017-04-20 05:12:37]

https://codereview.chromium.org/2643183002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/140001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2092: } else if
(associated_registry_->CanBindRequest(interface_name)) {
On 2017/04/11 22:20:36, Ken Rockot wrote:
> Optional nit: You could add a binder for RouteProvider to associated_registry_
> and get rid of the first branch here.

Done, by adding some new code further up in this file
(content/browser/renderer_host/render_process_host_impl.cc). There's a little
bit of C++ trickery wrt overloaded method names, so I'm not 100% confident I've
got it right. PTAL.

[nigeltao1, 2017-04-20 05:12:50]

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

[commit-bot: I haz the power, 2017-04-20 05:13:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-04-20 15:49:42]

lgtm

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1331:
static_cast<AssociatedInterfaceRegistry*>(associated_registry_.get())
nit: This is unfortunate, but I really don't think the comment above is
necessary. We occasionally suffer casts like this in other places for the same
reason. If it becomes a problem we can change the interface.

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:466:
std::unique_ptr<AssociatedInterfaceRegistryImpl> associated_registry_;
nit: associated_interfaces_?

[nigeltao1, 2017-04-21 00:07:54]

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1331:
static_cast<AssociatedInterfaceRegistry*>(associated_registry_.get())
On 2017/04/20 15:49:42, Ken Rockot wrote:
> nit: This is unfortunate, but I really don't think the comment above is
> necessary. We occasionally suffer casts like this in other places for the same
> reason. If it becomes a problem we can change the interface.

Done.

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2643183002/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:466:
std::unique_ptr<AssociatedInterfaceRegistryImpl> associated_registry_;
On 2017/04/20 15:49:42, Ken Rockot wrote:
> nit: associated_interfaces_?

Done.

[nigeltao1, 2017-04-21 00:08:12]

sky, PTAL for OWNERS.

[nigeltao1, 2017-04-21 00:08:33]

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

[commit-bot: I haz the power, 2017-04-21 00:08:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 02:09:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 02:09:28]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-21 04:30:39]

LGTM - I didn't look at content as I'm not an owner there.

https://codereview.chromium.org/2643183002/diff/200001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_thread_observer.cc (right):

https://codereview.chromium.org/2643183002/diff/200001/chrome/renderer/chrome...
chrome/renderer/chrome_render_thread_observer.cc:124: if
(!cache_stats_recorder_)
{} (as body spans multiple lines).

[nigeltao1, 2017-04-21 23:53:36]

nigeltao@chromium.org changed reviewers:
+ lcwu@chromium.org, pfeldman@chromium.org

[nigeltao1, 2017-04-21 23:53:38]

lcwu, can you review for chromecast/OWNERS?

pfeldman, can you review for content/OWNERS?

[nigeltao1, 2017-04-21 23:54:09]

https://codereview.chromium.org/2643183002/diff/200001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_thread_observer.cc (right):

https://codereview.chromium.org/2643183002/diff/200001/chrome/renderer/chrome...
chrome/renderer/chrome_render_thread_observer.cc:124: if
(!cache_stats_recorder_)
On 2017/04/21 04:30:39, sky wrote:
> {} (as body spans multiple lines).

Done.

[nigeltao1, 2017-04-27 00:52:17]

nigeltao@chromium.org changed reviewers:
+ halliwell@chromium.org, nick@chromium.org
- lcwu@chromium.org, pfeldman@chromium.org

[nigeltao1, 2017-04-27 00:52:18]

halliwell, can you review for chromecast/OWNERS? lcwu isn't responding.

nick, can you review for content/OWNERS? pfeldman isn't responding.

[halliwell, 2017-04-27 01:52:04]

On 2017/04/27 00:52:18, nigeltao1 wrote:
> halliwell, can you review for chromecast/OWNERS? lcwu isn't responding.
> 
> nick, can you review for content/OWNERS? pfeldman isn't responding.

chromecast/ lgtm

[ncarter (slow), 2017-04-27 20:43:54]

lgtm

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2749:
route_provider_binding_.Close();
Does associated_interfaces_ need to be reset here?

In general I think we want we want to disconnect pipes at ProcessDied time, both
the free up memory, and to prevent exploitable behavior (a malicious process can
potentially fake its own death via e.g. fast shutdown, we don't want to still be
fielding IPCs).

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:462: 
// Mojo interfaces provided to the child process are registered here if
// they need consistent delivery ordering with legacy IPC, and are
// are process-wide in nature (e.g. metrics, memory usage).

[nigeltao1, 2017-04-28 00:33:25]

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2749:
route_provider_binding_.Close();
On 2017/04/27 20:43:53, ncarter wrote:
> Does associated_interfaces_ need to be reset here?
> 
> In general I think we want we want to disconnect pipes at ProcessDied time,
both
> the free up memory, and to prevent exploitable behavior (a malicious process
can
> potentially fake its own death via e.g. fast shutdown, we don't want to still
be
> fielding IPCs).

I think so. Done.

rockot: (1) WDYT? (2) PTAL at this .cc file.

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2643183002/diff/220001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:462: 
On 2017/04/27 20:43:53, ncarter wrote:
> // Mojo interfaces provided to the child process are registered here if
> // they need consistent delivery ordering with legacy IPC, and are
> // are process-wide in nature (e.g. metrics, memory usage).

Done.

[Ken Rockot(use gerrit already), 2017-04-28 19:20:10]

still lgtm

https://codereview.chromium.org/2643183002/diff/240001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2152: if
((associated_interfaces_) &&
nit: please remove the redundant parenthesis from both of these expressions.
there is no potential ambiguity to be guarded against and so they add no value

https://codereview.chromium.org/2643183002/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2814:
associated_interfaces_.reset(NULL);
nit: please use nullptr instead of NULL in new code. but here you should in fact
just reset().

It's totally fine if you really want to reset associated_interfaces_ here, but
that has absolutely nothing to do with closing existing bound interfaces. It's
just a map of callbacks that are used to bind incoming requests. If you want to
close bound interfaces too, you have to do that explicitly (e.g. look at how
route_provider_binding_ is closed above).

The use of StrongAssociatedBinding for your interface makes it impossible for
you to force-close the pipe at your own discretion. Because it's a
Channel-associated interface, however, it is impossible for it to outlive the
IPC::Channel anyway, so I don't think there's actually any risk here.

[nigeltao1, 2017-04-29 07:07:57]

https://codereview.chromium.org/2643183002/diff/240001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2643183002/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2152: if
((associated_interfaces_) &&
On 2017/04/28 19:20:10, Ken Rockot wrote:
> nit: please remove the redundant parenthesis from both of these expressions.
> there is no potential ambiguity to be guarded against and so they add no value

Done.

https://codereview.chromium.org/2643183002/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2814:
associated_interfaces_.reset(NULL);
On 2017/04/28 19:20:10, Ken Rockot wrote:
> nit: please use nullptr instead of NULL in new code. but here you should in
fact
> just reset().
> 
> It's totally fine if you really want to reset associated_interfaces_ here, but
> that has absolutely nothing to do with closing existing bound interfaces. It's
> just a map of callbacks that are used to bind incoming requests. If you want
to
> close bound interfaces too, you have to do that explicitly (e.g. look at how
> route_provider_binding_ is closed above).
> 
> The use of StrongAssociatedBinding for your interface makes it impossible for
> you to force-close the pipe at your own discretion. Because it's a
> Channel-associated interface, however, it is impossible for it to outlive the
> IPC::Channel anyway, so I don't think there's actually any risk here.

Done.

[nigeltao1, 2017-04-29 07:13:43]

https://codereview.chromium.org/2643183002/diff/260001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (left):

https://codereview.chromium.org/2643183002/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_message_filter.cc:89: #if
BUILDFLAG(ENABLE_PLUGINS)
FTR, I also expanded this #if a bit. I guess that this switch has been whittled
down in the time this CL has been in review.

[nigeltao1, 2017-04-29 07:14:08]

The CQ bit was checked by nigeltao@chromium.org

[nigeltao1, 2017-04-29 07:14:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
tsepez@chromium.org, sky@chromium.org, nick@chromium.org,
halliwell@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2643183002/#ps280001
(title: "Convert ChromeViewHostMsg_UpdatedCacheStats to use mojo.")

[commit-bot: I haz the power, 2017-04-29 07:14:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 09:30:40]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1493450048974100,
"parent_rev": "c566301a304c90ca1c1b05c9ed96e09b4c0392da", "commit_rev":
"beff0a104dbf869114f66db3efc2d52094050971"}

[commit-bot: I haz the power, 2017-04-29 09:30:53]

Description was changed from

==========
Convert ChromeViewHostMsg_UpdatedCacheStats to use mojo.

BUG=577685
==========

to

==========
Convert ChromeViewHostMsg_UpdatedCacheStats to use mojo.

BUG=577685

Review-Url: https://codereview.chromium.org/2643183002
Cr-Commit-Position: refs/heads/master@{#468234}
Committed:
https://chromium.googlesource.com/chromium/src/+/beff0a104dbf869114f66db3efc2...
==========

[commit-bot: I haz the power, 2017-04-29 09:30:54]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/beff0a104dbf869114f66db3efc2...