Fix shutdown assert with the new Mojo EDK.

Fix shutdown assert with the new Mojo EDK.

This was seen on Windows tryjobs enabling the new EDK. The problem was that RoutedRawChannel was destroying the channel on write errors, while normally (without multiplexing) this is only done on read errors. So MessagePipeDispatcher thought it had a channel_ (which it didn't use because it knew it had seen a write error) and it was asserting at shutdown. The fix is to make RoutedRawChannel match non-multiplexed MessagePipeDispatcher's destruction of the channel only when read errors occur.

BUG=561803
Committed: https://crrev.com/a0c8ebc44e574bf12e7ffca0d492b9859f914a4d
Cr-Commit-Position: refs/heads/master@{#365175}

[jam, 2015-12-14 23:05:49]

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

[yzshen1, 2015-12-14 23:43:34]

lgtm

[jam, 2015-12-15 00:51:48]

Description was changed from

==========
Fix a race condition with the new Mojo EDK.

This was seen on Windows tryjobs enabling the new EDK. The first is that the
PostTask to shutdown the channel may not run if the IO thread is already gone.
In that case, we can safely null out channel_ so that the assert in the
destructor doesn't fire.

The other small fix is to ensure we only call EnsureLazyInitialized for the
transferable channel case.

BUG=561803
==========

to

==========
Fix shutdown assert with the new Mojo EDK.

This was seen on Windows tryjobs enabling the new EDK. The problem was that
RoutedRawChannel was destroying the channel on write errors, while normally
(without multiplexing) this is only done on read errors. So
MessagePipeDispatcher thought it had a channel_ (which it didn't use because it
knew it had seen a write error) and it was asserting at shutdown. The fix is to
make RoutedRawChannel match non-multiplexed MessagePipeDispatcher's destruction
of the channel only when read errors occur.

BUG=561803
==========

[jam, 2015-12-15 00:52:45]

ptal, here's the real fix as discussed. Thanks for asking more questions :)

[jam, 2015-12-15 00:55:18]

The CQ bit was checked by jam@chromium.org

[jam, 2015-12-15 00:55:18]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1526563004/#ps40001
(title: "real fix")

[commit-bot: I haz the power, 2015-12-15 00:56:14]

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

[commit-bot: I haz the power, 2015-12-15 02:51:20]

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

[commit-bot: I haz the power, 2015-12-15 02:51:21]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[yzshen1, 2015-12-15 03:06:01]

LGTM ++  Thanks!

https://codereview.chromium.org/1526563004/diff/40001/mojo/edk/system/routed_...
File mojo/edk/system/routed_raw_channel.cc (right):

https://codereview.chromium.org/1526563004/diff/40001/mojo/edk/system/routed_...
mojo/edk/system/routed_raw_channel.cc:135: if (error != ERROR_WRITE) {
It seems nice to add comment for it.

[jam, 2015-12-15 04:22:15]

The CQ bit was checked by jam@chromium.org

[jam, 2015-12-15 04:22:31]

https://codereview.chromium.org/1526563004/diff/40001/mojo/edk/system/routed_...
File mojo/edk/system/routed_raw_channel.cc (right):

https://codereview.chromium.org/1526563004/diff/40001/mojo/edk/system/routed_...
mojo/edk/system/routed_raw_channel.cc:135: if (error != ERROR_WRITE) {
On 2015/12/15 03:06:01, yzshen1 wrote:
> It seems nice to add comment for it.

Sure will do in a followup

[jam, 2015-12-15 04:27:09]

The CQ bit was checked by jam@chromium.org

[jam, 2015-12-15 04:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1526563004/#ps60001
(title: "add comment")

[commit-bot: I haz the power, 2015-12-15 04:28:50]

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

[commit-bot: I haz the power, 2015-12-15 06:18:21]

Description was changed from

==========
Fix shutdown assert with the new Mojo EDK.

This was seen on Windows tryjobs enabling the new EDK. The problem was that
RoutedRawChannel was destroying the channel on write errors, while normally
(without multiplexing) this is only done on read errors. So
MessagePipeDispatcher thought it had a channel_ (which it didn't use because it
knew it had seen a write error) and it was asserting at shutdown. The fix is to
make RoutedRawChannel match non-multiplexed MessagePipeDispatcher's destruction
of the channel only when read errors occur.

BUG=561803
==========

to

==========
Fix shutdown assert with the new Mojo EDK.

This was seen on Windows tryjobs enabling the new EDK. The problem was that
RoutedRawChannel was destroying the channel on write errors, while normally
(without multiplexing) this is only done on read errors. So
MessagePipeDispatcher thought it had a channel_ (which it didn't use because it
knew it had seen a write error) and it was asserting at shutdown. The fix is to
make RoutedRawChannel match non-multiplexed MessagePipeDispatcher's destruction
of the channel only when read errors occur.

BUG=561803
==========

[commit-bot: I haz the power, 2015-12-15 06:18:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-15 06:18:59]

Description was changed from

==========
Fix shutdown assert with the new Mojo EDK.

This was seen on Windows tryjobs enabling the new EDK. The problem was that
RoutedRawChannel was destroying the channel on write errors, while normally
(without multiplexing) this is only done on read errors. So
MessagePipeDispatcher thought it had a channel_ (which it didn't use because it
knew it had seen a write error) and it was asserting at shutdown. The fix is to
make RoutedRawChannel match non-multiplexed MessagePipeDispatcher's destruction
of the channel only when read errors occur.

BUG=561803
==========

to

==========
Fix shutdown assert with the new Mojo EDK.

This was seen on Windows tryjobs enabling the new EDK. The problem was that
RoutedRawChannel was destroying the channel on write errors, while normally
(without multiplexing) this is only done on read errors. So
MessagePipeDispatcher thought it had a channel_ (which it didn't use because it
knew it had seen a write error) and it was asserting at shutdown. The fix is to
make RoutedRawChannel match non-multiplexed MessagePipeDispatcher's destruction
of the channel only when read errors occur.

BUG=561803

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

[commit-bot: I haz the power, 2015-12-15 06:19:00]

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