NTP: simplify mojo connection setup

NTP: simplify mojo connection setup

Both the browser (SearchIPCRouter) and the frame (SearchBox) needs to be able to
be able to send messages to the other side (i.e. using mojo methods with replies
is not enough). Previously this was achieved by having each side separate
creating a connection to the other. Now only the frame initiates the connection,
passing an interface to the browser which the browser can use to talk back to
the frame. This removes complicated frame connection logic on the browser side.

BUG=653373
Review-Url: https://codereview.chromium.org/2688453002
Cr-Commit-Position: refs/heads/master@{#452684}
Committed: https://chromium.googlesource.com/chromium/src/+/2e6a9ebbc75837e1f7986ecc7f16e5d59d70299a

[tibell, 2017-02-08 03:35:30]

Description was changed from

==========
NTP: simplify mojo connection setup

BUG=653373
==========

to

==========
NTP: simplify mojo connection setup

Instead of having the browser and frame create separate connections to
communicate the frame now creates the connection, passing an interface to the
browser which it can use to talk back to the frame. This removes complicated
frame connection logic on the browser side.

This change also allows us to reject connections from non-instant processes in
one place, instead of rejecting individual messages from such processes.




BUG=653373
==========

[tibell, 2017-02-08 03:42:04]

Description was changed from

==========
NTP: simplify mojo connection setup

Instead of having the browser and frame create separate connections to
communicate the frame now creates the connection, passing an interface to the
browser which it can use to talk back to the frame. This removes complicated
frame connection logic on the browser side.

This change also allows us to reject connections from non-instant processes in
one place, instead of rejecting individual messages from such processes.




BUG=653373
==========

to

==========
NTP: simplify mojo connection setup

Both the browser (SearchIPCRouter) and the frame (SearchBox) needs to be able to
be able to send messages to the other side (i.e. using mojo methods with replies
is not enough). Previously this was achieved by having each side separate
creating a connection to the other. Now only the frame initiates the connection,
passing an interface to the browser which the browser can use to talk back to
the frame. This removes complicated frame connection logic on the browser side.

This change also allows us to reject connections from non-instant processes in
one place, instead of rejecting individual messages from such processes.

BUG=653373
==========

[tibell, 2017-02-08 03:44:43]

tibell@chromium.org changed reviewers:
+ jam@chromium.org, treib@chromium.org

[tibell, 2017-02-08 03:44:43]

treib@ is main reviewer. jam@ for OWNERS.

[tibell, 2017-02-08 03:44:50]

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

[commit-bot: I haz the power, 2017-02-08 03:45:25]

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

[tibell, 2017-02-08 03:46:16]

tibell@chromium.org changed reviewers:
+ dcheng@chromium.org

[tibell, 2017-02-08 03:46:16]

Adding dcheng@ for security review.

[commit-bot: I haz the power, 2017-02-08 03:47:35]

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

[commit-bot: I haz the power, 2017-02-08 03:47:36]

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

[tibell, 2017-02-08 03:54:42]

Marc,

I'd like to separate rename these interfaces to something better. The methods
currently exposed on Instant seems to be about more things than instant search
(e.g. there are methods for most visited pages in there). What do you think
about renaming Instant to NTP and SearchBox to NTPClient in the .mojom file?

[tibell, 2017-02-08 04:05:51]

Merge

[tibell, 2017-02-08 04:05:53]

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

[commit-bot: I haz the power, 2017-02-08 04:06:12]

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-08 04:32:38]

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

[commit-bot: I haz the power, 2017-02-08 04:32:39]

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

[tibell, 2017-02-08 05:52:12]

Restore IsRenderedInInstantProcess check

[tibell, 2017-02-08 05:52:15]

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

[commit-bot: I haz the power, 2017-02-08 05:52:42]

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

[tibell, 2017-02-08 05:53:11]

Description was changed from

==========
NTP: simplify mojo connection setup

Both the browser (SearchIPCRouter) and the frame (SearchBox) needs to be able to
be able to send messages to the other side (i.e. using mojo methods with replies
is not enough). Previously this was achieved by having each side separate
creating a connection to the other. Now only the frame initiates the connection,
passing an interface to the browser which the browser can use to talk back to
the frame. This removes complicated frame connection logic on the browser side.

This change also allows us to reject connections from non-instant processes in
one place, instead of rejecting individual messages from such processes.

BUG=653373
==========

to

==========
NTP: simplify mojo connection setup

Both the browser (SearchIPCRouter) and the frame (SearchBox) needs to be able to
be able to send messages to the other side (i.e. using mojo methods with replies
is not enough). Previously this was achieved by having each side separate
creating a connection to the other. Now only the frame initiates the connection,
passing an interface to the browser which the browser can use to talk back to
the frame. This removes complicated frame connection logic on the browser side.

BUG=653373
==========

[commit-bot: I haz the power, 2017-02-08 06:37:47]

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

[commit-bot: I haz the power, 2017-02-08 06:37:48]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-08 09:01:39]

Also, can you help me understand why the checks had to be restored?

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:64: // Used to bind incomming
interface requests to the implementation, which
Nit: incomming -> incoming

https://codereview.chromium.org/2688453002/diff/40001/chrome/common/instant.m...
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/common/instant.m...
chrome/common/instant.mojom:22: // passed when connecting.
Thanks for adding these comments. Mind describing the purpose of the
Instant/SearchBox interfaces as well? =)

[Marc Treib, 2017-02-08 09:50:08]

On 2017/02/08 03:54:42, tibell wrote:
> Marc,
> 
> I'd like to separate rename these interfaces to something better. The methods
> currently exposed on Instant seems to be about more things than instant search
> (e.g. there are methods for most visited pages in there). What do you think
> about renaming Instant to NTP and SearchBox to NTPClient in the .mojom file?

Agreed that the current names aren't great; I'm definitely in favor of renaming
the interfaces to make things clearer. However, they aren't *only* about the
NTP, so naming them like that would be misleading too.
The official, public name of these APIs is "embeddedSearch", see
https://dev.chromium.org/embeddedsearch. So how about EmbeddedSearch and
EmbeddedSearchClient? And then we should probably rename SearchBox and
SearchIPCRouter as well, to match the interfaces. The "SearchIPCRouter" and
"SearchBox" names confuse me every time I look at this... (Of course, that can
wait for another CL.)

[Marc Treib, 2017-02-08 10:00:36]

This generally lg tm, with the usual caveat that I know next to nothing about
Mojo/IPC. It'd probably be good to get someone more familiar with Mojo to look
at this.

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:26: // based on the RenderView.
Does this describe a possible optimization that would have to be undone in an
OOPIF world? In that case, I wouldn't even call it a TODO.

Also, note that the IsRenderedInInstantProcess state of a WebContents can change
during its lifetime; not sure if that matters here.

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:60: // we've been connected to a
frame, messages sent on this interface goes into
nit: s/goes/go/

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:244: // desired.
I don't really understand what this means.
The NTP has two frames (main frame + iframe), both of which need the
embeddedSearch APIs. Does this CL break that?

https://codereview.chromium.org/2688453002/diff/40001/chrome/common/instant.m...
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/common/instant.m...
chrome/common/instant.mojom:23: interface NTPConnector {
Per my other comment: I don't think "NTP" is the right name here.
"EmbeddedSearchConnector"?

[jam, 2017-02-08 17:10:55]

On 2017/02/08 03:44:43, tibell wrote:
> treib@ is main reviewer. jam@ for OWNERS.

treib is owner in the search and searchbox directories, so you don't need me.

[tibell, 2017-02-09 03:32:47]

Address review comments

[tibell, 2017-02-09 03:32:50]

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

[commit-bot: I haz the power, 2017-02-09 03:33:41]

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

[tibell, 2017-02-09 03:35:27]

tibell@chromium.org changed reviewers:
- jam@chromium.org

[tibell, 2017-02-09 03:35:28]

PTAL

> Also, can you help me understand why the checks had to be restored?

Two reasons:

 * Lots of tests will have to be changed because they explicitly rely on this
behavior in *how* they test the code (i.e. it's a test problem, not a production
problem). I'd rather punt on that in this CL.

 * We really want to do this check on a per-frame instead of a per-tab level,
but that requires some changes to the InstantService code in the browser first.
I'll leave that to the people working on search.

(Even pre-mojo this code worked with per-message filtering.)

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:26: // based on the RenderView.
On 2017/02/08 10:00:36, Marc Treib wrote:
> Does this describe a possible optimization that would have to be undone in an
> OOPIF world? In that case, I wouldn't even call it a TODO.
> 
> Also, note that the IsRenderedInInstantProcess state of a WebContents can
change
> during its lifetime; not sure if that matters here.

I've updated the comment to what I think we should do in the future (including
in an OOPIF world). Basically we want a
search::IsRenderedInInstantProcess(RenderFrameHost*) method to check the current
frame. The process type of frame should change over its lifetime even under
OOPIF (right?).

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:60: // we've been connected to a
frame, messages sent on this interface goes into
On 2017/02/08 10:00:36, Marc Treib wrote:
> nit: s/goes/go/

Done.

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:64: // Used to bind incomming
interface requests to the implementation, which
On 2017/02/08 09:01:39, dcheng wrote:
> Nit: incomming -> incoming

Done.

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:244: // desired.
On 2017/02/08 10:00:36, Marc Treib wrote:
> I don't really understand what this means.
> The NTP has two frames (main frame + iframe), both of which need the
> embeddedSearch APIs. Does this CL break that?

tl;dr: this CL doesn't change any behavior, just how we set up the connections
that I established in my initial CL.

Ellaboration:

Before my initial conversion:
 * Renderer state was stored in SearchBox. The state was updated through
messages sent from SearchIPCRouter.
 * searchbox_extension.cc would look up the SearchBox through the RenderView and
get the state it needed. Both frames would look up the same SearchBox.

My initial CL:
 * Renderer state is still stored in SearchBox.
 * Changed SearchBox from a RenderViewObserver to a RenderFrameObserver and only
installed that observer on the main frame. This change was necessitated by Mojo
not working with RenderViews (because they're supposed to go away).
 * searchbox_extension.cc would instead look up the SearchBox by finding the
main frame (using a changed SearchBoxExtensionWrapper::GetRenderFrame) and get
the state it needs. This will have to be changed if we ever have the two frames
run in different processes under OOPIF.

This CL:
 * Nothing changes w.r.t. how the renderer gets to the state. The state is still
on the main frame (in SearchBox). It's the connection from that main frame
that's stored in |binding_|.

What we could do in the future:

Option A:
 * If duplicating the state on the renderer side is not a problem we could a)
have one SearchBox per frame and b) have each frame connect to SearchIPCRouter.
This would require changing |binding_| to an AssociatedBindingSet and replace
search_box_ with e.g. a vector of InterfacePtrs to update when the browser state
changes.

Option B:
 * If we ever get a new abstraction for tabs on the renderer side (i.e. a new
RenderView): have a Mojo connection between WebContentsObserver and this tab
representation and then keep the renderer state on a per-tab basis again.

Option C:
 * Don't store state in the renderer and instead just ask the browser for it
when needed. I don't know if this is possible e.g. it won't work if the
embeddedSearch JS APIs require synchronous access to the data.

https://codereview.chromium.org/2688453002/diff/40001/chrome/common/instant.m...
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/common/instant.m...
chrome/common/instant.mojom:22: // passed when connecting.
On 2017/02/08 09:01:39, dcheng wrote:
> Thanks for adding these comments. Mind describing the purpose of the
> Instant/SearchBox interfaces as well? =)

I've added some more comments to express my current understanding.

https://codereview.chromium.org/2688453002/diff/40001/chrome/common/instant.m...
chrome/common/instant.mojom:23: interface NTPConnector {
On 2017/02/08 10:00:36, Marc Treib wrote:
> Per my other comment: I don't think "NTP" is the right name here.
> "EmbeddedSearchConnector"?

Done.

[tibell, 2017-02-09 03:39:01]

Get rid of more references to NTP

[tibell, 2017-02-09 03:39:04]

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

[commit-bot: I haz the power, 2017-02-09 03:40:00]

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

[tibell, 2017-02-09 03:41:56]

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

[tibell, 2017-02-09 03:41:57]

Re-adding jam@ as treib@ is not in OWNERS.

[tibell, 2017-02-09 04:12:45]

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

[commit-bot: I haz the power, 2017-02-09 04:13: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-02-09 05:13:53]

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

[commit-bot: I haz the power, 2017-02-09 05:13:54]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-02-09 09:30:47]

lgtm

Thanks!

(I *am* in owners for everything except the .mojom file. Rietveld just doesn't
understand referenced owners files.)

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2688453002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:244: // desired.
Thanks for the detailed explanation!

On 2017/02/09 03:35:28, tibell wrote:
> On 2017/02/08 10:00:36, Marc Treib wrote:
> > I don't really understand what this means.
> > The NTP has two frames (main frame + iframe), both of which need the
> > embeddedSearch APIs. Does this CL break that?
> 
> tl;dr: this CL doesn't change any behavior, just how we set up the connections
> that I established in my initial CL.
> 
> Ellaboration:
> 
> Before my initial conversion:
>  * Renderer state was stored in SearchBox. The state was updated through
> messages sent from SearchIPCRouter.
>  * searchbox_extension.cc would look up the SearchBox through the RenderView
and
> get the state it needed. Both frames would look up the same SearchBox.
> 
> My initial CL:
>  * Renderer state is still stored in SearchBox.
>  * Changed SearchBox from a RenderViewObserver to a RenderFrameObserver and
only
> installed that observer on the main frame. This change was necessitated by
Mojo
> not working with RenderViews (because they're supposed to go away).
>  * searchbox_extension.cc would instead look up the SearchBox by finding the
> main frame (using a changed SearchBoxExtensionWrapper::GetRenderFrame) and get
> the state it needs. This will have to be changed if we ever have the two
frames
> run in different processes under OOPIF.
> 
> This CL:
>  * Nothing changes w.r.t. how the renderer gets to the state. The state is
still
> on the main frame (in SearchBox). It's the connection from that main frame
> that's stored in |binding_|.
> 
> What we could do in the future:
> 
> Option A:
>  * If duplicating the state on the renderer side is not a problem we could a)
> have one SearchBox per frame and b) have each frame connect to
SearchIPCRouter.
> This would require changing |binding_| to an AssociatedBindingSet and replace
> search_box_ with e.g. a vector of InterfacePtrs to update when the browser
state
> changes.

This sounds like it would fit well with OOPIF.

> Option B:
>  * If we ever get a new abstraction for tabs on the renderer side (i.e. a new
> RenderView): have a Mojo connection between WebContentsObserver and this tab
> representation and then keep the renderer state on a per-tab basis again.

If that new RenderView happens, then this sound like it'd be simpler than a).

> Option C:
>  * Don't store state in the renderer and instead just ask the browser for it
> when needed. I don't know if this is possible e.g. it won't work if the
> embeddedSearch JS APIs require synchronous access to the data.

They do have a bunch of synchronous getters, so I think this option is out.

https://codereview.chromium.org/2688453002/diff/80001/chrome/common/instant.m...
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2688453002/diff/80001/chrome/common/instant.m...
chrome/common/instant.mojom:120: // the correspnding methods in the Instant
interface above.
Ah, is this something made possible by Mojo? Nice!

[tibell, 2017-02-09 22:31:37]

On 2017/02/09 09:30:47, Marc Treib wrote:
> lgtm
> 
> Thanks!
> 
> (I *am* in owners for everything except the .mojom file. Rietveld just doesn't
> understand referenced owners files.)

I tried to manually follow all those referenced owners files and didn't find you
before I removed jam@. I don't know what I did wrong. :/ In any case I've
removed jam@ again.

[tibell, 2017-02-09 22:32:01]

dcheng, I need your security review.

https://codereview.chromium.org/2688453002/diff/80001/chrome/common/instant.m...
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2688453002/diff/80001/chrome/common/instant.m...
chrome/common/instant.mojom:120: // the correspnding methods in the Instant
interface above.
On 2017/02/09 09:30:47, Marc Treib wrote:
> Ah, is this something made possible by Mojo? Nice!

Yup! It's convenient and also removes the need to use e.g. tokens in the
messages to associate requests with responses. The method implementor just gets
passed a base::Callback that is used to reply.

[tibell, 2017-02-09 22:32:18]

tibell@chromium.org changed reviewers:
- jam@chromium.org

[dcheng, 2017-02-10 10:24:14]

https://codereview.chromium.org/2688453002/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2688453002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:79: if
(!IsRenderedInInstantProcess(web_contents_)) {
One thing I was wondering... once a renderer process is an instant process, it's
always an instant process, right? Thus, if we guarantee that we swap renderers
when loading an instant page, we don't need to manually close the pipes on
navigation. Is this true today?

Another thought is if it's OK that this interface method can be reached by a
non-instant process... I think that should be OK, given that we filter like
this.

[tibell, 2017-02-12 23:15:26]

https://codereview.chromium.org/2688453002/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2688453002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:79: if
(!IsRenderedInInstantProcess(web_contents_)) {
On 2017/02/10 10:24:14, dcheng wrote:
> One thing I was wondering... once a renderer process is an instant process,
it's
> always an instant process, right? Thus, if we guarantee that we swap renderers
> when loading an instant page, we don't need to manually close the pipes on
> navigation. Is this true today?
> 
> Another thought is if it's OK that this interface method can be reached by a
> non-instant process... I think that should be OK, given that we filter like
> this.

A renderer should stay the same process type during its lifetime, so no need to
manually close its connection. This is true today AFAIK.

It's fine if non-instant processes try to connect here, give this filtering.

As I explained in the very long comment to one of Marc's other questions,
ideally someone would change InstantService to expose a
IsRenderedInInstantProcess(RenderFrame*). Having that and putting in some
efforts in rewriting the tests this code could be nicer (i.e. we'd *only* check
process type on connection but not on each message received.)

[tibell, 2017-02-13 00:37:48]

Check connections on a per-frame basis

[tibell, 2017-02-13 00:37:54]

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

[commit-bot: I haz the power, 2017-02-13 00:38:03]

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-13 00:39:34]

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

[commit-bot: I haz the power, 2017-02-13 00:39:35]

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

[tibell, 2017-02-13 00:41:31]

PTAL

I've uploaded a version that does the "is instant process" check correctly on a
per frame basis. In the process I've deleted two tests that no longer makes
sense.

* One that checks that messages get filtered. This is instead covered by the
connection logic.
* One that checks that navigating away from the NTP causes future messages to
get filtered. This is covered by the RenderFrame being deleted on navigation and
thus the connections severed.

[tibell, 2017-02-13 00:45:43]

Merge

[tibell, 2017-02-13 00:45:46]

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

[commit-bot: I haz the power, 2017-02-13 00:46: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-02-13 01:04:22]

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

[commit-bot: I haz the power, 2017-02-13 01:04:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tibell, 2017-02-13 02:04:37]

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

[commit-bot: I haz the power, 2017-02-13 02:04:53]

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-13 02:47:18]

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

[commit-bot: I haz the power, 2017-02-13 02:47:19]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-02-13 09:35:57]

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:24: bool
IsInstantProcess(content::RenderFrameHost* render_frame) {
nit: IsInInstantProcess maybe?

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:42: : binding_(binding),
bindings_(web_contents, this) {
There's both "binding_" and "bindings_" which I find quite confusing. Can we
find more distinctive names?

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (left):

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router_unittest.cc:447: // Navigate away
from the NTP. Afterwards, all messages should be ignored.
It's not clear to me why it's safe to remove these tests. Unless there are other
tests covering the behavior, we should add some.

[dcheng, 2017-02-13 21:06:58]

I think I'm largely with OK with this change. I think it does slightly change
the semantics (before, only the main frame could bind; now any one frame can
bind), but that's /probably/ OK. I think I would feel better if we limited it to
the main frame and then expanded the scope in a followup CL though: does that
sound reasonable?

[tibell, 2017-02-19 23:18:19]

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

[commit-bot: I haz the power, 2017-02-19 23:18:26]

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

[tibell, 2017-02-19 23:18:33]

On 2017/02/13 21:06:58, dcheng wrote:
> I think I'm largely with OK with this change. I think it does slightly change
> the semantics (before, only the main frame could bind; now any one frame can
> bind), but that's /probably/ OK. I think I would feel better if we limited it
to
> the main frame and then expanded the scope in a followup CL though: does that
> sound reasonable?

Done.

[tibell, 2017-02-19 23:18:45]

PTAL

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:24: bool
IsInstantProcess(content::RenderFrameHost* render_frame) {
On 2017/02/13 09:35:56, Marc Treib wrote:
> nit: IsInInstantProcess maybe?

Done.

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:42: : binding_(binding),
bindings_(web_contents, this) {
On 2017/02/13 09:35:56, Marc Treib wrote:
> There's both "binding_" and "bindings_" which I find quite confusing. Can we
> find more distinctive names?

Done.

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (left):

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router_unittest.cc:447: // Navigate away
from the NTP. Afterwards, all messages should be ignored.
On 2017/02/13 09:35:57, Marc Treib wrote:
> It's not clear to me why it's safe to remove these tests. Unless there are
other
> tests covering the behavior, we should add some.

Mojo being connection oriented helps us here. If we start in the state where the
main frame of the NTP has a connection to the SearchIPCRouter and we navigate
away from the NTP then 1) the old frame gets destroyed and all its Mojo
connections with it and 2) the new frame is not allowed to set up a connection,
because it fails the IsInInstantProcess test.

What we could test is that a non-instant frame isn't allowed to connect to the
SearchIPCRouter. I'm still relatively new to Chrome so I'm not sure how to set
up such a test using our testing framework. Thinking about this it seems like a
browser test wouldn't work, because the way we instantiate the SearchBox ensures
it doesn't get attached to a non-instant frame to begin with. We'd need to write
a test that creates a "custom" frame that is non-instant but still has a
SearchBox attached to it. Any idea how to do that?

[commit-bot: I haz the power, 2017-02-19 23:26:52]

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

[commit-bot: I haz the power, 2017-02-19 23:26:53]

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

[tibell, 2017-02-20 00:14:54]

Merge

[tibell, 2017-02-20 00:14:58]

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

[commit-bot: I haz the power, 2017-02-20 00:15:12]

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-20 01:14:55]

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

[commit-bot: I haz the power, 2017-02-20 01:14:56]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-02-20 10:14:52]

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (left):

https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router_unittest.cc:447: // Navigate away
from the NTP. Afterwards, all messages should be ignored.
On 2017/02/19 23:18:45, tibell wrote:
> On 2017/02/13 09:35:57, Marc Treib wrote:
> > It's not clear to me why it's safe to remove these tests. Unless there are
> other
> > tests covering the behavior, we should add some.
> 
> Mojo being connection oriented helps us here. If we start in the state where
the
> main frame of the NTP has a connection to the SearchIPCRouter and we navigate
> away from the NTP then 1) the old frame gets destroyed and all its Mojo
> connections with it and 2) the new frame is not allowed to set up a
connection,
> because it fails the IsInInstantProcess test.

That sounds reasonable (to the engineer who has no idea about IPC/Mojo :D).

> What we could test is that a non-instant frame isn't allowed to connect to the
> SearchIPCRouter. I'm still relatively new to Chrome so I'm not sure how to set
> up such a test using our testing framework. Thinking about this it seems like
a
> browser test wouldn't work, because the way we instantiate the SearchBox
ensures
> it doesn't get attached to a non-instant frame to begin with. We'd need to
write
> a test that creates a "custom" frame that is non-instant but still has a
> SearchBox attached to it. Any idea how to do that?

I'm not really familiar enough with Mojo and the whole IPC setup to advise here,
but it seems like it should be possible to just manually create a SearchBox
instance for a given RenderFrame? Though after your explanations, I'm not sure
how useful such a test would be.

In related news, I've been working on adding some end-to-end integration test
for the NTP, including the embeddedSearch APIs, see e.g.
https://codereview.chromium.org/2695813012/ (pending). It should be fairly easy
to add a test that navigates to an NTP and away again, and makes sure the APIs
are only available on the NTP.
(Doesn't have to happen in this CL since it's somewhat orthogonal. But it would
be great to have this sort of coverage before making more/bigger changes to the
IPC setup. WDYT?)

[dcheng, 2017-02-21 23:29:34]

On 2017/02/19 23:18:45, tibell wrote:
> PTAL
> 
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> File chrome/browser/ui/search/search_ipc_router.cc (right):
> 
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> chrome/browser/ui/search/search_ipc_router.cc:24: bool
> IsInstantProcess(content::RenderFrameHost* render_frame) {
> On 2017/02/13 09:35:56, Marc Treib wrote:
> > nit: IsInInstantProcess maybe?
> 
> Done.
> 
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> chrome/browser/ui/search/search_ipc_router.cc:42: : binding_(binding),
> bindings_(web_contents, this) {
> On 2017/02/13 09:35:56, Marc Treib wrote:
> > There's both "binding_" and "bindings_" which I find quite confusing. Can we
> > find more distinctive names?
> 
> Done.
> 
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> File chrome/browser/ui/search/search_ipc_router_unittest.cc (left):
> 
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> chrome/browser/ui/search/search_ipc_router_unittest.cc:447: // Navigate away
> from the NTP. Afterwards, all messages should be ignored.
> On 2017/02/13 09:35:57, Marc Treib wrote:
> > It's not clear to me why it's safe to remove these tests. Unless there are
> other
> > tests covering the behavior, we should add some.
> 
> Mojo being connection oriented helps us here. If we start in the state where
the
> main frame of the NTP has a connection to the SearchIPCRouter and we navigate
> away from the NTP then 1) the old frame gets destroyed and all its Mojo
> connections with it and 2) the new frame is not allowed to set up a
connection,
> because it fails the IsInInstantProcess test.
> 
> What we could test is that a non-instant frame isn't allowed to connect to the
> SearchIPCRouter. I'm still relatively new to Chrome so I'm not sure how to set
> up such a test using our testing framework. Thinking about this it seems like
a
> browser test wouldn't work, because the way we instantiate the SearchBox
ensures
> it doesn't get attached to a non-instant frame to begin with. We'd need to
write
> a test that creates a "custom" frame that is non-instant but still has a
> SearchBox attached to it. Any idea how to do that?

With IPC, we had things like
https://cs.chromium.org/chromium/src/content/browser/security_exploit_browser...
to test IPCs coming from a simulated bad renderer. There doesn't seem to be a
mojo equivalent for that though.

LGTM, but let's file a followup bug to figure out some way to do exploited
renderer testing.

[tibell, 2017-02-23 02:49:54]

On 2017/02/20 10:14:52, Marc Treib wrote:
> > What we could test is that a non-instant frame isn't allowed to connect to
the
> > SearchIPCRouter. I'm still relatively new to Chrome so I'm not sure how to
set
> > up such a test using our testing framework. Thinking about this it seems
like
> a
> > browser test wouldn't work, because the way we instantiate the SearchBox
> ensures
> > it doesn't get attached to a non-instant frame to begin with. We'd need to
> write
> > a test that creates a "custom" frame that is non-instant but still has a
> > SearchBox attached to it. Any idea how to do that?
> 
> I'm not really familiar enough with Mojo and the whole IPC setup to advise
here,
> but it seems like it should be possible to just manually create a SearchBox
> instance for a given RenderFrame? Though after your explanations, I'm not sure
> how useful such a test would be.

That sounds about right, can you do that in a browser test?

> In related news, I've been working on adding some end-to-end integration test
> for the NTP, including the embeddedSearch APIs, see e.g.
> https://codereview.chromium.org/2695813012/ (pending). It should be fairly
easy
> to add a test that navigates to an NTP and away again, and makes sure the APIs
> are only available on the NTP.
> (Doesn't have to happen in this CL since it's somewhat orthogonal. But it
would
> be great to have this sort of coverage before making more/bigger changes to
the
> IPC setup. WDYT?)

Looks great. We do have good unit test coverage, but some end-to-end testing is
very welcome.

With the changes dcheng@ asked for, is there anything else you want to see
before submitting?

[tibell, 2017-02-23 02:52:41]

On 2017/02/21 23:29:34, dcheng wrote:
> On 2017/02/19 23:18:45, tibell wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> > File chrome/browser/ui/search/search_ipc_router.cc (right):
> > 
> >
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> > chrome/browser/ui/search/search_ipc_router.cc:24: bool
> > IsInstantProcess(content::RenderFrameHost* render_frame) {
> > On 2017/02/13 09:35:56, Marc Treib wrote:
> > > nit: IsInInstantProcess maybe?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> > chrome/browser/ui/search/search_ipc_router.cc:42: : binding_(binding),
> > bindings_(web_contents, this) {
> > On 2017/02/13 09:35:56, Marc Treib wrote:
> > > There's both "binding_" and "bindings_" which I find quite confusing. Can
we
> > > find more distinctive names?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> > File chrome/browser/ui/search/search_ipc_router_unittest.cc (left):
> > 
> >
>
https://codereview.chromium.org/2688453002/diff/130001/chrome/browser/ui/sear...
> > chrome/browser/ui/search/search_ipc_router_unittest.cc:447: // Navigate away
> > from the NTP. Afterwards, all messages should be ignored.
> > On 2017/02/13 09:35:57, Marc Treib wrote:
> > > It's not clear to me why it's safe to remove these tests. Unless there are
> > other
> > > tests covering the behavior, we should add some.
> > 
> > Mojo being connection oriented helps us here. If we start in the state where
> the
> > main frame of the NTP has a connection to the SearchIPCRouter and we
navigate
> > away from the NTP then 1) the old frame gets destroyed and all its Mojo
> > connections with it and 2) the new frame is not allowed to set up a
> connection,
> > because it fails the IsInInstantProcess test.
> > 
> > What we could test is that a non-instant frame isn't allowed to connect to
the
> > SearchIPCRouter. I'm still relatively new to Chrome so I'm not sure how to
set
> > up such a test using our testing framework. Thinking about this it seems
like
> a
> > browser test wouldn't work, because the way we instantiate the SearchBox
> ensures
> > it doesn't get attached to a non-instant frame to begin with. We'd need to
> write
> > a test that creates a "custom" frame that is non-instant but still has a
> > SearchBox attached to it. Any idea how to do that?
> 
> With IPC, we had things like
>
https://cs.chromium.org/chromium/src/content/browser/security_exploit_browser...
> to test IPCs coming from a simulated bad renderer. There doesn't seem to be a
> mojo equivalent for that though.
> 
> LGTM, but let's file a followup bug to figure out some way to do exploited
> renderer testing.

Filed http://crbug.com/695295

[Marc Treib, 2017-02-23 10:27:00]

On 2017/02/23 02:49:54, tibell wrote:
> On 2017/02/20 10:14:52, Marc Treib wrote:
> > > What we could test is that a non-instant frame isn't allowed to connect to
> the
> > > SearchIPCRouter. I'm still relatively new to Chrome so I'm not sure how to
> set
> > > up such a test using our testing framework. Thinking about this it seems
> like
> > a
> > > browser test wouldn't work, because the way we instantiate the SearchBox
> > ensures
> > > it doesn't get attached to a non-instant frame to begin with. We'd need to
> > write
> > > a test that creates a "custom" frame that is non-instant but still has a
> > > SearchBox attached to it. Any idea how to do that?
> > 
> > I'm not really familiar enough with Mojo and the whole IPC setup to advise
> here,
> > but it seems like it should be possible to just manually create a SearchBox
> > instance for a given RenderFrame? Though after your explanations, I'm not
sure
> > how useful such a test would be.
> 
> That sounds about right, can you do that in a browser test?

I guess so? Possibly even in a unit test?

> > In related news, I've been working on adding some end-to-end integration
test
> > for the NTP, including the embeddedSearch APIs, see e.g.
> > https://codereview.chromium.org/2695813012/ (pending). It should be fairly
> easy
> > to add a test that navigates to an NTP and away again, and makes sure the
APIs
> > are only available on the NTP.
> > (Doesn't have to happen in this CL since it's somewhat orthogonal. But it
> would
> > be great to have this sort of coverage before making more/bigger changes to
> the
> > IPC setup. WDYT?)
> 
> Looks great. We do have good unit test coverage, but some end-to-end testing
is
> very welcome.

I've recently landed a test that makes sure the API comes and goes as expected:
https://codereview.chromium.org/2704373002

> With the changes dcheng@ asked for, is there anything else you want to see
> before submitting?

Nope, I think this is good to land.
Thanks!

[tibell, 2017-02-23 23:05:15]

The CQ bit was checked by tibell@chromium.org

[tibell, 2017-02-23 23:05:16]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2688453002/#ps170001
(title: "Merge")

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

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-24 00:12:22]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1487891115657840,
"parent_rev": "2be5e825df1ea527e38e213763bef38d4a29f177", "commit_rev":
"2e6a9ebbc75837e1f7986ecc7f16e5d59d70299a"}

[commit-bot: I haz the power, 2017-02-24 00:12:59]

Description was changed from

==========
NTP: simplify mojo connection setup

Both the browser (SearchIPCRouter) and the frame (SearchBox) needs to be able to
be able to send messages to the other side (i.e. using mojo methods with replies
is not enough). Previously this was achieved by having each side separate
creating a connection to the other. Now only the frame initiates the connection,
passing an interface to the browser which the browser can use to talk back to
the frame. This removes complicated frame connection logic on the browser side.

BUG=653373
==========

to

==========
NTP: simplify mojo connection setup

Both the browser (SearchIPCRouter) and the frame (SearchBox) needs to be able to
be able to send messages to the other side (i.e. using mojo methods with replies
is not enough). Previously this was achieved by having each side separate
creating a connection to the other. Now only the frame initiates the connection,
passing an interface to the browser which the browser can use to talk back to
the frame. This removes complicated frame connection logic on the browser side.

BUG=653373

Review-Url: https://codereview.chromium.org/2688453002
Cr-Commit-Position: refs/heads/master@{#452684}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e6a9ebbc75837e1f7986ecc7f16...
==========

[commit-bot: I haz the power, 2017-02-24 00:13:01]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/2e6a9ebbc75837e1f7986ecc7f16...