Ensure that in-flight message pipes are always closed and the other end is notified.

Ensure that in-flight message pipes are always closed and the other end is notified.

This was exposed by the three tests below when the new EDK was switched on for the test target. The problem is we have the following three pairs of ports:
A <> B
C <> D
E <> F

A is sent over C and then C is sent over E. C is closed immediately after it's recieved. Before this fix, A was not recieved (i.e. TransportData::DeserializeDispatchers wasn't called for it). As a result, B never received a peer closed notification so its error handler was never called.

The fix is to ensure that when a message pipe is closed it asks its peer to close instead. That way by the time the notification is received from the peer that it's been closed we can ensure that all in-flight messages are dispatched.

BUG=561803
TEST=mojo_shell_unittests --gtest_filter=AboutFetcherTest.AboutBlank:ApplicationManagerTest.TestEndApplicationClosure:AboutFetcherTest.UnrecognizedURL

Committed: https://crrev.com/277a2fa76b238c46af6cc98eba54296b82423415
Cr-Commit-Position: refs/heads/master@{#367168}

[jam, 2015-12-30 05:52:52]

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

[sky, 2015-12-30 16:33:32]

I'm not the best reviewer of this code, but I tried.

https://codereview.chromium.org/1554623005/diff/1/mojo/edk/system/message_pip...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1554623005/diff/1/mojo/edk/system/message_pip...
mojo/edk/system/message_pipe_dispatcher.cc:969:
base::MessageLoop::current()->ReleaseSoon(FROM_HERE, this);
Is there a reason you can't Release() rather than ReleaseSoon()?

[jam, 2015-12-30 17:03:40]

https://codereview.chromium.org/1554623005/diff/1/mojo/edk/system/message_pip...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1554623005/diff/1/mojo/edk/system/message_pip...
mojo/edk/system/message_pipe_dispatcher.cc:969:
base::MessageLoop::current()->ReleaseSoon(FROM_HERE, this);
On 2015/12/30 16:33:32, sky wrote:
> Is there a reason you can't Release() rather than ReleaseSoon()? 

This might be the last reference, so I was just simplifying the code (since this
is used a few lines down). I fixed this though along with the other place I
called ReleaseSoon to avoid leaks in unit tests.

https://codereview.chromium.org/1554623005/diff/20001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1554623005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:907: Release();
(unlike the other two changes from patchset 1, which avoided leaks just in
tests, this was a real leak)

[sky, 2015-12-30 17:32:30]

https://codereview.chromium.org/1554623005/diff/20001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1554623005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:907: Release();
On 2015/12/30 17:03:40, jam wrote:
> (unlike the other two changes from patchset 1, which avoided leaks just in
> tests, this was a real leak)

If the Release() here deletes this, won't 916 result in a use after free?

[sky, 2015-12-30 17:41:20]

LGTM

[jam, 2015-12-30 17:41:27]

https://codereview.chromium.org/1554623005/diff/20001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1554623005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:907: Release();
On 2015/12/30 17:32:29, sky wrote:
> On 2015/12/30 17:03:40, jam wrote:
> > (unlike the other two changes from patchset 1, which avoided leaks just in
> > tests, this was a real leak)
> 
> If the Release() here deletes this, won't 916 result in a use after free?

yep. fixed this along with dchecks that i was hitting about locks still being
acquired on shutdown.

I had tried this locally (with release build) and it patchset 2 passed for
content_unittests. but then after the trybot errors i rebuilt with
release-with-asserts and that exposed these. Not sure why asan didn't catch the
uaf originally; it might be that in unittests' timing this Release() didn't
cause the object to be destructed.

[jam, 2015-12-30 18:12:57]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-12-30 18:13:32]

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

[commit-bot: I haz the power, 2015-12-30 19:04:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-30 19:05:49]

Description was changed from

==========
Ensure that in-flight message pipes are always closed and the other end is
notified.

This was exposed by the three tests below when the new EDK was switched on for
the test target. The problem is we have the following three pairs of ports:
A <> B
C <> D
E <> F

A is sent over C and then C is sent over E. C is closed immediately after it's
recieved. Before this fix, A was not recieved (i.e.
TransportData::DeserializeDispatchers wasn't called for it). As a result, B
never received a peer closed notification so its error handler was never called.

The fix is to ensure that when a message pipe is closed it asks its peer to
close instead. That way by the time the notification is received from the peer
that it's been closed we can ensure that all in-flight messages are dispatched.

BUG=561803
TEST=mojo_shell_unittests
--gtest_filter=AboutFetcherTest.AboutBlank:ApplicationManagerTest.TestEndApplicationClosure:AboutFetcherTest.UnrecognizedURL
==========

to

==========
Ensure that in-flight message pipes are always closed and the other end is
notified.

This was exposed by the three tests below when the new EDK was switched on for
the test target. The problem is we have the following three pairs of ports:
A <> B
C <> D
E <> F

A is sent over C and then C is sent over E. C is closed immediately after it's
recieved. Before this fix, A was not recieved (i.e.
TransportData::DeserializeDispatchers wasn't called for it). As a result, B
never received a peer closed notification so its error handler was never called.

The fix is to ensure that when a message pipe is closed it asks its peer to
close instead. That way by the time the notification is received from the peer
that it's been closed we can ensure that all in-flight messages are dispatched.

BUG=561803
TEST=mojo_shell_unittests
--gtest_filter=AboutFetcherTest.AboutBlank:ApplicationManagerTest.TestEndApplicationClosure:AboutFetcherTest.UnrecognizedURL

Committed: https://crrev.com/277a2fa76b238c46af6cc98eba54296b82423415
Cr-Commit-Position: refs/heads/master@{#367168}
==========

[commit-bot: I haz the power, 2015-12-30 19:05:50]

Patchset 3 (id:??) landed as
https://crrev.com/277a2fa76b238c46af6cc98eba54296b82423415
Cr-Commit-Position: refs/heads/master@{#367168}