Actually we safeguard against the invitation issue, so that isn't a possibility after all.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2c962cc9582a41bb34726bb424aff21be4f5083

commit c2c962cc9582a41bb34726bb424aff21be4f5083
Author: Ken Rockot <rockot@chromium.org>
Date: Mon Sep 24 23:03:14 2018

[mojo-core] PCHECK for invalid process handles

Changes a DCHECK to a PCHECK in an attempt to help sort out some crashes
we see in the wild where the broker process apparently receives handles
but doesn't attempt to duplicate them to itself. This should only be
possible if it doesn't know a valid handle for the remote (sending)
process, which points to potential application raciness in ownership of
the process handles passed into Mojo.

Bug:  887576 
Change-Id: I1f8786af8170b7db03804a98125365eeb44fc598
Reviewed-on: https://chromium-review.googlesource.com/1241197
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593733}
[modify] https://crrev.com/c2c962cc9582a41bb34726bb424aff21be4f5083/mojo/core/scoped_process_handle.cc

Hi Ken,

We modified the same check internally to be a CHECK, and it didn't change anything, we were still getting the same failures in base::win::internal::ScopedHandleVerifier::StartTracking. Sorry, I should have updated you on it earlier.

I have a test build of the Chrome Cleanup Tool running on the canary channel now, which is still getting these crashes. Since the assert is in ScopedHandleVerifier::StartTracking, the call stack where the handle was originally added to the verifier is also available (in the other.trace_ variable).

In the first minidump I looked at the handle was first added at:

base::debug::StackTrace::StackTrace+0x24
base::win::internal::ScopedHandleVerifier::StartTracking+0x85
base::win::VerifierTraits::StartTracking+0x33
base::win::GenericScopedHandle<base::win::HandleTraits,base::win::VerifierTraits>::Set+0x47
mojo::PlatformHandle::operator=+0x2b
mojo::core::PlatformHandleInTransit::operator=+0x87
mojo::core::PlatformHandleInTransit::PlatformHandleInTransit+0x30
std::vector<mojo::core::PlatformHandleInTransit,std::allocator<mojo::core::PlatformHandleInTransit> >::emplace_back<mojo::core::PlatformHandleInTransit>+0x20
mojo::core::Channel::Message::SetHandles+0x91
mojo::core::NodeChannel::AddBrokerClient+0xd1
mojo::core::NodeController::OnAcceptInvitation+0x1aa
mojo::core::NodeChannel::OnChannelMessage+0xba
mojo::core::Channel::OnReadComplete+0x209
mojo::core::`anonymous namespace'::ChannelWin::OnIOCompleted+0x21b
base::MessagePumpForIO::WaitForIOCompletion+0x1b0
base::MessagePumpForIO::DoRunLoop+0x13d
base::MessagePumpWin::Run+0x4e
base::RunLoop::Run+0x31
base::Thread::ThreadMain+0x180
base::`anonymous namespace'::ThreadFunc+0xf4

The same handle was added again (triggering the assert) at:

base::win::internal::ScopedHandleVerifier::StartTracking+0x1bf
base::win::VerifierTraits::StartTracking+0x33
base::win::GenericScopedHandle<base::win::HandleTraits,base::win::VerifierTraits>::Set+0x47
mojo::core::`anonymous namespace'::ChannelWin::GetReadPlatformHandles+0xd8
mojo::core::Channel::OnReadComplete+0x18f
mojo::core::`anonymous namespace'::ChannelWin::OnIOCompleted+0x21b
base::MessagePumpForIO::WaitForIOCompletion+0x1b0
base::MessagePumpForIO::DoRunLoop+0x8d
base::MessagePumpWin::Run+0x4e
base::RunLoop::Run+0x31
base::Thread::ThreadMain+0x180
base::`anonymous namespace'::ThreadFunc+0xf4

The handle itself:

!handle 0x31 0xF
Handle 0000000000000031
  Type         	Desktop
  Attributes   	0
  GrantedAccess	0xf01ff:
         Delete,ReadControl,WriteDac,WriteOwner
         ReadObjects,CreateWindow,CreateMenu,HookControl,JournalRecord,JournalPlayback,Enumerate,WriteObjects,SwitchDesktop
  HandleCount  	35
  PointerCount 	2198
  Name         	<none>
  No object specific information available

This is probably fixed by https://bugs.chromium.org/p/chromium/issues/detail?id=900655. I haven't been able to get a test build working to confirm it, though. Further comments in that bug, but I'll leave this open until I'm sure it's fixed the problem.

Confirmed fixed by https://bugs.chromium.org/p/chromium/issues/detail?id=900655

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c22f16c7dd504edffca98f09e29ec486afbc50a

commit 8c22f16c7dd504edffca98f09e29ec486afbc50a
Author: Joe Mason <joenotcharles@chromium.org>
Date: Mon Nov 05 16:28:49 2018

Revert "[mojo-core] PCHECK for invalid process handles"

This reverts commit c2c962cc9582a41bb34726bb424aff21be4f5083.

Reason for revert: The crash being debugged was fixed by https://chromium-review.googlesource.com/c/1310505

Original change's description:
> [mojo-core] PCHECK for invalid process handles
>
> Changes a DCHECK to a PCHECK in an attempt to help sort out some crashes
> we see in the wild where the broker process apparently receives handles
> but doesn't attempt to duplicate them to itself. This should only be
> possible if it doesn't know a valid handle for the remote (sending)
> process, which points to potential application raciness in ownership of
> the process handles passed into Mojo.
>
> Bug:  887576 
> Change-Id: I1f8786af8170b7db03804a98125365eeb44fc598
> Reviewed-on: https://chromium-review.googlesource.com/1241197
> Commit-Queue: Ken Rockot <rockot@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#593733}

R=rockot@google.com,reillyg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  887576 
Change-Id: Ic31379f9a85a20c8c47caccc185180768156eaa4
Reviewed-on: https://chromium-review.googlesource.com/c/1318069
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605351}
[modify] https://crrev.com/8c22f16c7dd504edffca98f09e29ec486afbc50a/mojo/core/scoped_process_handle.cc