[Extensions] Extension Port Ids and Initialization 2.0

[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous (crbug.com/642380), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  (crbug.com/660706); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  (crbug.com/649942).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer, crbug.com/597698). Test added courtesy of
  rob@robwu.nl.
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG=649942
BUG=597698
Committed: https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092
Cr-Commit-Position: refs/heads/master@{#436655}

[Devlin, 2016-12-02 00:45:16]

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

[commit-bot: I haz the power, 2016-12-02 00:45:50]

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

[Devlin, 2016-12-02 00:52:14]

Description was changed from

==========
[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous (crbug.com/642380), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  (crbug.com/660706); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  (crbug.com/649942).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer, crbug.com/597698).
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG=649942
BUG=597698
==========

to

==========
[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous (crbug.com/642380), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  (crbug.com/660706); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  (crbug.com/649942).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer, crbug.com/597698). Test added courtesy of
  rob@robwu.nl.
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG=649942
BUG=597698
==========

[Devlin, 2016-12-02 02:00:01]

rdevlin.cronin@chromium.org changed reviewers:
+ dcheng@chromium.org, lazyboy@chromium.org, rob@robwu.nl

[Devlin, 2016-12-02 02:00:02]

Hey folks, please take a look.  Sorry for the size.

Rob, I believe this fixes the issue from
https://codereview.chromium.org/2529213002/.  Please also review because I think
you're as knowledgeable about extension messaging as most of us. :)

Daniel, please review IPCs, and also the changes in webkit (removing the
temporary helper methods).

Istiaque, as the lucky extensions owner, please review everything (sorry!).

[commit-bot: I haz the power, 2016-12-02 03:50:34]

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

[commit-bot: I haz the power, 2016-12-02 03:50:35]

Dry run: This issue passed the CQ dry run.

[robwu, 2016-12-02 11:58:54]

lgtm with nits.

Incidentally, (script) context ID + local port ID is also exactly how messages
are routed/filtered in Firefox's implementation of the extension messaging API.
Their context ID is a local counter concatenated with a process-unique ID, but a
GUID as you've done here suffices as well. Hopefully the sun doesn't explode ;)

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.cc:76:
!source_port_id.is_opener))
Can GET_CHANNEL_ID and GET_OPPOSITE_PORT_ID be (inline) functions instead of
macros?

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.h:187: using ChannelId =
std::pair<std::string, int>;
A comment that explains what the string and int means wouldn't hurt.

https://codereview.chromium.org/2547753002/diff/20001/extensions/common/api/m...
File extensions/common/api/messaging/port_id.h (right):

https://codereview.chromium.org/2547753002/diff/20001/extensions/common/api/m...
extensions/common/api/messaging/port_id.h:33: // - this means that multiple
contexts could have the same id. However, GUIDs
Nit: "-" is superfluous.

https://codereview.chromium.org/2547753002/diff/20001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (left):

https://codereview.chromium.org/2547753002/diff/20001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:83: return;  // The channel was opened
by this same context; ignore it.
I'd extend this comment with the remark that this only happens when messages are
sent to a process. When messages are directed to individual frames, this
wouldn't happen, as they are filtered out in the browser (by
ExtensionMessagePort::RemoveCommonFrames).

[Devlin, 2016-12-02 17:16:25]

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

[Devlin, 2016-12-02 17:16:32]

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.cc:76:
!source_port_id.is_opener))
On 2016/12/02 11:58:54, robwu wrote:
> Can GET_CHANNEL_ID and GET_OPPOSITE_PORT_ID be (inline) functions instead of
> macros?

I was leaning the same way; sure.

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/2547753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.h:187: using ChannelId =
std::pair<std::string, int>;
On 2016/12/02 11:58:54, robwu wrote:
> A comment that explains what the string and int means wouldn't hurt.

Done.

https://codereview.chromium.org/2547753002/diff/20001/extensions/common/api/m...
File extensions/common/api/messaging/port_id.h (right):

https://codereview.chromium.org/2547753002/diff/20001/extensions/common/api/m...
extensions/common/api/messaging/port_id.h:33: // - this means that multiple
contexts could have the same id. However, GUIDs
On 2016/12/02 11:58:54, robwu wrote:
> Nit: "-" is superfluous.

Whoops!  Didn't mean to have that one there.  Thanks.

https://codereview.chromium.org/2547753002/diff/20001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (left):

https://codereview.chromium.org/2547753002/diff/20001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:83: return;  // The channel was opened
by this same context; ignore it.
On 2016/12/02 11:58:54, robwu wrote:
> I'd extend this comment with the remark that this only happens when messages
are
> sent to a process. When messages are directed to individual frames, this
> wouldn't happen, as they are filtered out in the browser (by
> ExtensionMessagePort::RemoveCommonFrames).

Done.

[commit-bot: I haz the power, 2016-12-02 17:16:49]

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-02 17:20:11]

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

[commit-bot: I haz the power, 2016-12-02 17:20:11]

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

[dcheng, 2016-12-02 19:32:04]

Yay!

https://codereview.chromium.org/2547753002/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.h (right):

https://codereview.chromium.org/2547753002/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:131: const std::string context_id_;
How would you feel about using base::UnguessableToken for this? It's a bit
better typed than a GUID.

[Devlin, 2016-12-02 20:25:25]

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

[Devlin, 2016-12-02 20:25:49]

rdevlin.cronin@chromium.org changed reviewers:
+ sky@chromium.org

[Devlin, 2016-12-02 20:25:50]

+sky@ as well for chrome/renderer/chrome_mock_render_thread.cc (purely code
deletion, yay!)

https://codereview.chromium.org/2547753002/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.h (right):

https://codereview.chromium.org/2547753002/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:131: const std::string context_id_;
On 2016/12/02 19:32:04, dcheng wrote:
> How would you feel about using base::UnguessableToken for this? It's a bit
> better typed than a GUID.

Sure!  That was my original thought, but I had concerns around uniqueness. 
Discussed offline, and agreed that the sun is still more likely to explode, so
we're okay. :)

[commit-bot: I haz the power, 2016-12-02 20:25:59]

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

[sky, 2016-12-02 21:18:46]

LGTM

[commit-bot: I haz the power, 2016-12-03 00:33:55]

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

[commit-bot: I haz the power, 2016-12-03 00:33:56]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-03 00:50:56]

https://codereview.chromium.org/2547753002/diff/60001/base/unguessable_token_...
File base/unguessable_token_unittest.cc (right):

https://codereview.chromium.org/2547753002/diff/60001/base/unguessable_token_...
base/unguessable_token_unittest.cc:123:
ASSERT_TRUE(tokens.insert(UnguessableToken::Create()).second);
How long does this take to run? Honestly, this feels kind of like overkill to me
=)

https://codereview.chromium.org/2547753002/diff/60001/base/unguessable_token_...
base/unguessable_token_unittest.cc:124: }
Nit: newline after this

[Devlin, 2016-12-03 01:46:08]

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

[Devlin, 2016-12-03 01:46:12]

https://codereview.chromium.org/2547753002/diff/60001/base/unguessable_token_...
File base/unguessable_token_unittest.cc (right):

https://codereview.chromium.org/2547753002/diff/60001/base/unguessable_token_...
base/unguessable_token_unittest.cc:123:
ASSERT_TRUE(tokens.insert(UnguessableToken::Create()).second);
On 2016/12/03 00:50:56, dcheng wrote:
> How long does this take to run? Honestly, this feels kind of like overkill to
me
> =)

Whoops!  This wasn't actually supposed to be included - it was just for my own
sanity check.  Ran this with --gtest_repeat=20.

Removed.

[commit-bot: I haz the power, 2016-12-03 01:46:26]

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

[dcheng, 2016-12-03 03:19:39]

ipc+blink lgtm

[commit-bot: I haz the power, 2016-12-03 03:48:13]

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

[commit-bot: I haz the power, 2016-12-03 03:48:15]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-12-03 03:53:27]

https://codereview.chromium.org/2547753002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2547753002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:12: #include
"base/guid.h"
Revert.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_port.h (right):

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_port.h:21: // handles sending related IPCs to the
browser. Since global port IDs are
Remove comment about global port id and add a note about |js_id|

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:86: return;  // The channel was opened
by this same context; ignore it.
nit: This comment seems extraneous now give the comment in #82 ^^^

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:465:
"Extensions.Messaging.GetPortIdSyncTime.Extension");
*SyncTime* for these UMA is misleading now given we don't have sync/async times,
maybe rename these entirely now? GetPortIdTime? Or keep them calling Async.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.h (right):

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:66: // Returns an existing port with
the given |global_id|, or null.
s/global_id/id

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:69: // Creates a new port with the
given |global_id|. MessagingBindings owns the
Same as ^^^

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:122: size_t
ports_created_in_before_unload_ = 0;
These seems to be unused atm, remove these and their histogram code?

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:131: // A unique identifier for this
context.
nit: context/ScriptContext to be specific

[Devlin, 2016-12-05 17:07:43]

https://codereview.chromium.org/2547753002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2547753002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:12: #include
"base/guid.h"
On 2016/12/03 03:53:26, lazyboy wrote:
> Revert.

Done.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_port.h (right):

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_port.h:21: // handles sending related IPCs to the
browser. Since global port IDs are
On 2016/12/03 03:53:26, lazyboy wrote:
> Remove comment about global port id and add a note about |js_id|

Done.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:86: return;  // The channel was opened
by this same context; ignore it.
On 2016/12/03 03:53:26, lazyboy wrote:
> nit: This comment seems extraneous now give the comment in #82 ^^^

Removed.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:465:
"Extensions.Messaging.GetPortIdSyncTime.Extension");
On 2016/12/03 03:53:26, lazyboy wrote:
> *SyncTime* for these UMA is misleading now given we don't have sync/async
times,
> maybe rename these entirely now? GetPortIdTime? Or keep them calling Async.

Hmm... good point.  For comparison purposes, I'd like to keep recording this -
I'm curious how it stacks up against the previous sync/async methods (it should
certainly be faster, but how much?).  I've added a new histogram to track it so
that it's clear it's different.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.h (right):

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:66: // Returns an existing port with
the given |global_id|, or null.
On 2016/12/03 03:53:27, lazyboy wrote:
> s/global_id/id

Done.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:69: // Creates a new port with the
given |global_id|. MessagingBindings owns the
On 2016/12/03 03:53:27, lazyboy wrote:
> Same as ^^^

Done.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:122: size_t
ports_created_in_before_unload_ = 0;
On 2016/12/03 03:53:26, lazyboy wrote:
> These seems to be unused atm, remove these and their histogram code?

Good catch.  I'm still interested in this data overall, but we now we can only
really track total.  Reworked.

https://codereview.chromium.org/2547753002/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:131: // A unique identifier for this
context.
On 2016/12/03 03:53:26, lazyboy wrote:
> nit: context/ScriptContext to be specific

I'd prefer JS context, since this is a per-JS context object and that's really
what we're identifying.  Otherwise the id should really go directly on the
ScriptContext (which we may do at some point).

[Devlin, 2016-12-05 17:09:00]

rdevlin.cronin@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Devlin, 2016-12-05 17:09:01]

+rkaplow for metrics

https://codereview.chromium.org/2547753002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2547753002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:108600: label="Created during an event
handler for the 'unload' event."/>
rkaplow: the above suffixes are no longer being recorded, but the new one is. 
Can we mark suffixes as obsolete?  Or should we just make a new metric, even
though it's conceptually similar?  Or something else?

[lazyboy, 2016-12-05 20:39:03]

lgtm

[rkaplow, 2016-12-05 23:01:15]

lgtm

https://codereview.chromium.org/2547753002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2547753002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:108600: label="Created during an event
handler for the 'unload' event."/>
On 2016/12/05 17:09:01, Devlin wrote:
> rkaplow: the above suffixes are no longer being recorded, but the new one is. 
> Can we mark suffixes as obsolete?  Or should we just make a new metric, even
> though it's conceptually similar?  Or something else?

yes, you can add obsolete tags to a suffix. See ModuleIntegrityVerificationType
for an example.

[Devlin, 2016-12-06 16:37:54]

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

[commit-bot: I haz the power, 2016-12-06 16:38:15]

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

[Devlin, 2016-12-06 17:53:11]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2016-12-06 17:53:19]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-12-06 17:53:19]

The patchset sent to the CQ was uploaded after l-g-t-m from rob@robwu.nl,
sky@chromium.org, dcheng@chromium.org, lazyboy@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2547753002/#ps120001
(title: "rkaplow")

[commit-bot: I haz the power, 2016-12-06 17:53: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, 2016-12-06 18:40:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481046799540940,
"parent_rev": "34e8f71148c90b662b9c7cea42ccf39dc4a8cdb7", "commit_rev":
"95c80bb3d2131eaabec8a2a20a654259caea3b5a"}

[commit-bot: I haz the power, 2016-12-06 18:41:00]

Description was changed from

==========
[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous (crbug.com/642380), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  (crbug.com/660706); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  (crbug.com/649942).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer, crbug.com/597698). Test added courtesy of
  rob@robwu.nl.
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG=649942
BUG=597698
==========

to

==========
[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous (crbug.com/642380), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  (crbug.com/660706); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  (crbug.com/649942).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer, crbug.com/597698). Test added courtesy of
  rob@robwu.nl.
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG=649942
BUG=597698
==========

[commit-bot: I haz the power, 2016-12-06 18:41:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-06 18:44:14]

Description was changed from

==========
[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous (crbug.com/642380), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  (crbug.com/660706); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  (crbug.com/649942).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer, crbug.com/597698). Test added courtesy of
  rob@robwu.nl.
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG=649942
BUG=597698
==========

to

==========
[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous (crbug.com/642380), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  (crbug.com/660706); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  (crbug.com/649942).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer, crbug.com/597698). Test added courtesy of
  rob@robwu.nl.
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG=649942
BUG=597698

Committed: https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092
Cr-Commit-Position: refs/heads/master@{#436655}
==========

[commit-bot: I haz the power, 2016-12-06 18:44:15]

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

[Devlin, 2016-12-08 16:09:44]

Whoops, forgot to update.  (Comment was addressed in the patchset that landed.)

https://codereview.chromium.org/2547753002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2547753002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:108600: label="Created during an event
handler for the 'unload' event."/>
On 2016/12/05 23:01:15, rkaplow wrote:
> On 2016/12/05 17:09:01, Devlin wrote:
> > rkaplow: the above suffixes are no longer being recorded, but the new one
is. 
> > Can we mark suffixes as obsolete?  Or should we just make a new metric, even
> > though it's conceptually similar?  Or something else?
> 
> yes, you can add obsolete tags to a suffix. See
ModuleIntegrityVerificationType
> for an example.
> 
> 

Thanks!  Done.