Pass navigator.connect calls through to the browser process.

Pass navigator.connect calls through to the browser process.

Depends on blink side change in https://codereview.chromium.org/782463002/
and on the blink side change in https://codereview.chromium.org/799703003/

BUG=426458
Committed: https://crrev.com/6ef5c59d66a61a1f5002b3fa2d5dda0ecd41886b
Cr-Commit-Position: refs/heads/master@{#308247}

[Marijn Kruisselbrink, 2014-12-04 14:02:01]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2014-12-04 14:08:35]

mek@chromium.org changed reviewers:
+ scheib@chromium.org

[Marijn Kruisselbrink, 2014-12-04 14:08:36]

It seems a bit overkill to have a NavigatorConnectDispatcher,
NavigatorConnectDispatcherHost, navigator_connect_messages.h etc for what is and
probably always will be a single method, but it doesn't really fit in with
anything else either. In my proof-of-concept end-to-end working
navigator.connect implementation (https://codereview.chromium.org/668303004/) I
just squeezed all of this in with service worker messages and dispatchers, but
that doesn't quite make sense, certainly in the long term either.
(I also played around with using mojo for this IPC and having a
NavigatorConnectService mojo interface, but that becomes rather complicated too
with mojo not really supporting multi-threaded code; navigator.connect can be
called from multiple threads. All solvable problems of course, but not as simple
and clean as I had hoped)

[scheib, 2014-12-05 05:49:50]

Generally looks reasonable, though yes this is more code and complexity than
should be required. ;( It's confusing, class comments will go a long way. It's
sad we have a few highly similar patterns and I'm not sure why they differ (e.g.
geofencing vs push).

https://codereview.chromium.org/781723002/diff/20001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_dispatcher_host.h
(right):

https://codereview.chromium.org/781723002/diff/20001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_dispatcher_host.h:14: class
NavigatorConnectDispatcherHost : public BrowserMessageFilter {
Class comments
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments
(for all classes in patch). Lifetime, thread usage, etc. I did an ~OK job on
first pass Bluetooth, perhaps could do better
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_dispatcher.cc (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_dispatcher.cc:38:
IPC_MESSAGE_FORWARD(NavigatorConnectMsg_ConnectResult,
Why not the pattern as in GeofencingMessageFilter, which seemed easier to read?
Is there a functional difference?
  if (IPC_MESSAGE_CLASS(msg) != GeofencingMsgStart)
    return false;
  GeofencingDispatcher::GetOrCreateThreadSpecificInstance(
      thread_safe_sender_.get())->OnMessageReceived(msg);
  return true;

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_dispatcher.h (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_dispatcher.h:18: class
NavigatorConnectDispatcher : public ChildMessageFilter {
Dispatcher or Filter? A Filter runs on IPC-IO thread and filters messages to
correct handlers? I'm weak on the terminology difference, but it is interesting
to see a Dispatcher is-a filter here. Class comments should help.

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_provider.cc (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.cc:61:
DCHECK(callbacks);
DCHECK and crash, or handle it, not both:
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_provider.h (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.h:26: class
NavigatorConnectProvider : public blink::WebNavigatorConnectProvider,
Definitely mention ownership of instances of this per thread, and relationship
to filter/dispatcher.

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.h:41: static
NavigatorConnectProvider* ThreadSpecificInstance(
Perhaps GetOrCreateThreadspecificInstance

[Marijn Kruisselbrink, 2014-12-05 14:25:50]

https://codereview.chromium.org/781723002/diff/20001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_dispatcher_host.h
(right):

https://codereview.chromium.org/781723002/diff/20001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_dispatcher_host.h:14: class
NavigatorConnectDispatcherHost : public BrowserMessageFilter {
On 2014/12/05 05:49:50, scheib wrote:
> Class comments
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments
> (for all classes in patch). Lifetime, thread usage, etc. I did an ~OK job on
> first pass Bluetooth, perhaps could do better
>
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...

Done.

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_dispatcher.cc (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_dispatcher.cc:38:
IPC_MESSAGE_FORWARD(NavigatorConnectMsg_ConnectResult,
On 2014/12/05 05:49:50, scheib wrote:
> Why not the pattern as in GeofencingMessageFilter, which seemed easier to
read?
> Is there a functional difference?
>   if (IPC_MESSAGE_CLASS(msg) != GeofencingMsgStart)
>     return false;
>   GeofencingDispatcher::GetOrCreateThreadSpecificInstance(
>       thread_safe_sender_.get())->OnMessageReceived(msg);
>   return true;
No functional difference, no. Just saving some lines of code by having this code
directly dispatch to the correct NavigatorConnectProvider methods instead of
going though another OnMessageReceived method. But consistency is probably
better indeed, so I changed it to match the others.

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_dispatcher.h (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_dispatcher.h:18: class
NavigatorConnectDispatcher : public ChildMessageFilter {
On 2014/12/05 05:49:50, scheib wrote:
> Dispatcher or Filter? A Filter runs on IPC-IO thread and filters messages to
> correct handlers? I'm weak on the terminology difference, but it is
interesting
> to see a Dispatcher is-a filter here. Class comments should help.
I don't know why some code calls it Dispatcher and some code calls it
MessageFilter... In this case I followed the push/notifications example of
calling this class Dispatcher although I don't really have strong reasons for
that (other than at the time it seemed like it would be less confusing, since it
sort of avoids overloading the term Message too much especially in code like
this that deals with messages and messageports in a different meaning).

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_provider.cc (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.cc:61:
DCHECK(callbacks);
On 2014/12/05 05:49:50, scheib wrote:
> DCHECK and crash, or handle it, not both:
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

Hmm, yeah, not sure how I ended up with code like this in the geofencing code...
I've gotten rid of the if here, and will try to remember to clean up the
geofencing code similarly.

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_provider.h (right):

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.h:26: class
NavigatorConnectProvider : public blink::WebNavigatorConnectProvider,
On 2014/12/05 05:49:50, scheib wrote:
> Definitely mention ownership of instances of this per thread, and relationship
> to filter/dispatcher.

Done.

https://codereview.chromium.org/781723002/diff/20001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.h:41: static
NavigatorConnectProvider* ThreadSpecificInstance(
On 2014/12/05 05:49:50, scheib wrote:
> Perhaps GetOrCreateThreadspecificInstance
Maybe... PushProvider and NotificationManager call this just
ThreadSpecificInstance, but other classes indeed call it
GetOrCreateThreadspecificInstance. I don't really have a strong opinion either
way.

[scheib, 2014-12-05 18:59:35]

lgtm

https://codereview.chromium.org/781723002/diff/40001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_provider.cc (right):

https://codereview.chromium.org/781723002/diff/40001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.cc:80:
NavigatorConnectProvider* NavigatorConnectProvider::ThreadSpecificInstance(
Keep definitions matching the declaration order.

[Marijn Kruisselbrink, 2014-12-09 23:23:31]

https://codereview.chromium.org/781723002/diff/40001/content/child/navigator_...
File content/child/navigator_connect/navigator_connect_provider.cc (right):

https://codereview.chromium.org/781723002/diff/40001/content/child/navigator_...
content/child/navigator_connect/navigator_connect_provider.cc:80:
NavigatorConnectProvider* NavigatorConnectProvider::ThreadSpecificInstance(
On 2014/12/05 18:59:35, scheib wrote:
> Keep definitions matching the declaration order.

Done.

[Marijn Kruisselbrink, 2014-12-09 23:24:59]

mek@chromium.org changed reviewers:
+ avi@chromium.org, palmer@chromium.org

[Marijn Kruisselbrink, 2014-12-09 23:24:59]

+avi for content/ OWNERS
+palmer for IPC OWNERS

[Avi (use Gerrit), 2014-12-10 00:17:32]

lgtm

stampity stamp

https://codereview.chromium.org/781723002/diff/80001/content/browser/navigato...
File content/browser/navigator_connect/OWNERS (right):

https://codereview.chromium.org/781723002/diff/80001/content/browser/navigato...
content/browser/navigator_connect/OWNERS:1: mek@chromium.org
Can you get a secondary for this file?

[palmer, 2014-12-10 01:40:45]

https://codereview.chromium.org/781723002/diff/80001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_dispatcher_host.cc
(right):

https://codereview.chromium.org/781723002/diff/80001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_dispatcher_host.cc:33: //
TODO(mek): Actually setup a connection.
Should there be any check that the |target_url| has permission to talk
to/through the given message port? Does the |target_url| have to be in the same
origin as the code on the other end of the message port, or...?

[Marijn Kruisselbrink, 2014-12-10 17:14:26]

https://codereview.chromium.org/781723002/diff/80001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_dispatcher_host.cc
(right):

https://codereview.chromium.org/781723002/diff/80001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_dispatcher_host.cc:33: //
TODO(mek): Actually setup a connection.
On 2014/12/10 01:40:44, Chromium Palmer wrote:
> Should there be any check that the |target_url| has permission to talk
> to/through the given message port? Does the |target_url| have to be in the
same
> origin as the code on the other end of the message port, or...?
It is completely up to the (service worker controlling the) target_url if it
wants to accept or reject a connection. And generally |target_url| will be in a
different origin than the code on the other end of the message port. For the
target url to be able to make this decision, the idea currently is to give it
access to the origin of the page/worker that called navigator.connect (see
http://mkruisselbrink.github.io/navigator-connect/#idl-def-CrossOriginService...
for the interface the service worker will have access to when having to decide
if it should allow or deny a connection attempt).
Currently this origin of the client attempting to make a connection isn't passed
through yet (but then it isn't possible to actually setup a connection yet
either).

I suppose I could change the code to make it the responsibility of the service
to create a message port pair when it accepts a connection (and then pass one
port back from the service to the client), although it doesn't seem like that
really is any more secure than the current design where the client creates a
message port pair passing one port off to the service. Either way the browser
process will be in between and can ensure no messages are passed until the
connection as been accepted by the service.

[chromium-reviews, 2014-12-11 22:08:31]

I wrote a brilliant comment, but it was eaten. Sigh. :( In fact
Rietveld seems totally broken now... Email it is.

Basically, we should definitely provide at least the ability for the
callee to learn the caller's origin imperatively, and also we should
probably provide a declarative API allowing SWs to say "I expect only
callers from these origins: A, B, C...". That's a major saving grace
in Android IPC; generally, IPC endpoint authors at large are not sure
how to defend against malicious callers and that declarative way of
limiting callers is the way to go.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2014-12-11 22:08:57]

I wrote a brilliant comment, but it was eaten. Sigh. :(

Basically, we should definitely provide at least the ability for the callee to
learn the caller's origin imperatively, and also we should probably provide a
declarative API allowing SWs to say "I expect only callers from these origins:
A, B, C...". That's a major saving grace in Android IPC; generally, IPC endpoint
authors at large are not sure how to defend against malicious callers and that
declarative way of limiting callers is the way to go.

[Marijn Kruisselbrink, 2014-12-12 00:06:25]

On 2014/12/11 22:08:57, Chromium Palmer wrote:
> I wrote a brilliant comment, but it was eaten. Sigh. :(
> 
> Basically, we should definitely provide at least the ability for the callee to
> learn the caller's origin imperatively,
Agreed, that was already in the spec and planned, I just had forgotten to
implement
it. https://codereview.chromium.org/799703003/ now implements the blink side of
this
and I updated this CL to depend on that.

> and also we should probably provide a
> declarative API allowing SWs to say "I expect only callers from these origins:
> A, B, C...". That's a major saving grace in Android IPC; generally, IPC
endpoint
> authors at large are not sure how to defend against malicious callers and that
> declarative way of limiting callers is the way to go.
I filed https://github.com/mkruisselbrink/navigator-connect/issues/17 for this.

[palmer, 2014-12-13 01:52:02]

LGTM. Thanks!

[Marijn Kruisselbrink, 2014-12-13 02:14:18]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-12-13 02:15:27]

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

[commit-bot: I haz the power, 2014-12-13 02:20:10]

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

[commit-bot: I haz the power, 2014-12-13 02:20:11]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Marijn Kruisselbrink, 2014-12-13 02:28:02]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-12-13 02:30:04]

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

[commit-bot: I haz the power, 2014-12-13 04:02:43]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-12-13 04:03:29]

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