Remove port lifetime management from renderer

Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

- In the browser process, ask the renderer whether a newly created opener
  port exists because the originating frame might have navigated since the
  request to create a port.

BUG=475536
Committed: https://crrev.com/d1db9f61a6000339514472fe0d69c02e9917571c
Cr-Commit-Position: refs/heads/master@{#395861}

[robwu, 2016-05-11 20:12:28]

Description was changed from

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

BUG=475536,597698
==========

to

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

BUG=475536
==========

[robwu, 2016-05-11 20:12:28]

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

[robwu, 2016-05-12 23:02:46]

Patchset #1 (id:1) has been deleted

[robwu, 2016-05-12 23:02:55]

Patchset #1 (id:20001) has been deleted

[robwu, 2016-05-12 23:03:05]

Patchset #1 (id:40001) has been deleted

[robwu, 2016-05-12 23:03:19]

Patchset #1 (id:60001) has been deleted

[robwu, 2016-05-12 23:08:28]

Description was changed from

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

BUG=475536
==========

to

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

- In the browser process, ask the renderer whether a newly created opener
  port exists because the originating frame might have navigated since the
  request to create a port.

BUG=475536
==========

[Devlin, 2016-05-17 00:14:58]

On 2016/05/12 23:08:48, robwu wrote:

Sorry for the delay on this, Rob.  I'll make sure to get to it tomorrow.

[Devlin, 2016-05-17 19:50:30]

Nice!

https://codereview.chromium.org/1966283002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1966283002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:44: // Check
whether the port exists in the frames. Frames that don't know about
nitty nit: descriptive rather than imperative function comments, so this should
be "Checks"

https://codereview.chromium.org/1966283002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:44: // Check
whether the port exists in the frames. Frames that don't know about
Specify "the frames"

https://codereview.chromium.org/1966283002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:102: // Only
set on receiver ports (if the opener was a tab).
Specify that this is -1 if invalid (pretty common, but good to be explicit)

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:96: // Close a port, optionally
forcefully (= close the whole channel instead of
nitty nit: s/=/meaning

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:99: // Arguments are (int32_t port_id,
boolean force_close).
nitty nit: don't mix language types - let's make read 'bool'

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:126: port_id, /* force_close */
false);
nit: put the comment after "false"

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:133: void ClosePort(int port_id, bool
force_close) {
function doc

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:327: void
MessagingBindings::CheckHasMessagePort(
nit: given the functionality, maybe rename this ValidateMessagePort() or
similar.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:331: int routing_id =
render_frame->GetRoutingID();
nit: inline this on line 341

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:63: if (!(this.portId_ in ports))
'in' implicitly relies on Object.prototype; use $Object.hasOwnProperty.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:86: if (!(this.portId_ in ports))
ditto

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:95: if (!(this.portId_ in ports))
ditto, but can this happen?

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:178: // TODO(robwu): This can be
changed to disconnect() because there is
What's the reason to not do this now?

[robwu, 2016-05-17 22:47:51]

https://codereview.chromium.org/1966283002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1966283002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:44: // Check
whether the port exists in the frames. Frames that don't know about
On 2016/05/17 19:50:29, Devlin wrote:
> Specify "the frames"

Done 2x.

https://codereview.chromium.org/1966283002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:102: // Only
set on receiver ports (if the opener was a tab).
On 2016/05/17 19:50:29, Devlin wrote:
> Specify that this is -1 if invalid (pretty common, but good to be explicit)

Done.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:96: // Close a port, optionally
forcefully (= close the whole channel instead of
On 2016/05/17 19:50:29, Devlin wrote:
> nitty nit: s/=/meaning

Done. (s/=/i.e./).

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:99: // Arguments are (int32_t port_id,
boolean force_close).
On 2016/05/17 19:50:30, Devlin wrote:
> nitty nit: don't mix language types - let's make read 'bool'

Done.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:126: port_id, /* force_close */
false);
On 2016/05/17 19:50:30, Devlin wrote:
> nit: put the comment after "false"

Done.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:133: void ClosePort(int port_id, bool
force_close) {
On 2016/05/17 19:50:29, Devlin wrote:
> function doc

Done.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:331: int routing_id =
render_frame->GetRoutingID();
On 2016/05/17 19:50:30, Devlin wrote:
> nit: inline this on line 341

No, this was done for a reason. I have added a comment that explains why I did
it like this.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:63: if (!(this.portId_ in ports))
On 2016/05/17 19:50:30, Devlin wrote:
> 'in' implicitly relies on Object.prototype; use $Object.hasOwnProperty.

ports has a null prototype, so it doesn't matter. But I've switched to
hasOwnProperty anyway (and changed all other "in"s for consistency.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:86: if (!(this.portId_ in ports))
On 2016/05/17 19:50:30, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:95: if (!(this.portId_ in ports))
On 2016/05/17 19:50:30, Devlin wrote:
> ditto, but can this happen?

Done. This is not expected to happen, but I didn't bother with a DCHECK/CHECK
for no specific reason.

https://codereview.chromium.org/1966283002/diff/100001/extensions/renderer/re...
extensions/renderer/resources/messaging.js:178: // TODO(robwu): This can be
changed to disconnect() because there is
On 2016/05/17 19:50:30, Devlin wrote:
> What's the reason to not do this now?

Because this would be a potential change in behavior. This CL is refactoring,
the next one is fixing the TODOs, and then finally another one to fix
crbug.com/597698.

If for some reason one of my assumptions in the TODO are incorrect, then
bisecting would point to the (small) follow-up CLs instead of this relatively
big patch.

[Devlin, 2016-05-18 17:55:09]

lgtm

https://codereview.chromium.org/1966283002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1966283002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.cc:170: new
ExtensionMsg_ValidateMessagePort(MSG_ROUTING_NONE, port_id_)));
It's probably worth duplicating the comment that valid == no response, invalid
== send close port message.

https://codereview.chromium.org/1966283002/diff/120001/extensions/renderer/me...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/1966283002/diff/120001/extensions/renderer/me...
extensions/renderer/messaging_bindings.cc:64: ExtensionImpl(ScriptContext*
context)
nit: explicit

[robwu, 2016-05-18 19:44:09]

rob@robwu.nl changed reviewers:
+ dcheng@chromium.org

[robwu, 2016-05-18 19:44:10]

+dcheng Please review/stamp extensions/common/extension_messages.h

[robwu, 2016-05-24 22:37:55]

On 2016/05/18 19:44:10, robwu wrote:
> +dcheng Please review/stamp extensions/common/extension_messages.h

Ping again?

[dcheng, 2016-05-24 23:49:15]

ipc lgtm

https://codereview.chromium.org/1966283002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1966283002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.cc:170:
SendToPort(base::WrapUnique(
base::MakeUnique<ExtensionMsg_ValidateMessagePort>(MSG_ROUTING_NONE, port_id_)

[robwu, 2016-05-25 08:58:09]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-05-25 08:58:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1966283002/#ps160001
(title: "Use MakeUnique instead of WrapUnique")

[commit-bot: I haz the power, 2016-05-25 08:58:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1966283002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1966283002/160001

[commit-bot: I haz the power, 2016-05-25 11:03:21]

Description was changed from

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

- In the browser process, ask the renderer whether a newly created opener
  port exists because the originating frame might have navigated since the
  request to create a port.

BUG=475536
==========

to

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

- In the browser process, ask the renderer whether a newly created opener
  port exists because the originating frame might have navigated since the
  request to create a port.

BUG=475536
==========

[commit-bot: I haz the power, 2016-05-25 11:03:23]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-05-25 11:04:48]

Description was changed from

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

- In the browser process, ask the renderer whether a newly created opener
  port exists because the originating frame might have navigated since the
  request to create a port.

BUG=475536
==========

to

==========
Remove port lifetime management from renderer

- Remove port tracking logic from the renderer because it is handled by
  the browser since https://codereview.chromium.org/1413543005/.

- Store the source tab id on the port in the browser to remove the need
  for tracking the tab ID in the renderer (originally introduced with
  https://codereview.chromium.org/145463002).

- Remove redundant onDestroy_ from messaging.js (because the event
  listeners are already removed as a part of the event's destroy_()).

- In the browser process, ask the renderer whether a newly created opener
  port exists because the originating frame might have navigated since the
  request to create a port.

BUG=475536

Committed: https://crrev.com/d1db9f61a6000339514472fe0d69c02e9917571c
Cr-Commit-Position: refs/heads/master@{#395861}
==========

[commit-bot: I haz the power, 2016-05-25 11:04:49]

Patchset 5 (id:??) landed as
https://crrev.com/d1db9f61a6000339514472fe0d69c02e9917571c
Cr-Commit-Position: refs/heads/master@{#395861}