HTML MessagePort as mojo::MessagePipeHandle

HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort class is added as an abstraction around a mojo::MessagePipeHandle. This provides some type safety but also encapsulates some of the details of reading and writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to pass a MessagePort around by value or to store it as a member of a Chrome IPC structure that is passed around by value. Eventually, this class will be made move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports to Java as exposed through the Android WebView and CCT APIs. This necessitated having MessagePort be more than just a ref-counted wrapper for mojo::MessagePipeHandle. The methods for reading and writing to a message port need to live in content and be usable from the browser process. Otherwise, this code could just be part of Blink. The AppWebMessagePort Java class becomes a Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++ class is machinery to accomplish this. The result is much less complexity in the Java code.

Further, to support using MessagePort directly from Java, it was necessary to cope with the serialization format of a message sent from Blink. This format is only known to Blink and V8, and the corresponding code is only designed to be used from a renderer process. Rather than IPC over to the renderer process to execute this code, this CL duplicates some of the serialization logic (just for simple string messages) in app_web_message_port.cc. This is a trade-off between overall 
complexity and code duplication. Fortunately, this serialization format is static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared worker infrastructure relied on the message port ID being a static identifier that it could use for other purposes. But now with this CL, the message port ID is gone and we just have transferable MessagePipeHandles. As a result, during setup of a shared worker another ID is needed to represent a connection that is being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and coining a Blob URL (RegisterPublicURL) are made into synchronous IPCs. This is to avoid a race condition between Blob(URL) registration and passing a Blob(URL) over a Mojo channel, which is now a different FIFO. The previous code relied on the Chrome IPC channel between the renderer and browser being a single FIFO over which Blob(URL)s are registered and MessagePort IPCs are transmitted. Using a Mojo message pipe has the consequence of creating a new FIFO, which is good for performance (the FIFO is also direct between end-points rather than routing through the browser IO thread), but it comes at the cost of introducing potential race conditions. A number of alternative designs were considered that come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL adds UMA metrics to measure the cost of these synchronous IPCs. The cost should be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2422793002
Cr-Commit-Position: refs/heads/master@{#450970}
Committed: https://chromium.googlesource.com/chromium/src/+/2d145fe56a1d995fc1a016da03c1f91f217612e0

[darin (slow to review), 2016-10-14 23:37:12]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2016-10-25 19:54:54]

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

[commit-bot: I haz the power, 2016-10-25 19:55: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, 2016-10-25 20:07:45]

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

[commit-bot: I haz the power, 2016-10-25 20:07:46]

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

[darin (slow to review), 2016-10-25 22:36:58]

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

[commit-bot: I haz the power, 2016-10-25 22:37:42]

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, 2016-10-25 22:48:36]

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

[commit-bot: I haz the power, 2016-10-25 22:48:37]

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

[darin (slow to review), 2016-11-21 00:03:43]

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

[commit-bot: I haz the power, 2016-11-21 00:03: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, 2016-11-21 00:11:22]

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

[commit-bot: I haz the power, 2016-11-21 00:11:23]

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

[darin (slow to review), 2016-12-09 01:23:39]

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

[commit-bot: I haz the power, 2016-12-09 01:24:33]

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, 2016-12-09 02:01:44]

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

[commit-bot: I haz the power, 2016-12-09 02:01:45]

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_...)

[darin (slow to review), 2016-12-09 20:30:07]

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

[commit-bot: I haz the power, 2016-12-09 20:31:11]

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, 2016-12-09 20:39:26]

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

[commit-bot: I haz the power, 2016-12-09 20:39:26]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[darin (slow to review), 2016-12-09 22:08:24]

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

[commit-bot: I haz the power, 2016-12-09 22:09:29]

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, 2016-12-09 23:03:53]

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

[commit-bot: I haz the power, 2016-12-09 23:03:54]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[darin (slow to review), 2016-12-09 23:48:50]

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

[commit-bot: I haz the power, 2016-12-09 23:49:31]

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, 2016-12-10 00:51:48]

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

[commit-bot: I haz the power, 2016-12-10 00:51:49]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[darin (slow to review), 2016-12-12 21:31:34]

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

[commit-bot: I haz the power, 2016-12-12 21:33:09]

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, 2016-12-12 23:47:53]

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

[commit-bot: I haz the power, 2016-12-12 23:47:54]

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_...)

[darin (slow to review), 2016-12-15 20:39:44]

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

[commit-bot: I haz the power, 2016-12-15 20:40:24]

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, 2016-12-15 20:43:19]

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

[commit-bot: I haz the power, 2016-12-15 20:43:21]

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...)

[darin (slow to review), 2016-12-16 23:37:36]

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

[commit-bot: I haz the power, 2016-12-16 23:38:19]

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, 2016-12-16 23:51:27]

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

[commit-bot: I haz the power, 2016-12-16 23:51:28]

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

[darin (slow to review), 2016-12-17 01:10:29]

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

[commit-bot: I haz the power, 2016-12-17 01:11:18]

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, 2016-12-17 01:23:21]

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

[commit-bot: I haz the power, 2016-12-17 01:23:22]

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

[darin (slow to review), 2016-12-30 17:44:55]

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

[commit-bot: I haz the power, 2016-12-30 17:44:59]

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, 2016-12-30 17:47:48]

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

[commit-bot: I haz the power, 2016-12-30 17:47:49]

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

[darin (slow to review), 2016-12-30 17:55:20]

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

[commit-bot: I haz the power, 2016-12-30 17:55:29]

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, 2016-12-30 18:03:29]

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

[commit-bot: I haz the power, 2016-12-30 18:03:30]

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

[darin (slow to review), 2016-12-30 20:07:28]

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

[commit-bot: I haz the power, 2016-12-30 20:07:35]

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, 2016-12-30 20:15:04]

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

[commit-bot: I haz the power, 2016-12-30 20:15:06]

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

[darin (slow to review), 2017-01-03 18:11:48]

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

[commit-bot: I haz the power, 2017-01-03 18:12:07]

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-01-03 18:21:17]

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

[commit-bot: I haz the power, 2017-01-03 18:21:19]

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

[darin (slow to review), 2017-01-05 01:07:45]

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

[commit-bot: I haz the power, 2017-01-05 01:08:19]

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-01-05 01:11:47]

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

[commit-bot: I haz the power, 2017-01-05 01:11:49]

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

[darin (slow to review), 2017-01-05 17:17:52]

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

[commit-bot: I haz the power, 2017-01-05 17:18:25]

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-01-05 17:28:57]

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

[commit-bot: I haz the power, 2017-01-05 17:28:58]

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

[darin (slow to review), 2017-01-05 23:04:42]

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

[commit-bot: I haz the power, 2017-01-05 23:05:31]

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-01-05 23:21:31]

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

[commit-bot: I haz the power, 2017-01-05 23:21:33]

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

[darin (slow to review), 2017-01-06 20:04:08]

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

[commit-bot: I haz the power, 2017-01-06 20:04: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-01-06 20:08:23]

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

[commit-bot: I haz the power, 2017-01-06 20:08:24]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[darin (slow to review), 2017-01-06 20:29:25]

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

[commit-bot: I haz the power, 2017-01-06 20:30:14]

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

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

[commit-bot: I haz the power, 2017-01-06 20:50:57]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2017-01-06 21:16:53]

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

[commit-bot: I haz the power, 2017-01-06 21:17:25]

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-01-06 21:44:49]

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

[commit-bot: I haz the power, 2017-01-06 21:44:50]

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

[darin (slow to review), 2017-01-07 04:31:52]

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

[commit-bot: I haz the power, 2017-01-07 04:32: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-01-07 06:39:56]

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

[commit-bot: I haz the power, 2017-01-07 06:39:57]

Dry run: This issue passed the CQ dry run.

[darin (slow to review), 2017-01-07 17:36:15]

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

[commit-bot: I haz the power, 2017-01-07 17:36: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-01-07 17:49:18]

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

[commit-bot: I haz the power, 2017-01-07 17:49:19]

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_...)

[darin (slow to review), 2017-01-07 19:39:20]

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

[commit-bot: I haz the power, 2017-01-07 19:39:35]

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-01-07 22:03:36]

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

[commit-bot: I haz the power, 2017-01-07 22:03:38]

Dry run: This issue passed the CQ dry run.

[darin (slow to review), 2017-01-08 01:57:53]

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

[commit-bot: I haz the power, 2017-01-08 01:57:56]

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-01-08 03:18:14]

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

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

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-01-09 17:17:37]

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

[Ken Rockot(use gerrit already), 2017-01-09 17:17:40]

Woohoo! I hope we can tolerate the sync IPCs!

https://codereview.chromium.org/2422793002/diff/510001/content/public/common/...
File content/public/common/message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/510001/content/public/common/...
content/public/common/message_port.cc:179: // And now his watch is ended.
ha!

[darin (slow to review), 2017-01-10 22:25:28]

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

[commit-bot: I haz the power, 2017-01-10 22:26:04]

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-01-11 00:18:06]

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

[commit-bot: I haz the power, 2017-01-11 00:18:08]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[darin (slow to review), 2017-01-11 20:59:18]

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

[commit-bot: I haz the power, 2017-01-11 20:59:43]

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-01-11 23:55:51]

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

[commit-bot: I haz the power, 2017-01-11 23:55:53]

Dry run: This issue passed the CQ dry run.

[darin (slow to review), 2017-01-20 01:03:46]

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

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

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-01-20 03:07:24]

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

[commit-bot: I haz the power, 2017-01-20 03:07:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[darin (slow to review), 2017-01-20 21:17:00]

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

[commit-bot: I haz the power, 2017-01-20 21:17:31]

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

[darin (slow to review), 2017-01-20 21:57:50]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it has a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be a class that lives in content and be usable from the
browser process. Otherwise, this code could just be part of Blink. The
AppWebMessagePort Java class becomes a Java wrapper for the
content::MessagePort. The content::AppWebMessagePort C++ class is machinery to
accomplish this. The result is much less complexity in the Java code.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

XXX Blob registration

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-01-20 22:02:05]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it has a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be a class that lives in content and be usable from the
browser process. Otherwise, this code could just be part of Blink. The
AppWebMessagePort Java class becomes a Java wrapper for the
content::MessagePort. The content::AppWebMessagePort C++ class is machinery to
accomplish this. The result is much less complexity in the Java code.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

XXX Blob registration

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it has a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be a class that lives in content and be usable from the
browser process. Otherwise, this code could just be part of Blink. The
AppWebMessagePort Java class becomes a Java wrapper for the
content::MessagePort. The content::AppWebMessagePort C++ class is machinery to
accomplish this. The result is much less complexity in the Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall complexity and code duplication. Fortunately, this serialization format
is static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration and "createObjectURL" (RegisterPublicURL) are made
synchronous IPCs. This is to avoid a race condition between Blob registration
and passing a Blob over a Mojo channel, which would be a different FIFO. The
previous code relied on the Chrome IPC channel between the renderer and browser
being a single FIFO overwhich both Blobs were registered and MessagePort IPCs
were transmitted. Using a Mojo message pipe has the consequence of creating a
new FIFO, which is good for performance (the FIFO is also direct between
end-points rather than routing through the browser IO thread), but it comes at
the cost of introducing potential race conditions. XXX link to alternatives
considered.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-20 23:07:25]

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

[commit-bot: I haz the power, 2017-01-20 23:07:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[darin (slow to review), 2017-01-20 23:30:17]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it has a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be a class that lives in content and be usable from the
browser process. Otherwise, this code could just be part of Blink. The
AppWebMessagePort Java class becomes a Java wrapper for the
content::MessagePort. The content::AppWebMessagePort C++ class is machinery to
accomplish this. The result is much less complexity in the Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall complexity and code duplication. Fortunately, this serialization format
is static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration and "createObjectURL" (RegisterPublicURL) are made
synchronous IPCs. This is to avoid a race condition between Blob registration
and passing a Blob over a Mojo channel, which would be a different FIFO. The
previous code relied on the Chrome IPC channel between the renderer and browser
being a single FIFO overwhich both Blobs were registered and MessagePort IPCs
were transmitted. Using a Mojo message pipe has the consequence of creating a
new FIFO, which is good for performance (the FIFO is also direct between
end-points rather than routing through the browser IO thread), but it comes at
the cost of introducing potential race conditions. XXX link to alternatives
considered.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall complexity and code duplication. Fortunately, this serialization format
is static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and "createObjectURL"
(RegisterPublicURL) are made synchronous IPCs. This is to avoid a race condition
between Blob registration and passing a Blob over a Mojo channel, which is now a
different FIFO. The previous code relied on the Chrome IPC channel between the
renderer and browser being a single FIFO over which Blobs are registered and
MessagePort IPCs are transmitted. Using a Mojo message pipe has the consequence
of creating a new FIFO, which is good for performance (the FIFO is also direct
between end-points rather than routing through the browser IO thread), but it
comes at the cost of introducing potential race conditions. A number of
alternative designs were considered that come with much greater complexity.
Instead, this CL adds UMA metrics to measure the cost of these synchronous IPCs.
The cost should be acceptably low as these IPCs terminate on the browser IO
thread.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-01-20 23:38:29]

darin@chromium.org changed reviewers:
+ esprehn@chromium.org, jam@chromium.org, jbroman@chromium.org,
sgurun@chromium.org, yusufo@chromium.org

[darin (slow to review), 2017-01-20 23:38:32]

+yusufo   - please review the Android changes as you've been in this code a lot
recently
+sgurun   - please provide OWNERS review for the //android_webview changes
+esprehn  - please provide OWNERS review for the third_party/WebKit/ changes
+jbroman  - please review the serialization code in app_web_message_port.cc
+jam      - please provide OWNERS review for //content
+rockot   - please do an overall design review

Thanks!

[darin (slow to review), 2017-01-20 23:45:57]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall complexity and code duplication. Fortunately, this serialization format
is static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and "createObjectURL"
(RegisterPublicURL) are made synchronous IPCs. This is to avoid a race condition
between Blob registration and passing a Blob over a Mojo channel, which is now a
different FIFO. The previous code relied on the Chrome IPC channel between the
renderer and browser being a single FIFO over which Blobs are registered and
MessagePort IPCs are transmitted. Using a Mojo message pipe has the consequence
of creating a new FIFO, which is good for performance (the FIFO is also direct
between end-points rather than routing through the browser IO thread), but it
comes at the cost of introducing potential race conditions. A number of
alternative designs were considered that come with much greater complexity.
Instead, this CL adds UMA metrics to measure the cost of these synchronous IPCs.
The cost should be acceptably low as these IPCs terminate on the browser IO
thread.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall complexity and code duplication. Fortunately, this serialization format
is static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and "createObjectURL"
(RegisterPublicURL) are made synchronous IPCs. This is to avoid a race condition
between Blob registration and passing a Blob over a Mojo channel, which is now a
different FIFO. The previous code relied on the Chrome IPC channel between the
renderer and browser being a single FIFO over which Blobs are registered and
MessagePort IPCs are transmitted. Using a Mojo message pipe has the consequence
of creating a new FIFO, which is good for performance (the FIFO is also direct
between end-points rather than routing through the browser IO thread), but it
comes at the cost of introducing potential race conditions. A number of
alternative designs were considered that come with much greater complexity.
Instead, this CL adds UMA metrics to measure the cost of these synchronous IPCs.
The cost should be acceptably low as these IPCs terminate on the browser IO
thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-01-23 18:01:45]

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

[jam, 2017-01-23 18:08:56]

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
content/browser/android/app_web_message_port.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
I don't remember who worked on the initial implementation of this feature on
Android. +tedchoc to review the content/browser/android and
content/public/android files as I don't feel qualified.

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
content/public/common/common_param_traits.cc:152: const param_type& p) {
nit: indentation here, 157, 173

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
File content/public/common/message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
content/public/common/message_port.cc:98: // XXX ensure num_bytes is a multiple
of 2!
is this an old comment that can be removed?

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
File content/public/common/message_port.h (right):

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
content/public/common/message_port.h:5: #ifndef CONTENT_COMMON_MESSAGE_PORT_H_
nit: CONTENT_PUBLIC

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
content/public/common/message_port.h:24: class CONTENT_EXPORT MessagePort {
I think none of these methods are used outside content, and also content/public
just has interfaces instead of implementations. can this just be an empty
interface so it can be passed in the 2 places outside of content/, and then this
implementation can move to content/common/message_port_impl?

[sgurun-gerrit only, 2017-01-23 18:27:14]

On 2017/01/23 18:08:56, jam wrote:
>
https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
> File content/browser/android/app_web_message_port.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
> content/browser/android/app_web_message_port.cc:1: // Copyright 2016 The
> Chromium Authors. All rights reserved.
> I don't remember who worked on the initial implementation of this feature on
> Android. +tedchoc to review the content/browser/android and
> content/public/android files as I don't feel qualified.


I wrote them initially for WebView, and Yusuf moved them to content. Will read
it as part of my review for aw.


> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> File content/public/common/common_param_traits.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> content/public/common/common_param_traits.cc:152: const param_type& p) {
> nit: indentation here, 157, 173
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> File content/public/common/message_port.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> content/public/common/message_port.cc:98: // XXX ensure num_bytes is a
multiple
> of 2!
> is this an old comment that can be removed?
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> File content/public/common/message_port.h (right):
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> content/public/common/message_port.h:5: #ifndef CONTENT_COMMON_MESSAGE_PORT_H_
> nit: CONTENT_PUBLIC
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> content/public/common/message_port.h:24: class CONTENT_EXPORT MessagePort {
> I think none of these methods are used outside content, and also
content/public
> just has interfaces instead of implementations. can this just be an empty
> interface so it can be passed in the 2 places outside of content/, and then
this
> implementation can move to content/common/message_port_impl?

[jam, 2017-01-23 20:42:21]

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

[jam, 2017-01-23 20:42:54]

On 2017/01/23 18:27:14, sgurun wrote:
> On 2017/01/23 18:08:56, jam wrote:
> >
>
https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
> > File content/browser/android/app_web_message_port.cc (right):
> > 
> >
>
https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
> > content/browser/android/app_web_message_port.cc:1: // Copyright 2016 The
> > Chromium Authors. All rights reserved.
> > I don't remember who worked on the initial implementation of this feature on
> > Android. +tedchoc to review the content/browser/android and
> > content/public/android files as I don't feel qualified.
> 
> 
> I wrote them initially for WebView, and Yusuf moved them to content. Will read
> it as part of my review for aw.

Great, ok I removed Ted.

[Ken Rockot(use gerrit already), 2017-01-23 23:20:42]

overall LGTM pending others' reviews

https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:301:
impl_->connect(channel);
I think we should send the WorkerConnected message first, or at least cache the
message_port_id locally first. Unless I'm misunderstanding something, connect()
appears to take ownership of |channel| and pass it to another thread, which
could in theory then free |channel| before our next (totally unsafe) access
below.

https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):

https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
content/renderer/shared_worker/embedded_shared_worker_stub.h:106:
WebMessagePortChannelImpl*>> PendingChannelList;
I think these should be unique_ptrs now, and we can explicitly release() them to
connect() and document that it takes ownership. I found the existing code quite
confusing to follow because the lifetime management of these objects is pretty
wacky.

[kinuko, 2017-01-24 12:32:52]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-01-24 12:32:55]

Looking great, let me throw a few drive-by comments...

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:58:
DCHECK(message_ports[i].GetHandle().is_valid());
I think we're leaking WebMessagePortChannel here now that we no longer have the
tricky lifetime management?

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:106: WebMessagePortChannelArray*
channels_ptr) {
This method is supposed to take channels_ptr, should we change the second param
unique_ptr?

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:107: // TODO: Address race condition
with blob registration.
Do we still have race condition now that we use sync IPC for blob registration?

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:113: ReleaseMessagePort();
This probably is leaking WebMessagePortChannel here too.

Probably WebMessagePortChannelArray should be just an array of
std::unique_ptr<WebMessagePortChannel>?

https://codereview.chromium.org/2422793002/diff/590001/content/common/fileapi...
File content/common/fileapi/webblob_messages.h (right):

https://codereview.chromium.org/2422793002/diff/590001/content/common/fileapi...
content/common/fileapi/webblob_messages.h:68: std::vector<storage::DataElement>
/* item_descriptions */)
I assume blob people are aware/ok about this change (+dmurph)

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
File content/public/common/message_port.h (right):

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
content/public/common/message_port.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: 2017 (for all new files) =)

https://codereview.chromium.org/2422793002/diff/590001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMessagePortChannel.h (right):

https://codereview.chromium.org/2422793002/diff/590001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMessagePortChannel.h:46: typedef
WebVector<class WebMessagePortChannel*> WebMessagePortChannelArray;
Should this probably be a vector of std::unique_ptr<WebMessagePortChannel>?

https://codereview.chromium.org/2422793002/diff/590001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMessagePortChannel.h:50: // call.
This comment is stale now?

[jbroman, 2017-01-24 22:41:51]

Reviewed the serialization/deserialization code only.

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
content/browser/android/app_web_message_port.cc:66: if (result_num_bytes >
buffer.size())
nit: std::basic_string::resize promises to zero-initialize characters (well,
technically initialize to CharT()), so this isn't necessary.

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
content/browser/android/app_web_message_port.cc:87: *value |=
(static_cast<uint32_t>(current_byte & kVarIntMask) << shift);
nit: shifts of >=32 bits are undefined behavior.

Especially if these messages can come from out of process, probably safer to
check for that and not invoke undefined behavior? (I did add this check to the
V8 implementation, even though the legacy Blink one doesn't have it. Not
necessarily a big deal, because I have a hard time imagining a compiler doing
anything terrible with this particular bit of UB.)

https://chromium.googlesource.com/v8/v8/+/e56437b6/src/value-serializer.cc#951

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
content/browser/android/app_web_message_port.cc:111: switch (tag) {
You also need to be able to handle the padding tag, '\0' (for legacy reasons --
I may simplify this in the future). This can occur for two-byte strings,
depending on the length of the string. If the beginning of the character array
would not be two-byte-aligned, a padding tag is added before kStringUCharTag.

For messages where the string is the entire message (as here), this will occur
if the byte length of the string takes an even number of bytes to encode (e.g.,
from 128 to 16384 bytes, or 64 to 8192 two-byte chars).

Here it's probably sufficient to essentially add this before the switch:

// Discard a leading padding tag if one is present.
if (tag == kPaddingTag && !ReadUint8Tag(&ptr, end, &tag))
  return false;

That should be sufficient to handle the current output of the serializer. A unit
test with 200 smilies or similar would catch this.

Apologies for this weird corner case. :)

[esprehn, 2017-01-24 23:57:37]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall complexity and code duplication. Fortunately, this serialization format
is static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and "createObjectURL"
(RegisterPublicURL) are made synchronous IPCs. This is to avoid a race condition
between Blob registration and passing a Blob over a Mojo channel, which is now a
different FIFO. The previous code relied on the Chrome IPC channel between the
renderer and browser being a single FIFO over which Blobs are registered and
MessagePort IPCs are transmitted. Using a Mojo message pipe has the consequence
of creating a new FIFO, which is good for performance (the FIFO is also direct
between end-points rather than routing through the browser IO thread), but it
comes at the cost of introducing potential race conditions. A number of
alternative designs were considered that come with much greater complexity.
Instead, this CL adds UMA metrics to measure the cost of these synchronous IPCs.
The cost should be acceptably low as these IPCs terminate on the browser IO
thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class 
is added as an abstraction around a mojo::MessagePipeHandle. This provides some 
type safety but also encapsulates some of the details of reading and writing to
a 
HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a 
ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to 
pass a MessagePort around by value or to store it as a member of a Chrome IPC 
structure that is passed around by value. Eventually, this class will be made 
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to 
Java as exposed through the Android WebView and CCT APIs. This necessitated
having 
MessagePort be more than just a ref-counted wrapper for mojo::MessagePipeHandle.

The methods for reading and writing to a message port need to live in content
and 
be usable from the browser process. Otherwise, this code could just be part of 
Blink. The AppWebMessagePort Java class becomes a Java wrapper for the 
content::MessagePort. The content::AppWebMessagePort C++ class is machinery to 
accomplish this. The result is much less complexity in the Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope 
with the serialization format of a message sent from Blink. This format is only 
known to Blink and V8, and the corresponding code is only designed to be used
from 
a renderer process. Rather than IPC over to the renderer process to execute this

code, this CL duplicates some of the serialization logic (just for simple string

messages) in app_web_message_port.cc. This is a trade-off between overall 
complexity and code duplication. Fortunately, this serialization format is
static 
as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared 
worker infrastructure relied on the message port ID being a static identifier
that 
it could use for other purposes. But now with this CL, the message port ID is
gone 
and we just have transferable MessagePipeHandles. As a result, during setup of a

shared worker another ID is needed to represent a connection that is being 
established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and "createObjectURL" 
(RegisterPublicURL) are made synchronous IPCs. This is to avoid a race condition

between Blob registration and passing a Blob over a Mojo channel, which is now a

different FIFO. The previous code relied on the Chrome IPC channel between the 
renderer and browser being a single FIFO over which Blobs are registered and 
MessagePort IPCs are transmitted. Using a Mojo message pipe has the consequence
of 
creating a new FIFO, which is good for performance (the FIFO is also direct 
between end-points rather than routing through the browser IO thread), but it 
comes at the cost of introducing potential race conditions. A number of 
alternative designs were considered that come with much greater complexity. 
Instead, this CL adds UMA metrics to measure the cost of these synchronous IPCs.

The cost should be acceptably low as these IPCs terminate on the browser IO 
thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dmurph, 2017-01-25 04:29:19]

dmurph@chromium.org changed reviewers:
+ dmurph@chromium.org

[dmurph, 2017-01-25 04:29:22]

blob/webblob lgtm

I think the sync message is a good solution, as that's basically what's needed -
for the metadata to be stored right away.

Thanks for adding me Kinuko!

https://codereview.chromium.org/2422793002/diff/590001/content/common/fileapi...
File content/common/fileapi/webblob_messages.h (right):

https://codereview.chromium.org/2422793002/diff/590001/content/common/fileapi...
content/common/fileapi/webblob_messages.h:68: std::vector<storage::DataElement>
/* item_descriptions */)
On 2017/01/24 12:32:55, kinuko wrote:
> I assume blob people are aware/ok about this change (+dmurph)

Thanks for adding me! Yes this sounds good. I think this is a good solution to
this problem.

[darin (slow to review), 2017-01-25 23:45:46]

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

[commit-bot: I haz the power, 2017-01-25 23:46:31]

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-01-26 01:24:02]

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

[commit-bot: I haz the power, 2017-01-26 01:24:04]

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_...)

[darin (slow to review), 2017-01-26 21:36:56]

Thanks for the feedback.

On 2017/01/23 18:08:56, jam wrote:
...
>
https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
> content/public/common/message_port.h:24: class CONTENT_EXPORT MessagePort {
> I think none of these methods are used outside content, and also
content/public
> just has interfaces instead of implementations. can this just be an empty
> interface so it can be passed in the 2 places outside of content/, and then
this
> implementation can move to content/common/message_port_impl?

Yeah, I could probably move MessagePort out of the public/ API altogether by
adding a few variations on content::MessagePortProvider::PostMessageToFrame.

Originally, I thought it could be generally useful to expose MessagePort as
part of the public/ API for future use, but given that the message encoding
is such an internal implementation detail, it is really odd for this class
to be part of the public API.

I will look into providing more specific methods on MessagePortProvider.

[darin (slow to review), 2017-01-26 22:06:30]

On 2017/01/23 23:20:42, Ken Rockot wrote:
> overall LGTM pending others' reviews
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
> File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
> content/renderer/shared_worker/embedded_shared_worker_stub.cc:301:
> impl_->connect(channel);
> I think we should send the WorkerConnected message first, or at least cache
the
> message_port_id locally first. Unless I'm misunderstanding something,
connect()
> appears to take ownership of |channel| and pass it to another thread, which
> could in theory then free |channel| before our next (totally unsafe) access
> below.

As discussed over IM, this one should no longer be an issue with this change.


>
https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
> File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):
> 
>
https://codereview.chromium.org/2422793002/diff/590001/content/renderer/share...
> content/renderer/shared_worker/embedded_shared_worker_stub.h:106:
> WebMessagePortChannelImpl*>> PendingChannelList;
> I think these should be unique_ptrs now, and we can explicitly release() them
to
> connect() and document that it takes ownership. I found the existing code
quite
> confusing to follow because the lifetime management of these objects is pretty
> wacky.

Done, good suggestion. This cleans up the code considerably.

[darin (slow to review), 2017-01-26 22:20:03]

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:58:
DCHECK(message_ports[i].GetHandle().is_valid());
On 2017/01/24 12:32:54, kinuko wrote:
> I think we're leaking WebMessagePortChannel here now that we no longer have
the
> tricky lifetime management?

Oh, wow. I didn't realize ownership of the WebMessagePortChannelImpl instances
was being passed. I should probably change the signature of this method to
better convey that.

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:106: WebMessagePortChannelArray*
channels_ptr) {
On 2017/01/24 12:32:55, kinuko wrote:
> This method is supposed to take channels_ptr, should we change the second
param
> unique_ptr?

Yes, good call

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:107: // TODO: Address race condition
with blob registration.
On 2017/01/24 12:32:55, kinuko wrote:
> Do we still have race condition now that we use sync IPC for blob
registration?

Oh, yes, I can kill this comment now. That was just meant as a reminder to me to
add the Sync IPCs or some other solution :)

https://codereview.chromium.org/2422793002/diff/590001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:113: ReleaseMessagePort();
On 2017/01/24 12:32:54, kinuko wrote:
> This probably is leaking WebMessagePortChannel here too.
> 
> Probably WebMessagePortChannelArray should be just an array of
> std::unique_ptr<WebMessagePortChannel>?

Yeah, that would probably be much better.

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
File content/public/common/message_port.h (right):

https://codereview.chromium.org/2422793002/diff/590001/content/public/common/...
content/public/common/message_port.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2017/01/24 12:32:55, kinuko wrote:
> nit: 2017 (for all new files) =)

Thanks

https://codereview.chromium.org/2422793002/diff/590001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMessagePortChannel.h (right):

https://codereview.chromium.org/2422793002/diff/590001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMessagePortChannel.h:46: typedef
WebVector<class WebMessagePortChannel*> WebMessagePortChannelArray;
On 2017/01/24 12:32:55, kinuko wrote:
> Should this probably be a vector of std::unique_ptr<WebMessagePortChannel>?

Great idea

https://codereview.chromium.org/2422793002/diff/590001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMessagePortChannel.h:50: // call.
On 2017/01/24 12:32:55, kinuko wrote:
> This comment is stale now?

Yes, thanks

[Yusuf, 2017-01-26 22:53:55]

https://codereview.chromium.org/2422793002/diff/610001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java
(right):

https://codereview.chromium.org/2422793002/diff/610001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:86:
webContents.postMessageToFrame(null, "", "", new AppWebMessagePort[]
{mChannel[1]});
Thanks for the cleanup! This looks much simpler now. You will need a rebase :)

https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
content/browser/android/app_web_message_port.cc:17: // To avoid setting up a V8
context here, we just hard code the format for
although this will be very useful reading for me, it does look scary! Maybe
someone who knows more about this serialization should also double check?

Is there a way to separate this hardcoding into its own util file?

https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
File content/browser/android/app_web_message_port_service_impl.h (left):

https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
content/browser/android/app_web_message_port_service_impl.h:59: void
OnConvertedWebToAppMessage(
does this mean we should also be cleaning up app_web_message_port_client on the
renderer side? And also the ipc_messages not used anymore?

https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:81:
private static final long UNINITIALIZED_PORT = -1;
in all JNI classes 0 in general is used as the uninitialized value. Can we also
add a _NATIVE_PTR to the end of the name?

https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:209:
public void dispatchReceivedMessages() {
javadoc

https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java
(right):

https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java:44:
return new AppWebMessagePort[] {new AppWebMessagePort(), new
AppWebMessagePort()};
does this mean, we maybe don't need this class to exist on the java side
anymore?

The native part of this is a singleton in implementation and this seems to be
the only useful java call which can be a static inside AppWebMessagePort as
well. Then we might actually need a content/public interface for the message
port on the native side (and get rid of app_web_message_port_service.h as well.)

[sgurun-gerrit only, 2017-01-26 23:24:57]

On 2017/01/26 22:53:55, Yusuf wrote:
>
https://codereview.chromium.org/2422793002/diff/610001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java
> (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:86:
> webContents.postMessageToFrame(null, "", "", new AppWebMessagePort[]
> {mChannel[1]});
> Thanks for the cleanup! This looks much simpler now. You will need a rebase :)
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> File content/browser/android/app_web_message_port.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> content/browser/android/app_web_message_port.cc:17: // To avoid setting up a
V8
> context here, we just hard code the format for
> although this will be very useful reading for me, it does look scary! Maybe
> someone who knows more about this serialization should also double check?
> 
> Is there a way to separate this hardcoding into its own util file?
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> File content/browser/android/app_web_message_port_service_impl.h (left):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> content/browser/android/app_web_message_port_service_impl.h:59: void
> OnConvertedWebToAppMessage(
> does this mean we should also be cleaning up app_web_message_port_client on
the
> renderer side? And also the ipc_messages not used anymore?
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
> (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:81:
> private static final long UNINITIALIZED_PORT = -1;
> in all JNI classes 0 in general is used as the uninitialized value. Can we
also
> add a _NATIVE_PTR to the end of the name?
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:209:
> public void dispatchReceivedMessages() {
> javadoc
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java
> (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java:44:
> return new AppWebMessagePort[] {new AppWebMessagePort(), new
> AppWebMessagePort()};
> does this mean, we maybe don't need this class to exist on the java side
> anymore?
> 
> The native part of this is a singleton in implementation and this seems to be
> the only useful java call which can be a static inside AppWebMessagePort as
> well. Then we might actually need a content/public interface for the message
> port on the native side (and get rid of app_web_message_port_service.h as
well.)

also seems like we will need an internal piece of code that depends on this too:
GoogleAuthenticatorNavigationInterceptor.java:180: error: incompatible types:
int[] cannot be converted to MessagePort[]
        mTab.getWebContents().postMessageToFrame(null, msg, mTab.getUrl(), new
int[]{});
                                                                           ^
Note: Some messages have been simplified; recompile with -Xdiags:verbose to get
full output
1 error

[sgurun-gerrit only, 2017-01-27 01:16:38]

a few early comments, will continue tomorrow/next week.

https://codereview.chromium.org/2422793002/diff/610001/content/browser/web_co...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2422793002/diff/610001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:575:
content::MessagePortProvider::PostMessageToFrame(
can we move the code in messageportprovider::postmessagetoframe to here? then we
can get rid of it entirely. after the refactorings for CCT and this, it is not
meaningful anymore IMO.

https://codereview.chromium.org/2422793002/diff/610001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:584:
->CreateMessageChannel(env, ports, web_contents_);
this will not need to get appwebmessageportservice once the implementation moves
here.

https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java
(right):

https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java:44:
return new AppWebMessagePort[] {new AppWebMessagePort(), new
AppWebMessagePort()};
On 2017/01/26 22:53:54, Yusuf wrote:
> does this mean, we maybe don't need this class to exist on the java side
> anymore?
> 
> The native part of this is a singleton in implementation and this seems to be
> the only useful java call which can be a static inside AppWebMessagePort as
> well. Then we might actually need a content/public interface for the message
> port on the native side (and get rid of app_web_message_port_service.h as
well.)

yes, that is what I noticed too. 
please get rid of this file and move createMessageChannel functionality to
WebContentsImpl.java

https://codereview.chromium.org/2422793002/diff/610001/content/public/browser...
File content/public/browser/android/app_web_message_port_service.h (right):

https://codereview.chromium.org/2422793002/diff/610001/content/public/browser...
content/public/browser/android/app_web_message_port_service.h:20: class
CONTENT_EXPORT AppWebMessagePortService {
please drop this class and move UnwrapJavaArray and CreateMessageChannel to
web_contents_android.cc

[sgurun-gerrit only, 2017-01-27 01:16:44]

a few early comments, will continue tomorrow/next week.

[darin (slow to review), 2017-01-28 00:57:55]

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

[commit-bot: I haz the power, 2017-01-28 00:58:30]

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-01-28 01:01:26]

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

[commit-bot: I haz the power, 2017-01-28 01:01:27]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jbroman, 2017-01-28 22:31:23]

Heads-up about the serialization code: I just landed a change that uses a
slightly different encoding for one-byte strings from V8. It should be
sufficient to recognize the tag, and use the same logic as for UTF-8, except
that you can just call the string16 constructor instead of base::UTF8ToUTF16.

https://codereview.chromium.org/2658793004

[darin (slow to review), 2017-01-30 21:14:24]

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

[commit-bot: I haz the power, 2017-01-30 21:15:32]

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

[darin (slow to review), 2017-01-30 21:27:42]

On 2017/01/26 22:53:55, Yusuf wrote:
...
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> File content/browser/android/app_web_message_port.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> content/browser/android/app_web_message_port.cc:17: // To avoid setting up a
V8
> context here, we just hard code the format for
> although this will be very useful reading for me, it does look scary! Maybe
> someone who knows more about this serialization should also double check?

This is why jbroman is on the review.


> Is there a way to separate this hardcoding into its own util file?

Yes, I think I will do that especially given the feedback from jbroman about
needing to support additional cases. I'll also see about adding a unit test.


>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> File content/browser/android/app_web_message_port_service_impl.h (left):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/androi...
> content/browser/android/app_web_message_port_service_impl.h:59: void
> OnConvertedWebToAppMessage(
> does this mean we should also be cleaning up app_web_message_port_client on
the
> renderer side? And also the ipc_messages not used anymore?

Oh, yes. Of course, thanks!


>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
> (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:81:
> private static final long UNINITIALIZED_PORT = -1;
> in all JNI classes 0 in general is used as the uninitialized value. Can we
also
> add a _NATIVE_PTR to the end of the name?

Will do, thanks.


> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:209:
> public void dispatchReceivedMessages() {
> javadoc
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java
> (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java:44:
> return new AppWebMessagePort[] {new AppWebMessagePort(), new
> AppWebMessagePort()};
> does this mean, we maybe don't need this class to exist on the java side
> anymore?
> 
> The native part of this is a singleton in implementation and this seems to be
> the only useful java call which can be a static inside AppWebMessagePort as
> well. Then we might actually need a content/public interface for the message
> port on the native side (and get rid of app_web_message_port_service.h as
well.)

Oh, right. I will investigate simplifications here. I was just trying to get it
to build, and then forgot to go back to clean this up. Thanks!

[commit-bot: I haz the power, 2017-01-30 22:24:23]

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

[commit-bot: I haz the power, 2017-01-30 22:24:25]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[darin (slow to review), 2017-01-30 23:26:27]

On 2017/01/28 22:31:23, jbroman wrote:
> Heads-up about the serialization code: I just landed a change that uses a
> slightly different encoding for one-byte strings from V8. It should be
> sufficient to recognize the tag, and use the same logic as for UTF-8, except
> that you can just call the string16 constructor instead of base::UTF8ToUTF16.
> 
> https://codereview.chromium.org/2658793004

Thanks for the heads up!

[ncarter (slow), 2017-01-31 20:57:43]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2017-01-31 20:57:46]

https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
File content/common/fileapi/webblob_messages.h (right):

https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
content/common/fileapi/webblob_messages.h:91: // URL before the URL is passed to
another process. This protects against a
Passing a blob URL to another renderer process generally doesn't work, so the
race described by this comment does't appear to be accurate.

To the extent it's possible to use a blob URL cross-origin, it's a security bug
-- e.g. crbug.com/647839. The relevant spec language is
https://www.w3.org/TR/FileAPI/#originOfBlobURL [Cross-origin requests on Blob
URLs must return a network error.] And, IIRC, the blink logic that enforces this
is here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...

Unlike blob URLs, blob UUIDs can definitely teleport processes, e.g., you can
postMessage a Blob object and use it in a different renderer.

For blob URLs, what does matter, as far as event sequencing goes, is for this
IPC to have arrived at the BlobDispatcherHost by the time the BlobURLRequestJob
spins up, either for a navigation or other local request. It's not 100% clear to
me whether a sync IPC is required for that. The following is a good test of the
blob-url-navigation case: FrameTreeBrowserTest.NavigateGrandchildToBlob.

[darin (slow to review), 2017-02-01 00:31:37]

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

[commit-bot: I haz the power, 2017-02-01 00:33:16]

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

[darin (slow to review), 2017-02-01 00:35:08]

On 2017/01/31 20:57:46, ncarter wrote:
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> File content/common/fileapi/webblob_messages.h (right):
> 
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> content/common/fileapi/webblob_messages.h:91: // URL before the URL is passed
to
> another process. This protects against a
> Passing a blob URL to another renderer process generally doesn't work, so the
> race described by this comment does't appear to be accurate.

I imagined the Blob URL being passed to a SharedWorker. Are you
saying that is not supported?


> 
> To the extent it's possible to use a blob URL cross-origin, it's a security
bug
> -- e.g. crbug.com/647839. The relevant spec language is
> https://www.w3.org/TR/FileAPI/#originOfBlobURL [Cross-origin requests on Blob
> URLs must return a network error.] And, IIRC, the blink logic that enforces
this
> is here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...
> 
> Unlike blob URLs, blob UUIDs can definitely teleport processes, e.g., you can
> postMessage a Blob object and use it in a different renderer.
> 
> For blob URLs, what does matter, as far as event sequencing goes, is for this
> IPC to have arrived at the BlobDispatcherHost by the time the
BlobURLRequestJob
> spins up, either for a navigation or other local request. It's not 100% clear
to
> me whether a sync IPC is required for that. The following is a good test of
the
> blob-url-navigation case: FrameTreeBrowserTest.NavigateGrandchildToBlob.

I'm happy to consider other solutions. I should probably share a braindump
of all of the discarded possible solutions. Maybe that belongs in a doc
linked to from these comments.

[commit-bot: I haz the power, 2017-02-01 00:38:12]

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

[commit-bot: I haz the power, 2017-02-01 00:38:14]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[darin (slow to review), 2017-02-01 18:52:33]

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

[commit-bot: I haz the power, 2017-02-01 18:53:17]

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-01 19:23:05]

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

[commit-bot: I haz the power, 2017-02-01 19:23:07]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[darin (slow to review), 2017-02-01 19:24:56]

On 2017/01/27 01:16:38, sgurun wrote:
> a few early comments, will continue tomorrow/next week.
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/web_co...
> File content/browser/web_contents/web_contents_android.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/web_co...
> content/browser/web_contents/web_contents_android.cc:575:
> content::MessagePortProvider::PostMessageToFrame(
> can we move the code in messageportprovider::postmessagetoframe to here? then
we
> can get rid of it entirely. after the refactorings for CCT and this, it is not
> meaningful anymore IMO.
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/browser/web_co...
> content/browser/web_contents/web_contents_android.cc:584:
> ->CreateMessageChannel(env, ports, web_contents_);
> this will not need to get appwebmessageportservice once the implementation
moves
> here.
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java
> (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePortService.java:44:
> return new AppWebMessagePort[] {new AppWebMessagePort(), new
> AppWebMessagePort()};
> On 2017/01/26 22:53:54, Yusuf wrote:
> > does this mean, we maybe don't need this class to exist on the java side
> > anymore?
> > 
> > The native part of this is a singleton in implementation and this seems to
be
> > the only useful java call which can be a static inside AppWebMessagePort as
> > well. Then we might actually need a content/public interface for the message
> > port on the native side (and get rid of app_web_message_port_service.h as
> well.)
> 
> yes, that is what I noticed too. 
> please get rid of this file and move createMessageChannel functionality to
> WebContentsImpl.java
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/browser...
> File content/public/browser/android/app_web_message_port_service.h (right):
> 
>
https://codereview.chromium.org/2422793002/diff/610001/content/public/browser...
> content/public/browser/android/app_web_message_port_service.h:20: class
> CONTENT_EXPORT AppWebMessagePortService {
> please drop this class and move UnwrapJavaArray and CreateMessageChannel to
> web_contents_android.cc

Thanks for the feedback. I significantly re-worked this code.
AppWebMessagePortService is gone, but MessagePortProvider remains.
This is because the code is needed in two places:
  AwContents::PostMessageToFrame and
  WebContentsAndroid::PostMessageToFrame

Let me know if you see a way to reduce this code further. Thanks!

[darin (slow to review), 2017-02-01 19:52:43]

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

[commit-bot: I haz the power, 2017-02-01 19:53:18]

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-01 20:21:24]

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

[commit-bot: I haz the power, 2017-02-01 20:21:26]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[darin (slow to review), 2017-02-01 23:40:19]

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

[commit-bot: I haz the power, 2017-02-01 23:40:51]

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 00:57:06]

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

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

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

[jam, 2017-02-02 01:17:16]

lgtm

[darin (slow to review), 2017-02-02 22:05:43]

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

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

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 23:28:30]

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

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

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_...)

[darin (slow to review), 2017-02-04 01:09:01]

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

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

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

[darin (slow to review), 2017-02-04 01:43:02]

@jbroman - Please take another look at the v8 / serialization related code. The
basic string codec is now not only a separate file but it is also accompanied by
a unit test. Hopefully this covers the interesting cases. Thanks!

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/590001/content/browser/androi...
content/browser/android/app_web_message_port.cc:66: if (result_num_bytes >
buffer.size())
On 2017/01/24 22:41:51, jbroman wrote:
> nit: std::basic_string::resize promises to zero-initialize characters (well,
> technically initialize to CharT()), so this isn't necessary.

Thanks

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

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

[commit-bot: I haz the power, 2017-02-04 02:49:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[kinuko, 2017-02-04 16:40:06]

On 2017/02/01 00:35:08, darin (slow to review) wrote:
> On 2017/01/31 20:57:46, ncarter wrote:
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > File content/common/fileapi/webblob_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > content/common/fileapi/webblob_messages.h:91: // URL before the URL is
passed
> to
> > another process. This protects against a
> > Passing a blob URL to another renderer process generally doesn't work, so
the
> > race described by this comment does't appear to be accurate.
> 
> I imagined the Blob URL being passed to a SharedWorker. Are you
> saying that is not supported?

I don't think this works, instead blob itself should be sent and then blob uuid
is passed internally in serialization.

> > To the extent it's possible to use a blob URL cross-origin, it's a security
> bug
> > -- e.g. crbug.com/647839. The relevant spec language is
> > https://www.w3.org/TR/FileAPI/#originOfBlobURL [Cross-origin requests on
Blob
> > URLs must return a network error.] And, IIRC, the blink logic that enforces
> this
> > is here:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...
> > 
> > Unlike blob URLs, blob UUIDs can definitely teleport processes, e.g., you
can
> > postMessage a Blob object and use it in a different renderer.
> > 
> > For blob URLs, what does matter, as far as event sequencing goes, is for
this
> > IPC to have arrived at the BlobDispatcherHost by the time the
> BlobURLRequestJob
> > spins up, either for a navigation or other local request. It's not 100%
clear
> to
> > me whether a sync IPC is required for that. The following is a good test of
> the
> > blob-url-navigation case: FrameTreeBrowserTest.NavigateGrandchildToBlob.
> 
> I'm happy to consider other solutions. I should probably share a braindump
> of all of the discarded possible solutions. Maybe that belongs in a doc
> linked to from these comments.

I believe we can just let RegisterPublicURL as is but only make RegisterBlob one
synchronous?  As nick said blob URL is for reading blob contents out via
resource loading and its consumer is the browser process (afaik).

(Separately from that linking to the braindump doc would be great)

[jbroman, 2017-02-06 15:53:21]

string_message_codec* lgtm

https://codereview.chromium.org/2422793002/diff/770001/content/browser/androi...
File content/browser/android/string_message_codec_unittest.cc (right):

https://codereview.chromium.org/2422793002/diff/770001/content/browser/androi...
content/browser/android/string_message_codec_unittest.cc:102:
message.push_back(base::char16(0x263A));
nit: equivalently,

base::string16 message(200, 0x263A);

https://codereview.chromium.org/2422793002/diff/770001/content/browser/androi...
content/browser/android/string_message_codec_unittest.cc:119: base::string16
message;
nit: equivalently,

base::string16 message(200, 0x263A);

[darin (slow to review), 2017-02-06 18:49:00]

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

[commit-bot: I haz the power, 2017-02-06 18:49:38]

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

[darin (slow to review), 2017-02-06 18:56:31]

On 2017/02/06 15:53:21, jbroman wrote:
> string_message_codec* lgtm
> 
>
https://codereview.chromium.org/2422793002/diff/770001/content/browser/androi...
> File content/browser/android/string_message_codec_unittest.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/770001/content/browser/androi...
> content/browser/android/string_message_codec_unittest.cc:102:
> message.push_back(base::char16(0x263A));
> nit: equivalently,
> 
> base::string16 message(200, 0x263A);
> 
>
https://codereview.chromium.org/2422793002/diff/770001/content/browser/androi...
> content/browser/android/string_message_codec_unittest.cc:119: base::string16
> message;
> nit: equivalently,
> 
> base::string16 message(200, 0x263A);

Good idea, thanks

[darin (slow to review), 2017-02-06 18:58:06]

On 2017/02/04 16:40:06, kinuko wrote:
> On 2017/02/01 00:35:08, darin (slow to review) wrote:
> > On 2017/01/31 20:57:46, ncarter wrote:
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > > File content/common/fileapi/webblob_messages.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > > content/common/fileapi/webblob_messages.h:91: // URL before the URL is
> passed
> > to
> > > another process. This protects against a
> > > Passing a blob URL to another renderer process generally doesn't work, so
> the
> > > race described by this comment does't appear to be accurate.
> > 
> > I imagined the Blob URL being passed to a SharedWorker. Are you
> > saying that is not supported?
> 
> I don't think this works, instead blob itself should be sent and then blob
uuid
> is passed internally in serialization.

OK, thanks for clarifying. That's good to know.


> > > To the extent it's possible to use a blob URL cross-origin, it's a
security
> > bug
> > > -- e.g. crbug.com/647839. The relevant spec language is
> > > https://www.w3.org/TR/FileAPI/#originOfBlobURL [Cross-origin requests on
> Blob
> > > URLs must return a network error.] And, IIRC, the blink logic that
enforces
> > this
> > > is here:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...
> > > 
> > > Unlike blob URLs, blob UUIDs can definitely teleport processes, e.g., you
> can
> > > postMessage a Blob object and use it in a different renderer.
> > > 
> > > For blob URLs, what does matter, as far as event sequencing goes, is for
> this
> > > IPC to have arrived at the BlobDispatcherHost by the time the
> > BlobURLRequestJob
> > > spins up, either for a navigation or other local request. It's not 100%
> clear
> > to
> > > me whether a sync IPC is required for that. The following is a good test
of
> > the
> > > blob-url-navigation case: FrameTreeBrowserTest.NavigateGrandchildToBlob.
> > 
> > I'm happy to consider other solutions. I should probably share a braindump
> > of all of the discarded possible solutions. Maybe that belongs in a doc
> > linked to from these comments.
> 
> I believe we can just let RegisterPublicURL as is but only make RegisterBlob
one
> synchronous?  As nick said blob URL is for reading blob contents out via
> resource loading and its consumer is the browser process (afaik).

Great. One less sync IPC!


> (Separately from that linking to the braindump doc would be great)

Will do.

[darin (slow to review), 2017-02-06 19:02:04]

On 2017/02/06 18:58:06, darin (slow to review) wrote:
> On 2017/02/04 16:40:06, kinuko wrote:
> > On 2017/02/01 00:35:08, darin (slow to review) wrote:
> > > On 2017/01/31 20:57:46, ncarter wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > > > File content/common/fileapi/webblob_messages.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > > > content/common/fileapi/webblob_messages.h:91: // URL before the URL is
> > passed
> > > to
> > > > another process. This protects against a
> > > > Passing a blob URL to another renderer process generally doesn't work,
so
> > the
> > > > race described by this comment does't appear to be accurate.
> > > 
> > > I imagined the Blob URL being passed to a SharedWorker. Are you
> > > saying that is not supported?
> > 
> > I don't think this works, instead blob itself should be sent and then blob
> uuid
> > is passed internally in serialization.
> 
> OK, thanks for clarifying. That's good to know.

Actually, what about cross-origin iframes? Soon those will be in
another process. Do we not allow passing a Blob URL to a cross-
origin iframe via postMessage? Perhaps I just need to test it out.

[darin (slow to review), 2017-02-06 19:08:20]

On 2017/02/06 19:02:04, darin (slow to review) wrote:
> On 2017/02/06 18:58:06, darin (slow to review) wrote:
> > On 2017/02/04 16:40:06, kinuko wrote:
> > > On 2017/02/01 00:35:08, darin (slow to review) wrote:
> > > > On 2017/01/31 20:57:46, ncarter wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > > > > File content/common/fileapi/webblob_messages.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/650001/content/common/fileapi...
> > > > > content/common/fileapi/webblob_messages.h:91: // URL before the URL is
> > > passed
> > > > to
> > > > > another process. This protects against a
> > > > > Passing a blob URL to another renderer process generally doesn't work,
> so
> > > the
> > > > > race described by this comment does't appear to be accurate.
> > > > 
> > > > I imagined the Blob URL being passed to a SharedWorker. Are you
> > > > saying that is not supported?
> > > 
> > > I don't think this works, instead blob itself should be sent and then blob
> > uuid
> > > is passed internally in serialization.
> > 
> > OK, thanks for clarifying. That's good to know.
> 
> Actually, what about cross-origin iframes? Soon those will be in
> another process. Do we not allow passing a Blob URL to a cross-
> origin iframe via postMessage? Perhaps I just need to test it out.

Nevermind. I see Nick's comment about that being a security bug.
Excellent!

[darin (slow to review), 2017-02-06 20:21:22]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class 
is added as an abstraction around a mojo::MessagePipeHandle. This provides some 
type safety but also encapsulates some of the details of reading and writing to
a 
HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a 
ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to 
pass a MessagePort around by value or to store it as a member of a Chrome IPC 
structure that is passed around by value. Eventually, this class will be made 
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to 
Java as exposed through the Android WebView and CCT APIs. This necessitated
having 
MessagePort be more than just a ref-counted wrapper for mojo::MessagePipeHandle.

The methods for reading and writing to a message port need to live in content
and 
be usable from the browser process. Otherwise, this code could just be part of 
Blink. The AppWebMessagePort Java class becomes a Java wrapper for the 
content::MessagePort. The content::AppWebMessagePort C++ class is machinery to 
accomplish this. The result is much less complexity in the Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope 
with the serialization format of a message sent from Blink. This format is only 
known to Blink and V8, and the corresponding code is only designed to be used
from 
a renderer process. Rather than IPC over to the renderer process to execute this

code, this CL duplicates some of the serialization logic (just for simple string

messages) in app_web_message_port.cc. This is a trade-off between overall 
complexity and code duplication. Fortunately, this serialization format is
static 
as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared 
worker infrastructure relied on the message port ID being a static identifier
that 
it could use for other purposes. But now with this CL, the message port ID is
gone 
and we just have transferable MessagePipeHandles. As a result, during setup of a

shared worker another ID is needed to represent a connection that is being 
established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and "createObjectURL" 
(RegisterPublicURL) are made synchronous IPCs. This is to avoid a race condition

between Blob registration and passing a Blob over a Mojo channel, which is now a

different FIFO. The previous code relied on the Chrome IPC channel between the 
renderer and browser being a single FIFO over which Blobs are registered and 
MessagePort IPCs are transmitted. Using a Mojo message pipe has the consequence
of 
creating a new FIFO, which is good for performance (the FIFO is also direct 
between end-points rather than routing through the browser IO thread), but it 
comes at the cost of introducing potential race conditions. A number of 
alternative designs were considered that come with much greater complexity. 
Instead, this CL adds UMA metrics to measure the cost of these synchronous IPCs.

The cost should be acceptably low as these IPCs terminate on the browser IO 
thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity. Instead, this CL adds UMA metrics to measure
the cost of these synchronous IPCs. The cost should be acceptably low as these
IPCs terminate on the browser IO 
thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-02-06 20:22:19]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity. Instead, this CL adds UMA metrics to measure
the cost of these synchronous IPCs. The cost should be acceptably low as these
IPCs terminate on the browser IO 
thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity. Instead, this CL adds UMA metrics to measure
the cost of these synchronous IPCs. The cost should be acceptably low as these
IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-02-06 20:35:35]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity. Instead, this CL adds UMA metrics to measure
the cost of these synchronous IPCs. The cost should be acceptably low as these
IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-02-06 20:45:27]

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

[commit-bot: I haz the power, 2017-02-06 20:46:11]

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

[darin (slow to review), 2017-02-06 20:53:40]

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

[commit-bot: I haz the power, 2017-02-06 20:54:21]

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-06 22:47:50]

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

[commit-bot: I haz the power, 2017-02-06 22:47:52]

Dry run: This issue passed the CQ dry run.

[esprehn, 2017-02-06 22:48:04]

LGTM on the public api change. I didn't get a chance look at the rest though.

[dcheng, 2017-02-07 00:01:33]

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

[dcheng, 2017-02-07 00:01:37]

Blink LGTM as well.

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/MessagePort.cpp (right):

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/MessagePort.cpp:98:
(*webChannels)[i].reset((*channels)[i].release());
Nit: does webChannels[i] = std::move((*channels)[i]) work here?

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebDOMMessageEvent.cpp (right):

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebDOMMessageEvent.cpp:86:
webChannels[i].reset((*channels)[i].release());
Nit: does webChannels[i] = std::move((*channels)[i]) work here?

[darin (slow to review), 2017-02-07 00:06:13]

Thanks Daniel

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/MessagePort.cpp (right):

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/MessagePort.cpp:98:
(*webChannels)[i].reset((*channels)[i].release());
On 2017/02/07 00:01:37, dcheng wrote:
> Nit: does webChannels[i] = std::move((*channels)[i]) work here?

Done.

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebDOMMessageEvent.cpp (right):

https://codereview.chromium.org/2422793002/diff/830001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebDOMMessageEvent.cpp:86:
webChannels[i].reset((*channels)[i].release());
On 2017/02/07 00:01:37, dcheng wrote:
> Nit: does webChannels[i] = std::move((*channels)[i]) work here?

Yes, it does. Thanks!

[darin (slow to review), 2017-02-07 00:07:40]

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

[commit-bot: I haz the power, 2017-02-07 00:08:24]

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-07 01:55:36]

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

[commit-bot: I haz the power, 2017-02-07 01:55:38]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-07 13:39:11]

https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:107: WebMessagePortChannelArray*
channels_ptr) {
I think channels_ptr itself should be given as std::unique_ptr, this method's
supposed to take the array

https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
content/child/webmessageportchannel_impl.cc:108: // TODO: Address race condition
with blob registration.
nit: let's remove this comment!

[kinuko, 2017-02-07 13:40:41]

On 2017/02/07 13:39:11, kinuko wrote:
>
https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
> File content/child/webmessageportchannel_impl.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
> content/child/webmessageportchannel_impl.cc:107: WebMessagePortChannelArray*
> channels_ptr) {
> I think channels_ptr itself should be given as std::unique_ptr, this method's

(should be -> should also be, I mean)

> supposed to take the array
> 
>
https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
> content/child/webmessageportchannel_impl.cc:108: // TODO: Address race
condition
> with blob registration.
> nit: let's remove this comment!

[Yusuf, 2017-02-07 19:12:17]

lgtm with styling nits

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:115:

javadoc?

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:119:
ports[0].nativeInitializeAppWebMessagePortPair(ports);
We can declare this static on this side as well. See my comment below in the
InitializeAppWebMessagePortPair definition

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:122:

We don't need this anymore?

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:216:
public void dispatchReceivedMessages() {
javadoc

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:249:
private native void nativeInitializeAppWebMessagePortPair(AppWebMessagePort[]
ports);
you can declare this private static native and then you don't have to do
ports[0].native... which was a bit confusing.

[darin (slow to review), 2017-02-08 06:57:05]

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

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

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

[darin (slow to review), 2017-02-08 06:59:32]

On 2017/02/07 13:40:41, kinuko wrote:
> On 2017/02/07 13:39:11, kinuko wrote:
> >
>
https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
> > File content/child/webmessageportchannel_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
> > content/child/webmessageportchannel_impl.cc:107: WebMessagePortChannelArray*
> > channels_ptr) {
> > I think channels_ptr itself should be given as std::unique_ptr, this
method's
> 
> (should be -> should also be, I mean)
> 
> > supposed to take the array

Thanks for spotting this memory leak. I believe the use of unique_ptr
here is unnecessary as WebVector now supports move semantics. So, I
started pulling on this thread and removed all of the use of unique_ptr
around {Web}MessagePortChannelArray. I believe it simplifies the code,
but at the cost of more files touched by this CL :-/

I had to extend CrossThreadCopier.h to support Vector, so please check
that I did that right.

[darin (slow to review), 2017-02-08 07:01:02]

On 2017/02/08 06:59:32, darin (slow to review) wrote:
> On 2017/02/07 13:40:41, kinuko wrote:
> > On 2017/02/07 13:39:11, kinuko wrote:
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
> > > File content/child/webmessageportchannel_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/850001/content/child/webmessa...
> > > content/child/webmessageportchannel_impl.cc:107:
WebMessagePortChannelArray*
> > > channels_ptr) {
> > > I think channels_ptr itself should be given as std::unique_ptr, this
> method's
> > 
> > (should be -> should also be, I mean)
> > 
> > > supposed to take the array
> 
> Thanks for spotting this memory leak. I believe the use of unique_ptr
> here is unnecessary as WebVector now supports move semantics. So, I
> started pulling on this thread and removed all of the use of unique_ptr
> around {Web}MessagePortChannelArray. I believe it simplifies the code,
> but at the cost of more files touched by this CL :-/

To clarify, I changed the parameter to just be a |WebMessagePortChannelArray|
rather
than |WebMessagePortChannelArray*| or
|std::unique_ptr<WebMessagePortChannelArray>|.

[darin (slow to review), 2017-02-08 07:29:27]

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:115:

On 2017/02/07 19:12:17, Yusuf wrote:
> javadoc?

Done.

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:119:
ports[0].nativeInitializeAppWebMessagePortPair(ports);
On 2017/02/07 19:12:17, Yusuf wrote:
> We can declare this static on this side as well. See my comment below in the
> InitializeAppWebMessagePortPair definition

Thanks, this worked. I had previously marked the method "native static" and that
doesn't work, but "static native" does work.

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:122:

On 2017/02/07 19:12:17, Yusuf wrote:
> We don't need this anymore?

Good catch. Removed.

https://codereview.chromium.org/2422793002/diff/850001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:216:
public void dispatchReceivedMessages() {
On 2017/02/07 19:12:17, Yusuf wrote:
> javadoc

I just changed this method to be private.

[darin (slow to review), 2017-02-08 07:31:29]

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

[commit-bot: I haz the power, 2017-02-08 07:31: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-02-08 09:05:19]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kinuko, 2017-02-08 09:45:07]

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
File content/browser/shared_worker/shared_worker_message_filter.h (right):

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
content/browser/shared_worker/shared_worker_message_filter.h:24: typedef
base::Callback<int(void)> NextRoutingIDCallback;
nit: using NextRoutingIDCallback = base::Callback<int(void)>;

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
File content/browser/shared_worker/shared_worker_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:35: typedef
std::pair<base::string16, std::vector<MessagePort>> QueuedMessage;
nit: using QueuedMessage = ...

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:134:
SharedWorkerMessageFilter::NextRoutingIDCallback callback,
nit: const ref

https://codereview.chromium.org/2422793002/diff/890001/content/renderer/share...
File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):

https://codereview.chromium.org/2422793002/diff/890001/content/renderer/share...
content/renderer/shared_worker/embedded_shared_worker_stub.h:106:
std::unique_ptr<WebMessagePortChannelImpl>> PendingChannel;
nit: using PendingChannel = std::pair<...>

https://codereview.chromium.org/2422793002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/2422793002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CrossThreadCopier.h:160: };
Hmm, I think having CrossThreadCoppier for generic Vector is dangerous, if T
needs non-trivial copier support we need to recursively call
CrossThreadCopier::copy() on each element.  Also the comment doesn't look right
as this either moves or copies the given vector depending on what the caller
passes, as Vector supports both copy and move semantics.

Could we... maybe just define CrossThreadCopier for Vector<std::unique_ptr<T>>?

[darin (slow to review), 2017-02-08 17:09:18]

Thanks kinuko!

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
File content/browser/shared_worker/shared_worker_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:35: typedef
std::pair<base::string16, std::vector<MessagePort>> QueuedMessage;
On 2017/02/08 09:45:06, kinuko wrote:
> nit: using QueuedMessage = ...

Actually, looks like I can just delete this altogether.

https://codereview.chromium.org/2422793002/diff/890001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:134:
SharedWorkerMessageFilter::NextRoutingIDCallback callback,
On 2017/02/08 09:45:06, kinuko wrote:
> nit: const ref

Done.

https://codereview.chromium.org/2422793002/diff/890001/content/renderer/share...
File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):

https://codereview.chromium.org/2422793002/diff/890001/content/renderer/share...
content/renderer/shared_worker/embedded_shared_worker_stub.h:106:
std::unique_ptr<WebMessagePortChannelImpl>> PendingChannel;
On 2017/02/08 09:45:06, kinuko wrote:
> nit: using PendingChannel = std::pair<...>

Done.

https://codereview.chromium.org/2422793002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/2422793002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CrossThreadCopier.h:160: };
On 2017/02/08 09:45:06, kinuko wrote:
> Hmm, I think having CrossThreadCoppier for generic Vector is dangerous, if T
> needs non-trivial copier support we need to recursively call
> CrossThreadCopier::copy() on each element.  Also the comment doesn't look
right
> as this either moves or copies the given vector depending on what the caller
> passes, as Vector supports both copy and move semantics.
> 
> Could we... maybe just define CrossThreadCopier for
Vector<std::unique_ptr<T>>?

Great suggestion. That works perfectly.

[darin (slow to review), 2017-02-08 17:13:13]

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

[commit-bot: I haz the power, 2017-02-08 17:13:37]

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

[darin (slow to review), 2017-02-08 17:27:17]

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

[commit-bot: I haz the power, 2017-02-08 17:27:41]

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

[darin (slow to review), 2017-02-08 19:23:33]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
TBR=asvitkine@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-02-08 19:24:57]

darin@chromium.org changed reviewers:
+ asvitkine@chromium.org

[darin (slow to review), 2017-02-08 19:25:00]

TBR=asvitkine@chromium.org for the addition to
tools/metrics/histograms/histograms.xml

[Alexei Svitkine (slow), 2017-02-08 19:29:29]

lgtm

https://codereview.chromium.org/2422793002/diff/930001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/2422793002/diff/930001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:186: base::TimeTicks
time_before = base::TimeTicks::Now();
Nit: You can make this a bit simpler by using
SCOPED_UMA_HISTOGRAM_TIMER("Storage.Blob.RegisterBlobTime");

[darin (slow to review), 2017-02-08 19:39:18]

On 2017/02/08 19:29:29, Alexei Svitkine (slow) wrote:
> lgtm
> 
>
https://codereview.chromium.org/2422793002/diff/930001/content/child/blob_sto...
> File content/child/blob_storage/blob_transport_controller.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/930001/content/child/blob_sto...
> content/child/blob_storage/blob_transport_controller.cc:186: base::TimeTicks
> time_before = base::TimeTicks::Now();
> Nit: You can make this a bit simpler by using
> SCOPED_UMA_HISTOGRAM_TIMER("Storage.Blob.RegisterBlobTime");

Sweet, thanks!

[commit-bot: I haz the power, 2017-02-08 20:03:43]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmurph, 2017-02-08 20:24:29]

I have one concern with this CL, which is more of an FYI.

Currently we have a race condition with blobs and passing them through
postMessage to other renderers / browsing contexts. The solution for this will
eventually be the mojo-ification of blobs, where each blob will be a message
pipe handle and we just clone the handle and send it to the other process.
However, currently there is a window of time between when the blob is
postMessage'd, and when it can be garbage collected in the sending context (if
it's dereferenced).

Since this CL will change message timings, it could possibly worsen this problem
if the receiving process doesn't increment the blob ref count soon enough. Or,
it could make the problem more rare if that task is executed sooner.

I definitely don't think this is a reason to rethink anything, I just wanted to
make sure this race condition is known in case we start seeing something weird.
We currently track the numbers here:
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170207&dayCount=1&h...

[darin (slow to review), 2017-02-08 20:29:41]

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

[commit-bot: I haz the power, 2017-02-08 20:30:37]

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

[darin (slow to review), 2017-02-08 20:49:43]

On 2017/02/08 20:24:29, dmurph wrote:
> I have one concern with this CL, which is more of an FYI.
> 
> Currently we have a race condition with blobs and passing them through
> postMessage to other renderers / browsing contexts. The solution for this will
> eventually be the mojo-ification of blobs, where each blob will be a message
> pipe handle and we just clone the handle and send it to the other process.
> However, currently there is a window of time between when the blob is
> postMessage'd, and when it can be garbage collected in the sending context (if
> it's dereferenced).
> 
> Since this CL will change message timings, it could possibly worsen this
problem
> if the receiving process doesn't increment the blob ref count soon enough. Or,
> it could make the problem more rare if that task is executed sooner.
> 
> I definitely don't think this is a reason to rethink anything, I just wanted
to
> make sure this race condition is known in case we start seeing something
weird.
> We currently track the numbers here:
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170207&dayCount=1&h...


Agreed. I think we just need to get busy with the conversion of Blobs away from
UUID and over to MessagePipeHandle. I think this CL is a pre-requisite to that
change. Perhaps there are other pre-requisite CLs that should land before we
can change how Blobs are represented.

[dmurph, 2017-02-08 21:08:49]

On 2017/02/08 20:49:43, darin (slow to review) wrote:
> On 2017/02/08 20:24:29, dmurph wrote:
> > I have one concern with this CL, which is more of an FYI.
> > 
> > Currently we have a race condition with blobs and passing them through
> > postMessage to other renderers / browsing contexts. The solution for this
will
> > eventually be the mojo-ification of blobs, where each blob will be a message
> > pipe handle and we just clone the handle and send it to the other process.
> > However, currently there is a window of time between when the blob is
> > postMessage'd, and when it can be garbage collected in the sending context
(if
> > it's dereferenced).
> > 
> > Since this CL will change message timings, it could possibly worsen this
> problem
> > if the receiving process doesn't increment the blob ref count soon enough.
Or,
> > it could make the problem more rare if that task is executed sooner.
> > 
> > I definitely don't think this is a reason to rethink anything, I just wanted
> to
> > make sure this race condition is known in case we start seeing something
> weird.
> > We currently track the numbers here:
> >
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170207&dayCount=1&h...
> 
> 
> Agreed. I think we just need to get busy with the conversion of Blobs away
from
> UUID and over to MessagePipeHandle. I think this CL is a pre-requisite to that
> change. Perhaps there are other pre-requisite CLs that should land before we
> can change how Blobs are represented.

Ack, sounds right to me.

[sgurun-gerrit only, 2017-02-08 21:47:50]

https://codereview.chromium.org/2422793002/diff/930001/content/browser/androi...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/930001/content/browser/androi...
content/browser/android/app_web_message_port.cc:111: env, java_ref_.get(env),
jmessage, jports);
see comment below about using weak ref.

https://codereview.chromium.org/2422793002/diff/930001/content/browser/androi...
content/browser/android/app_web_message_port.cc:135: // Called on the IO thread.
after moving to Mojo, is this still called on IO thread or some background
thread? asking before the comment on messageport.set_callback says "a background
thread".

https://codereview.chromium.org/2422793002/diff/930001/content/browser/androi...
content/browser/android/app_web_message_port.cc:137:
Java_AppWebMessagePort_onMessagesAvailable(env, java_ref_.get(env));
java_ref_ is weak. I think you need to make sure it is not null before making
the call. See an example here:

https://cs.chromium.org/chromium/src/android_webview/native/aw_contents.cc?rc...

https://codereview.chromium.org/2422793002/diff/930001/content/common/message...
File content/common/message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/930001/content/common/message...
content/common/message_port.cc:54: // MessagePorts have no way of reporting when
the peer is gone.
I wonder, if we it make sense to add this capability in future for WebView, so
that apps get to know when their peer is gone?
Just loudly thinking.

https://codereview.chromium.org/2422793002/diff/930001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/930001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:135:
private long releaseNativePortForTransfer() {
who is deleting the native message port after it is transferred? unless is
triggering closemessageport() (which I don't see), we are leaking them.

https://codereview.chromium.org/2422793002/diff/930001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:239:
nativePostMessage(mNativeAppWebMessagePort, message, ports);
I wonder if these changes create an opportunity for further optimization here
wrt to how we transfer ports. The current logic is:
postMessage -> nativePostMessage -> UnwrapJavaArray { for each port transferred:
Go back to java i.e. releaseNativePortForTransfer: set port as transferred and
uninitialize native ptr. }

Can we do:
postMessage { for each port: mark as transferred/ store native ptr/ reset native
ptr} -> nativePostMessage passing native ptrs. 

But then I guess the question is messageportprovider::postmessagetoframe can
also transfer ports. Himm, ok, nevermind.

[commit-bot: I haz the power, 2017-02-08 22:31:47]

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

[commit-bot: I haz the power, 2017-02-08 22:31:50]

Dry run: This issue passed the CQ dry run.

[darin (slow to review), 2017-02-09 00:13:17]

As discussed in person, I changed nativeDispatchReceivedMessages to
nativeDispatchNextMessage. I moved the dispatch loop to the Java side, and I
protected the nativeDispatchNextMessage w/ a null check of the native pointer.

https://codereview.chromium.org/2422793002/diff/930001/content/browser/androi...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/930001/content/browser/androi...
content/browser/android/app_web_message_port.cc:135: // Called on the IO thread.
On 2017/02/08 21:47:50, sgurun wrote:
> after moving to Mojo, is this still called on IO thread or some background
> thread? asking before the comment on messageport.set_callback says "a
background
> thread".
> 

Yeah, good catch. This code should not assume that it is the IO thread.

https://codereview.chromium.org/2422793002/diff/930001/content/browser/androi...
content/browser/android/app_web_message_port.cc:137:
Java_AppWebMessagePort_onMessagesAvailable(env, java_ref_.get(env));
On 2017/02/08 21:47:50, sgurun wrote:
> java_ref_ is weak. I think you need to make sure it is not null before making
> the call. See an example here:
> 
>
https://cs.chromium.org/chromium/src/android_webview/native/aw_contents.cc?rc...

Done.

https://codereview.chromium.org/2422793002/diff/930001/content/common/message...
File content/common/message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/930001/content/common/message...
content/common/message_port.cc:54: // MessagePorts have no way of reporting when
the peer is gone.
On 2017/02/08 21:47:50, sgurun wrote:
> I wonder, if we it make sense to add this capability in future for WebView, so
> that apps get to know when their peer is gone?
> Just loudly thinking.

Yeah, I think it could be quite valuable. Mojo has the ability to tell you when
the peer is gone. Sadly, that is not part of the HTML5 MessagePort API because
folks didn't want web developers to be able to observe GC behavior.

https://codereview.chromium.org/2422793002/diff/930001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/930001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:135:
private long releaseNativePortForTransfer() {
On 2017/02/08 21:47:50, sgurun wrote:
> who is deleting the native message port after it is transferred? unless is
> triggering closemessageport() (which I don't see), we are leaking them.

Thanks, I need to delete the native side from
AppWebMessagePort::UnwrapJavaArray. Presently, I am just leaking it. Good catch!

https://codereview.chromium.org/2422793002/diff/930001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:239:
nativePostMessage(mNativeAppWebMessagePort, message, ports);
On 2017/02/08 21:47:50, sgurun wrote:
> I wonder if these changes create an opportunity for further optimization here
> wrt to how we transfer ports. The current logic is:
> postMessage -> nativePostMessage -> UnwrapJavaArray { for each port
transferred:
> Go back to java i.e. releaseNativePortForTransfer: set port as transferred and
> uninitialize native ptr. }
> 
> Can we do:
> postMessage { for each port: mark as transferred/ store native ptr/ reset
native
> ptr} -> nativePostMessage passing native ptrs. 
> 
> But then I guess the question is messageportprovider::postmessagetoframe can
> also transfer ports. Himm, ok, nevermind.

Yeah, exactly. I had the same thoughts. I landed on this for the reason you
state.

[darin (slow to review), 2017-02-09 00:14:47]

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

[commit-bot: I haz the power, 2017-02-09 00:15:32]

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 02:07:09]

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

[commit-bot: I haz the power, 2017-02-09 02:07:12]

Dry run: This issue passed the CQ dry run.

[darin (slow to review), 2017-02-09 05:33:25]

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

[commit-bot: I haz the power, 2017-02-09 05:34: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-02-09 05:38:35]

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

[commit-bot: I haz the power, 2017-02-09 05:38:40]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[darin (slow to review), 2017-02-09 05:42:44]

Selim - I've updated AppWebMessagePort.java to address the thread-safety issue
you caught. PTAL, thanks!

[darin (slow to review), 2017-02-09 06:17:39]

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

[commit-bot: I haz the power, 2017-02-09 06:18:07]

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

[kinuko, 2017-02-09 06:21:31]

lgtm, thanks for making this change!

https://codereview.chromium.org/2422793002/diff/970001/content/browser/androi...
File content/browser/android/string_message_codec.h (right):

https://codereview.chromium.org/2422793002/diff/970001/content/browser/androi...
content/browser/android/string_message_codec.h:14: // encode and decode message
data using the same serialization format as V8.
nit: to be able encode -> to be able to encode

https://codereview.chromium.org/2422793002/diff/970001/content/child/service_...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/2422793002/diff/970001/content/child/service_...
content/child/service_worker/web_service_worker_impl.cc:122:
base::Passed(&channels)));
I think we could remove this PostTask too?  Could you try that too or have a
TODO comment for us?

[commit-bot: I haz the power, 2017-02-09 07:48:36]

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

[commit-bot: I haz the power, 2017-02-09 07:48:40]

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

[darin (slow to review), 2017-02-09 16:58:28]

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

[commit-bot: I haz the power, 2017-02-09 16:59:07]

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

[darin (slow to review), 2017-02-09 17:26:51]

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

[commit-bot: I haz the power, 2017-02-09 17:27:30]

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

[darin (slow to review), 2017-02-09 17:28:19]

On 2017/02/09 06:21:31, kinuko wrote:
> lgtm, thanks for making this change!
> 
>
https://codereview.chromium.org/2422793002/diff/970001/content/browser/androi...
> File content/browser/android/string_message_codec.h (right):
> 
>
https://codereview.chromium.org/2422793002/diff/970001/content/browser/androi...
> content/browser/android/string_message_codec.h:14: // encode and decode
message
> data using the same serialization format as V8.
> nit: to be able encode -> to be able to encode

Thanks

> 
>
https://codereview.chromium.org/2422793002/diff/970001/content/child/service_...
> File content/child/service_worker/web_service_worker_impl.cc (right):
> 
>
https://codereview.chromium.org/2422793002/diff/970001/content/child/service_...
> content/child/service_worker/web_service_worker_impl.cc:122:
> base::Passed(&channels)));
> I think we could remove this PostTask too?  Could you try that too or have a
> TODO comment for us?

Good suggestion. I made the change.

[sgurun-gerrit only, 2017-02-09 17:47:24]

https://codereview.chromium.org/2422793002/diff/1050001/content/browser/andro...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/1050001/content/browser/andro...
content/browser/android/app_web_message_port.cc:82: if
(!port_.GetMessage(&encoded_message, &ports))
It would be useful to document methods of MessagePort are thread safe I think.

https://codereview.chromium.org/2422793002/diff/1050001/content/browser/andro...
content/browser/android/app_web_message_port.cc:138:
base::android::ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
the java_ref_ is not thread fully thread safe though. See notes in
JavaObjectWeakGlobalRef.

The get() and destructor are called from different threads, one from the UI
thread in close and the other from the onmessagesavailable in a background
thread. We need to make sure they are synchronized.

[darin (slow to review), 2017-02-09 17:55:08]

https://codereview.chromium.org/2422793002/diff/1050001/content/browser/andro...
File content/browser/android/app_web_message_port.cc (right):

https://codereview.chromium.org/2422793002/diff/1050001/content/browser/andro...
content/browser/android/app_web_message_port.cc:82: if
(!port_.GetMessage(&encoded_message, &ports))
On 2017/02/09 17:47:24, sgurun wrote:
> It would be useful to document methods of MessagePort are thread safe I think.

Good idea, will do.

https://codereview.chromium.org/2422793002/diff/1050001/content/browser/andro...
content/browser/android/app_web_message_port.cc:138:
base::android::ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
On 2017/02/09 17:47:24, sgurun wrote:
> the java_ref_ is not thread fully thread safe though. See notes in
> JavaObjectWeakGlobalRef.
> 
> The get() and destructor are called from different threads, one from the UI
> thread in close and the other from the onmessagesavailable in a background
> thread. We need to make sure they are synchronized.
> 

See the comment in ~AppWebMessagePort. Explicitly resetting port_ has
the result of ensuring that OnMessagesAvailable has finished running.
~MessagePort makes this so.

[darin (slow to review), 2017-02-09 18:16:12]

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

[sgurun-gerrit only, 2017-02-09 18:16:13]

LGTM! thanks Darin.

I will prepare a downstream CL to prevent build break and will have it ready to
be rolled as soon as yours land. 

Would appreciate a ping before landing.

[darin (slow to review), 2017-02-09 18:26:31]

The CQ bit was checked by darin@chromium.org

[darin (slow to review), 2017-02-09 18:26:33]

The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org,
rockot@chromium.org, jam@chromium.org, jbroman@chromium.org,
esprehn@chromium.org, dcheng@chromium.org, yusufo@chromium.org,
asvitkine@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2422793002/#ps1070001
(title: "Add comments about threading in message_port.h")

[commit-bot: I haz the power, 2017-02-09 18:27:03]

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 20:06:12]

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

[commit-bot: I haz the power, 2017-02-09 20:06:17]

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_...)

[darin (slow to review), 2017-02-09 23:41:36]

On 2017/02/09 20:06:17, commit-bot: I haz the power wrote:
> 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_...)

Le sigh. It looks like some recent changes to record usecounters have tests that
become flaky as a result of this CL. Those tests are depending on MessagePort
communication being part of the same FIFO as Chrome IPCs. I think those tests
should be written differently, but that is another hurdle to cross before this
CL can be landed.

[Yusuf, 2017-02-10 21:43:50]

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:208:
if (!(isReady() && nativeDispatchNextMessage(mNativeAppWebMessagePort))) {
Something that just caught my eye, not excatly sure if it is problematic:

We sync this with closePort and startReceivingMessages above. So it is likely
that we we are looping here, the native pointer will be set to 0 (this can be on
a background thread and close is on UI). Should we have an early return checking
mNativeAppWebMessagePort != 0 here and before calling
nativeStartReceivingMessages above?

[sgurun-gerrit only, 2017-02-10 22:02:48]

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:170:
mStarted = true;
Another bug is that the user can call setMessageCallback after calling close. In
that case, it will crash. We need to check for if port is closed and early out.
See that we are checking if the port is closed or transferred in postMessage
call below. 

This bug seems to be an old one.

[sgurun-gerrit only, 2017-02-10 23:14:31]

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:170:
mStarted = true;
On 2017/02/10 22:02:48, sgurun wrote:
> Another bug is that the user can call setMessageCallback after calling close.
In
> that case, it will crash. We need to check for if port is closed and early
out.
> See that we are checking if the port is closed or transferred in postMessage
> call below. 
> 
> This bug seems to be an old one. 
> 
> 
> 

I mean not early out but throw.

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:170:
mStarted = true;
On 2017/02/10 22:02:48, sgurun wrote:
> Another bug is that the user can call setMessageCallback after calling close.
In
> that case, it will crash. We need to check for if port is closed and early
out.

i.e. Throw not early out.


> See that we are checking if the port is closed or transferred in postMessage
> call below. 
> 
> This bug seems to be an old one. 
> 
> 
>

[ncarter (slow), 2017-02-13 20:02:23]

https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
File content/common/fileapi/webblob_messages.h (right):

https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
content/common/fileapi/webblob_messages.h:90:
IPC_MESSAGE_CONTROL2(BlobHostMsg_RegisterPublicURL,
I was away for a bit, so just to close the loop on whether or not this needs to
be sync: the example darin provided in response to my comment (posting a
just-created blob URL to a cross-process worker) actually seemed like a
plausible race as far as I could tell, but kinuko's comment suggests that it
isn't supported. And, I am confident that the cross-process iframe/opener cases
aren't an issue, since the load-blob-url capability requires a same-origin
sender/receiver, which implies same-process.

So: lgtm on not making this message sync.

[nhiroki, 2017-02-14 04:25:40]

On 2017/02/09 23:41:36, darin (slow to review) wrote:
> On 2017/02/09 20:06:17, commit-bot: I haz the power wrote:
> > 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_...)
> 
> Le sigh. It looks like some recent changes to record usecounters have tests
that
> become flaky as a result of this CL. Those tests are depending on MessagePort
> communication being part of the same FIFO as Chrome IPCs. I think those tests
> should be written differently, but that is another hurdle to cross before this
> CL can be landed.

FYI: This should be fixed by https://codereview.chromium.org/2680423006/

[darin (slow to review), 2017-02-14 19:42:04]

The CQ bit was checked by darin@chromium.org

[darin (slow to review), 2017-02-14 19:42:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
nick@chromium.org, dmurph@chromium.org, esprehn@chromium.org, jam@chromium.org,
jbroman@chromium.org, dcheng@chromium.org, kinuko@chromium.org,
yusufo@chromium.org, asvitkine@chromium.org, sgurun@chromium.org
Link to the patchset: https://codereview.chromium.org/2422793002/#ps1090001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-14 19:42:53]

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

[darin (slow to review), 2017-02-14 19:43:38]

The CQ bit was unchecked by darin@chromium.org

[darin (slow to review), 2017-02-14 19:44:43]

On 2017/02/13 20:02:23, ncarter wrote:
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> File content/common/fileapi/webblob_messages.h (right):
> 
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> content/common/fileapi/webblob_messages.h:90:
> IPC_MESSAGE_CONTROL2(BlobHostMsg_RegisterPublicURL,
> I was away for a bit, so just to close the loop on whether or not this needs
to
> be sync: the example darin provided in response to my comment (posting a
> just-created blob URL to a cross-process worker) actually seemed like a
> plausible race as far as I could tell, but kinuko's comment suggests that it
> isn't supported. And, I am confident that the cross-process iframe/opener
cases
> aren't an issue, since the load-blob-url capability requires a same-origin
> sender/receiver, which implies same-process.
> 
> So: lgtm on not making this message sync.

Thanks for confirming

[darin (slow to review), 2017-02-14 19:50:05]

Thanks for the extra feedback!

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
(right):

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:170:
mStarted = true;
On 2017/02/10 23:14:31, sgurun wrote:
> On 2017/02/10 22:02:48, sgurun wrote:
> > Another bug is that the user can call setMessageCallback after calling
close.
> In
> > that case, it will crash. We need to check for if port is closed and early
> out.
> 
> i.e. Throw not early out.
> 
> 
> > See that we are checking if the port is closed or transferred in postMessage
> > call below. 
> > 
> > This bug seems to be an old one. 
> > 
> > 
> > 
> 

Got it. I can add such a check.

https://codereview.chromium.org/2422793002/diff/1070001/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java:208:
if (!(isReady() && nativeDispatchNextMessage(mNativeAppWebMessagePort))) {
On 2017/02/10 21:43:50, Yusuf wrote:
> Something that just caught my eye, not excatly sure if it is problematic:
> 
> We sync this with closePort and startReceivingMessages above. So it is likely
> that we we are looping here, the native pointer will be set to 0 (this can be
on
> a background thread and close is on UI). Should we have an early return
checking
> mNativeAppWebMessagePort != 0 here and before calling
> nativeStartReceivingMessages above?

isReady() checks mNativeAppWebMessagePort. I will add a similar check to
setMessageCallback, following also sgurun's advice.

[Marijn Kruisselbrink, 2017-02-14 19:51:35]

On 2017/02/14 at 19:44:43, darin wrote:
> On 2017/02/13 20:02:23, ncarter wrote:
> >
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > File content/common/fileapi/webblob_messages.h (right):
> > 
> >
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > content/common/fileapi/webblob_messages.h:90:
> > IPC_MESSAGE_CONTROL2(BlobHostMsg_RegisterPublicURL,
> > I was away for a bit, so just to close the loop on whether or not this needs
to
> > be sync: the example darin provided in response to my comment (posting a
> > just-created blob URL to a cross-process worker) actually seemed like a
> > plausible race as far as I could tell, but kinuko's comment suggests that it
> > isn't supported. And, I am confident that the cross-process iframe/opener
cases
> > aren't an issue, since the load-blob-url capability requires a same-origin
> > sender/receiver, which implies same-process.
> > 
> > So: lgtm on not making this message sync.
> 
> Thanks for confirming

Maybe all that is true in the current implementation, but that definitely does
not sound spec compliant. For example something like a window creating a blob
URL, somehow messaging that URL to a service worker, and the service worker then
fetching it should work just fine according to the spec, and that most
definitely can involve multiple processes. In practice blob URLs are a terribly
hack of course and hopefully not used in too many complicated ways, but there is
no spec level guarantee that blob URLs are only dereferenced in the process that
created them.

[darin (slow to review), 2017-02-14 19:55:00]

On 2017/02/14 19:51:35, Marijn Kruisselbrink wrote:
> On 2017/02/14 at 19:44:43, darin wrote:
> > On 2017/02/13 20:02:23, ncarter wrote:
> > >
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > > File content/common/fileapi/webblob_messages.h (right):
> > > 
> > >
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > > content/common/fileapi/webblob_messages.h:90:
> > > IPC_MESSAGE_CONTROL2(BlobHostMsg_RegisterPublicURL,
> > > I was away for a bit, so just to close the loop on whether or not this
needs
> to
> > > be sync: the example darin provided in response to my comment (posting a
> > > just-created blob URL to a cross-process worker) actually seemed like a
> > > plausible race as far as I could tell, but kinuko's comment suggests that
it
> > > isn't supported. And, I am confident that the cross-process iframe/opener
> cases
> > > aren't an issue, since the load-blob-url capability requires a same-origin
> > > sender/receiver, which implies same-process.
> > > 
> > > So: lgtm on not making this message sync.
> > 
> > Thanks for confirming
> 
> Maybe all that is true in the current implementation, but that definitely does
> not sound spec compliant. For example something like a window creating a blob
> URL, somehow messaging that URL to a service worker, and the service worker
then
> fetching it should work just fine according to the spec, and that most
> definitely can involve multiple processes. In practice blob URLs are a
terribly
> hack of course and hopefully not used in too many complicated ways, but there
is
> no spec level guarantee that blob URLs are only dereferenced in the process
that
> created them.

That's what I had thought. Sounds like I should write a test to see if it is
possible to use a Blob URL from a service worker / shared worker that was
created in a window.

[kinuko, 2017-02-14 22:35:34]

On 2017/02/14 19:55:00, darin (slow to review) wrote:
> On 2017/02/14 19:51:35, Marijn Kruisselbrink wrote:
> > On 2017/02/14 at 19:44:43, darin wrote:
> > > On 2017/02/13 20:02:23, ncarter wrote:
> > > >
> >
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > > > File content/common/fileapi/webblob_messages.h (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > > > content/common/fileapi/webblob_messages.h:90:
> > > > IPC_MESSAGE_CONTROL2(BlobHostMsg_RegisterPublicURL,
> > > > I was away for a bit, so just to close the loop on whether or not this
> needs
> > to
> > > > be sync: the example darin provided in response to my comment (posting a
> > > > just-created blob URL to a cross-process worker) actually seemed like a
> > > > plausible race as far as I could tell, but kinuko's comment suggests
that
> it
> > > > isn't supported. And, I am confident that the cross-process
iframe/opener
> > cases
> > > > aren't an issue, since the load-blob-url capability requires a
same-origin
> > > > sender/receiver, which implies same-process.
> > > > 
> > > > So: lgtm on not making this message sync.
> > > 
> > > Thanks for confirming
> > 
> > Maybe all that is true in the current implementation, but that definitely
does
> > not sound spec compliant. For example something like a window creating a
blob
> > URL, somehow messaging that URL to a service worker, and the service worker
> then
> > fetching it should work just fine according to the spec, and that most
> > definitely can involve multiple processes. In practice blob URLs are a
> terribly
> > hack of course and hopefully not used in too many complicated ways, but
there
> is
> > no spec level guarantee that blob URLs are only dereferenced in the process
> that
> > created them.
> 
> That's what I had thought. Sounds like I should write a test to see if it is
> possible to use a Blob URL from a service worker / shared worker that was
> created in a window.

Sigh... thanks for pointing out, looks like I was ignorant of some use cases
that I wanted to get rid of :(  I just wrote a test case and it works.

Also I started to feel that it's probably good to make this sync even if it's
not read by other child processes-- things could be racy in the cases where the
consumer is the browser process.

[darin (slow to review), 2017-02-15 01:11:39]

On 2017/02/14 22:35:34, kinuko wrote:
> On 2017/02/14 19:55:00, darin (slow to review) wrote:
> > On 2017/02/14 19:51:35, Marijn Kruisselbrink wrote:
> > > On 2017/02/14 at 19:44:43, darin wrote:
> > > > On 2017/02/13 20:02:23, ncarter wrote:
> > > > >
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > > > > File content/common/fileapi/webblob_messages.h (right):
> > > > > 
> > > > >
> > >
> >
>
https://codereview.chromium.org/2422793002/diff/1070001/content/common/fileap...
> > > > > content/common/fileapi/webblob_messages.h:90:
> > > > > IPC_MESSAGE_CONTROL2(BlobHostMsg_RegisterPublicURL,
> > > > > I was away for a bit, so just to close the loop on whether or not this
> > needs
> > > to
> > > > > be sync: the example darin provided in response to my comment (posting
a
> > > > > just-created blob URL to a cross-process worker) actually seemed like
a
> > > > > plausible race as far as I could tell, but kinuko's comment suggests
> that
> > it
> > > > > isn't supported. And, I am confident that the cross-process
> iframe/opener
> > > cases
> > > > > aren't an issue, since the load-blob-url capability requires a
> same-origin
> > > > > sender/receiver, which implies same-process.
> > > > > 
> > > > > So: lgtm on not making this message sync.
> > > > 
> > > > Thanks for confirming
> > > 
> > > Maybe all that is true in the current implementation, but that definitely
> does
> > > not sound spec compliant. For example something like a window creating a
> blob
> > > URL, somehow messaging that URL to a service worker, and the service
worker
> > then
> > > fetching it should work just fine according to the spec, and that most
> > > definitely can involve multiple processes. In practice blob URLs are a
> > terribly
> > > hack of course and hopefully not used in too many complicated ways, but
> there
> > is
> > > no spec level guarantee that blob URLs are only dereferenced in the
process
> > that
> > > created them.
> > 
> > That's what I had thought. Sounds like I should write a test to see if it is
> > possible to use a Blob URL from a service worker / shared worker that was
> > created in a window.
> 
> Sigh... thanks for pointing out, looks like I was ignorant of some use cases
> that I wanted to get rid of :(  I just wrote a test case and it works.
> 
> Also I started to feel that it's probably good to make this sync even if it's
> not read by other child processes-- things could be racy in the cases where
the
> consumer is the browser process.

Thanks for thinking about this further. I will go ahead and change it back
to a Sync IPC.

[darin (slow to review), 2017-02-15 01:16:24]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) is made into a synchronous IPC. This
is to avoid a race condition between Blob registration and passing a Blob over a
Mojo channel, which is now a different FIFO. The previous code relied on the
Chrome IPC channel between the renderer and browser being a single FIFO over
which Blobs are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
TBR=asvitkine@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and coining a Blob URL
(RegisterPublicURL) are made into synchronous IPCs. This is to avoid a race
condition between Blob(URL) registration and passing a Blob(URL) over a Mojo
channel, which is now a different FIFO. The previous code relied on the Chrome
IPC channel between the renderer and browser being a single FIFO over which
Blob(URL)s are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
TBR=asvitkine@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-02-15 01:22:54]

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

[commit-bot: I haz the power, 2017-02-15 01:23:56]

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-15 02:54:08]

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

[commit-bot: I haz the power, 2017-02-15 02:54:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[darin (slow to review), 2017-02-15 20:16:03]

The CQ bit was checked by darin@chromium.org

[darin (slow to review), 2017-02-15 20:16:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
nick@chromium.org, dmurph@chromium.org, esprehn@chromium.org, jam@chromium.org,
jbroman@chromium.org, dcheng@chromium.org, kinuko@chromium.org,
yusufo@chromium.org, asvitkine@chromium.org, sgurun@chromium.org
Link to the patchset: https://codereview.chromium.org/2422793002/#ps1150001
(title: "Add missing ScopedAsyncTaskScheduler instance for the new unit tests;
required by a recent change t…")

[darin (slow to review), 2017-02-15 20:16:38]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and coining a Blob URL
(RegisterPublicURL) are made into synchronous IPCs. This is to avoid a race
condition between Blob(URL) registration and passing a Blob(URL) over a Mojo
channel, which is now a different FIFO. The previous code relied on the Chrome
IPC channel between the renderer and browser being a single FIFO over which
Blob(URL)s are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
TBR=asvitkine@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and coining a Blob URL
(RegisterPublicURL) are made into synchronous IPCs. This is to avoid a race
condition between Blob(URL) registration and passing a Blob(URL) over a Mojo
channel, which is now a different FIFO. The previous code relied on the Chrome
IPC channel between the renderer and browser being a single FIFO over which
Blob(URL)s are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2017-02-15 20:16:38]

darin@chromium.org changed reviewers:
+ fdoray@chromium.org

[commit-bot: I haz the power, 2017-02-15 20:17:50]

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-15 21:32:15]

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

[darin (slow to review), 2017-02-16 07:44:41]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2017-02-16 07:45:49]

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-16 10:59:53]

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

[commit-bot: I haz the power, 2017-02-16 10:59:59]

Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[darin (slow to review), 2017-02-16 14:22:40]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2017-02-16 14:23:13]

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-16 15:22:41]

CQ is committing da patch.

Bot data: {"patchset_id": 1150001, "attempt_start_ts": 1487254960692470,
"parent_rev": "1ee86ba6f52bb583d9d11b4e39b49fe4f4af32d5", "commit_rev":
"2d145fe56a1d995fc1a016da03c1f91f217612e0"}

[commit-bot: I haz the power, 2017-02-16 15:23:28]

Description was changed from

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and coining a Blob URL
(RegisterPublicURL) are made into synchronous IPCs. This is to avoid a race
condition between Blob(URL) registration and passing a Blob(URL) over a Mojo
channel, which is now a different FIFO. The previous code relied on the Chrome
IPC channel between the renderer and browser being a single FIFO over which
Blob(URL)s are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
HTML MessagePort as mojo::MessagePipeHandle

This is mostly a reduction in code and complexity. The content::MessagePort
class is added as an abstraction around a mojo::MessagePipeHandle. This provides
some type safety but also encapsulates some of the details of reading and
writing to a HTML MessagePort.

The MessagePort class also eases the transition from Chrome IPC to Mojo by being
a ref-counted wrapper for the mojo::MessagePipeHandle. This makes it possible to
pass a MessagePort around by value or to store it as a member of a Chrome IPC
structure that is passed around by value. Eventually, this class will be made
move-only.

A good bit of complexity in this CL has to do with bridging HTML message ports
to Java as exposed through the Android WebView and CCT APIs. This necessitated
having MessagePort be more than just a ref-counted wrapper for
mojo::MessagePipeHandle. The methods for reading and writing to a message port
need to live in content and be usable from the browser process. Otherwise, this
code could just be part of Blink. The AppWebMessagePort Java class becomes a
Java wrapper for the content::MessagePort. The content::AppWebMessagePort C++
class is machinery to accomplish this. The result is much less complexity in the
Java code.

Further, to support using MessagePort directly from Java, it was necessary to
cope with the serialization format of a message sent from Blink. This format is
only known to Blink and V8, and the corresponding code is only designed to be
used from a renderer process. Rather than IPC over to the renderer process to
execute this code, this CL duplicates some of the serialization logic (just for
simple string messages) in app_web_message_port.cc. This is a trade-off between
overall 
complexity and code duplication. Fortunately, this serialization format is
static as it is a format we currently persist to disk via IndexedDB.

There is also some complexity related to shared workers. Previously, the shared
worker infrastructure relied on the message port ID being a static identifier
that it could use for other purposes. But now with this CL, the message port ID
is gone and we just have transferable MessagePipeHandles. As a result, during
setup of a shared worker another ID is needed to represent a connection that is
being established. This is what the connection_request_id is for.

Finally, Blob registration (RegisterBlob) and coining a Blob URL
(RegisterPublicURL) are made into synchronous IPCs. This is to avoid a race
condition between Blob(URL) registration and passing a Blob(URL) over a Mojo
channel, which is now a different FIFO. The previous code relied on the Chrome
IPC channel between the renderer and browser being a single FIFO over which
Blob(URL)s are registered and MessagePort IPCs are transmitted. Using a Mojo
message pipe has the consequence of creating a new FIFO, which is good for
performance (the FIFO is also direct between end-points rather than routing
through the browser IO thread), but it comes at the cost of introducing
potential race conditions. A number of alternative designs were considered that
come with much greater complexity (see https://goo.gl/bfdE64). Instead, this CL
adds UMA metrics to measure the cost of these synchronous IPCs. The cost should
be acceptably low as these IPCs terminate on the browser IO thread.

BUG=361001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2017-02-16 15:23:34]

Committed patchset #59 (id:1150001) as
https://chromium.googlesource.com/chromium/src/+/2d145fe56a1d995fc1a016da03c1...