Implement the close() API for Message ports

Implement the close() API for Message ports

The close API has to consider these cases:
1. A message port can be in a pending state
2. A message could be queued in renderer waiting for conversion

Therefore close() immediately closes the port, but it does not
cleanup the resources until all the messages are transferred.

BUG=393291
Committed: https://crrev.com/1f47db6eeea87febb755db2c498335fd9bb226b1
Cr-Commit-Position: refs/heads/master@{#318271}

[sgurun-gerrit only, 2015-02-25 02:00:04]

sgurun@chromium.org changed reviewers:
+ hush@chromium.org

[sgurun-gerrit only, 2015-02-25 02:00:13]

ptal thanks

[sgurun-gerrit only, 2015-02-25 02:32:17]

sgurun@chromium.org changed reviewers:
+ nasko@chromium.org

[sgurun-gerrit only, 2015-02-25 02:33:07]

On 2015/02/25 02:00:13, sgurun wrote:
> ptal thanks

nasko: please review
android_webview/common
and 
content/

thanks

[mnaganov (inactive), 2015-02-25 10:36:26]

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

[mnaganov (inactive), 2015-02-25 10:36:27]

Great job! I have some drive-by comments.

https://codereview.chromium.org/956763002/diff/1/android_webview/common/aw_me...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/956763002/diff/1/android_webview/common/aw_me...
android_webview/common/aw_message_port_messages.h:60: bool /* whether to close
the message port */)
As for the port closing message there is no actual data that needs to be
converted, wouldn't it be more logical to avoid using this message at all, and
instead have a special dedicated one? This should also make cleaner code that
sends and code that handles the close command. Probably, a comment will be
needed that we need to use message in order to synchronize with in-flight
conversion messages.

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:130:
+ "        var received = \"\";"
nit: You can use single quotes to avoid escaping for better readability.

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:132:
+ "            received = received + e.data;"
nit: use +=

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:156:
assertTrue("Received title " + mAwContents.getTitle() + " while expecting " +
title,
You should put titles in quotes to make the result more comprehensible if title
is empty.

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:164:
return mAwContents.getTitle().equals(title);
nit: Should be "title.equals(mAwContents.getTitle())" to avoid crashing if
"null" is returned.

[nasko, 2015-02-25 15:03:08]

android_webview/common and content/ LGTM

[hush (inactive), 2015-02-25 19:44:28]

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(left):

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:131:
removeSentPorts(sentPorts);
why is this removed?

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:127:
mPortStorage.put(messagePortId, null);
what's different between this and mPortStorage.remove(messagePortId)?

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:68:
return mMessageQueue.size() > 0;
shouldn't this be == 0? I will be surprised if the tests pass with this...

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:92:
if (mMessageQueue.size() > 0 || !mDelegate.isPostMessageSenderReady()) {
use isMessageQueueEmpty() here since you have created such a method

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:363:
// Verify a transferred port using postmessagetoframe cannot be closed.
MessagePort.postMessage instead of postmessagetoframe

[sgurun-gerrit only, 2015-02-25 20:06:38]

mnaganov: good point on ipc to clean the code. done.
nasko: changed the ipc as mnaganov suggested (FYI since you already lgtm'ed)

https://codereview.chromium.org/956763002/diff/1/android_webview/common/aw_me...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/956763002/diff/1/android_webview/common/aw_me...
android_webview/common/aw_message_port_messages.h:60: bool /* whether to close
the message port */)
I think a special IPC would also work and the code will be cleaner. 
On 2015/02/25 10:36:26, mnaganov (cr) wrote:
> As for the port closing message there is no actual data that needs to be
> converted, wouldn't it be more logical to avoid using this message at all, and
> instead have a special dedicated one? This should also make cleaner code that
> sends and code that handles the close command. Probably, a comment will be
> needed that we need to use message in order to synchronize with in-flight
> conversion messages.

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:130:
+ "        var received = \"\";"
On 2015/02/25 10:36:27, mnaganov (cr) wrote:
> nit: You can use single quotes to avoid escaping for better readability.

Done.

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:132:
+ "            received = received + e.data;"
On 2015/02/25 10:36:27, mnaganov (cr) wrote:
> nit: use +=

Done.

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:156:
assertTrue("Received title " + mAwContents.getTitle() + " while expecting " +
title,
On 2015/02/25 10:36:26, mnaganov (cr) wrote:
> You should put titles in quotes to make the result more comprehensible if
title
> is empty.

Done.

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:164:
return mAwContents.getTitle().equals(title);
On 2015/02/25 10:36:26, mnaganov (cr) wrote:
> nit: Should be "title.equals(mAwContents.getTitle())" to avoid crashing if
> "null" is returned.

Done.

[nasko, 2015-02-25 21:03:57]

On 2015/02/25 20:06:38, sgurun wrote:
> mnaganov: good point on ipc to clean the code. done.
> nasko: changed the ipc as mnaganov suggested (FYI since you already lgtm'ed)

I'll wait for the final LGTM on other reviewers and then look again at IPC, to
wait out other potential changes.

[mnaganov (inactive), 2015-02-25 21:08:32]

Thanks! LGTM

https://codereview.chromium.org/956763002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_message_port_message_filter.cc (right):

https://codereview.chromium.org/956763002/diff/20001/android_webview/browser/...
android_webview/browser/aw_message_port_message_filter.cc:48: 
nit: remove empty line

[sgurun-gerrit only, 2015-02-25 21:38:54]

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(left):

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:131:
removeSentPorts(sentPorts);
On 2015/02/25 19:44:27, hush wrote:
> why is this removed?

this is actually a bug. The ports are removed in PostMessageSender (see
removeSentPorts there)

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:127:
mPortStorage.put(messagePortId, null);
On 2015/02/25 19:44:27, hush wrote:
> what's different between this and mPortStorage.remove(messagePortId)?

good catch.

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:68:
return mMessageQueue.size() > 0;
On 2015/02/25 19:44:27, hush wrote:
> shouldn't this be == 0? I will be surprised if the tests pass with this...

Done.

https://codereview.chromium.org/956763002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:92:
if (mMessageQueue.size() > 0 || !mDelegate.isPostMessageSenderReady()) {
On 2015/02/25 19:44:27, hush wrote:
> use isMessageQueueEmpty() here since you have created such a method

I think I can read this a lot faster :) the method is for public access

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/956763002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:363:
// Verify a transferred port using postmessagetoframe cannot be closed.
On 2015/02/25 19:44:27, hush wrote:
> MessagePort.postMessage instead of postmessagetoframe

Done.

[hush (inactive), 2015-02-25 22:03:19]

lgtm

[sgurun-gerrit only, 2015-02-25 22:10:47]

On 2015/02/25 22:03:19, hush wrote:
> lgtm

ok waiting for Nasko's re-lgtm. thanks all.

[nasko, 2015-02-26 17:45:20]

LGTM after the comment is addressed.

https://codereview.chromium.org/956763002/diff/40001/android_webview/common/a...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/956763002/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:83:
IPC_MESSAGE_ROUTED1(AwMessagePortHostMsg_ClosePort,
Since this is a response to a message sent from the browser, it is an ack. Let's
add this at the end of the message name - AwMessagePortHostMsg_ClosePortAck.
This is a pattern used in other IPC messages and makes it immediately clear what
the message intent is without looking up its definition.

[sgurun-gerrit only, 2015-02-26 18:00:51]

https://codereview.chromium.org/956763002/diff/40001/android_webview/common/a...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/956763002/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:83:
IPC_MESSAGE_ROUTED1(AwMessagePortHostMsg_ClosePort,
On 2015/02/26 17:45:20, nasko wrote:
> Since this is a response to a message sent from the browser, it is an ack.
Let's
> add this at the end of the message name - AwMessagePortHostMsg_ClosePortAck.
> This is a pattern used in other IPC messages and makes it immediately clear
what
> the message intent is without looking up its definition.

good point. done.

[sgurun-gerrit only, 2015-02-26 18:01:09]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2015-02-26 18:01:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org, hush@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/956763002/#ps60001 (title:
"address Nasko review")

[commit-bot: I haz the power, 2015-02-26 18:01:49]

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

[commit-bot: I haz the power, 2015-02-26 18:51:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-26 18:53:13]

Patchset 4 (id:??) landed as
https://crrev.com/1f47db6eeea87febb755db2c498335fd9bb226b1
Cr-Commit-Position: refs/heads/master@{#318271}