Bug 1828389 - Ensure IPC channel is closed with error after KillHard, r=ipc-reviewers,mccr8

This patch changes KillHard() such that the IPC channel is immediately
shut down with an error after a KillHard() is performed. This is done by
fixing the previously-broken CLOSE_CHANNEL_WITH_ERROR support in
ShutDownProcess, and calling that method after KillHard().

This ensures that after the process has been killed, no further messages
will be delivered and processed, even if they were sent before the
process was killed.

In addition, the assertions and KillHard calls which are disabled for
fuzzing were changed to also shut down the channel, making fuzzing IPC
errors cause the connection to be terminated like it is in production
for these actors.

This change does not impact actors which ignore processing errors.

Differential Revision: https://phabricator.services.mozilla.com/D178383

dom/ipc/ContentParent.cpp

@@ -1879,17 +1879,18 @@ bool ContentParent::ShutDownProcess(ShutDownMethod aMethod) {
     qms->AbortOperationsForProcess(mChildID);
   }
 
-  // If Close() fails with an error, we'll end up back in this function, but
-  // with aMethod = CLOSE_CHANNEL_WITH_ERROR.
-
-  if (aMethod == CLOSE_CHANNEL) {
+  if (aMethod == CLOSE_CHANNEL || aMethod == CLOSE_CHANNEL_WITH_ERROR) {
     if (!mCalledClose) {
       MaybeLogBlockShutdownDiagnostics(
           this, "ShutDownProcess: Closing channel.", __FILE__, __LINE__);
-      // Close() can only be called once: It kicks off the destruction
-      // sequence.
+      // Close()/CloseWithError() can only be called once: They kick off the
+      // destruction sequence.
       mCalledClose = true;
-      Close();
+      if (aMethod == CLOSE_CHANNEL_WITH_ERROR) {
+        CloseWithError();
+      } else {
+        Close();
+      }
     }
     result = true;
   }
@@ -2074,10 +2075,11 @@ void ContentParent::ProcessingError(Result aCode, const char* aReason) {
   if (MsgDropped == aCode) {
     return;
   }
-#ifndef FUZZING
   // Other errors are big deals.
+#ifndef FUZZING
   KillHard(aReason);
 #endif
+  ShutDownProcess(CLOSE_CHANNEL_WITH_ERROR);
 }
 
 void ContentParent::ActorDestroy(ActorDestroyReason why) {
@@ -4515,6 +4517,7 @@ void ContentParent::KillHard(const char* aReason) {
   ProcessHandle otherProcessHandle;
   if (!base::OpenProcessHandle(OtherPid(), &otherProcessHandle)) {
     NS_ERROR("Failed to open child process when attempting kill.");
+    ShutDownProcess(CLOSE_CHANNEL_WITH_ERROR);
     return;
   }
 
@@ -4535,6 +4538,10 @@ void ContentParent::KillHard(const char* aReason) {
     mSubprocess->SetAlreadyDead();
   }
 
+  // After we've killed the remote process, also ensure we close the IPC channel
+  // with an error to immediately stop all IPC communication on this channel.
+  ShutDownProcess(CLOSE_CHANNEL_WITH_ERROR);
+
   // EnsureProcessTerminated has responsibilty for closing otherProcessHandle.
   XRE_GetIOMessageLoop()->PostTask(
       NewRunnableFunction("EnsureProcessTerminatedRunnable",

dom/media/ipc/RemoteDecoderManagerChild.cpp

@@ -859,7 +859,7 @@ void RemoteDecoderManagerChild::DeallocateSurfaceDescriptor(
       })));
 }
 
-void RemoteDecoderManagerChild::HandleFatalError(const char* aMsg) const {
+void RemoteDecoderManagerChild::HandleFatalError(const char* aMsg) {
   dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid());
 }
 

dom/media/ipc/RemoteDecoderManagerChild.h

@@ -107,7 +107,7 @@ class RemoteDecoderManagerChild final
       RemoteDecodeIn aLocation);
 
  protected:
-  void HandleFatalError(const char* aMsg) const override;
+  void HandleFatalError(const char* aMsg) override;
 
   PRemoteDecoderChild* AllocPRemoteDecoderChild(
       const RemoteDecoderInfoIPDL& aRemoteDecoderInfo,

gfx/ipc/VsyncBridgeChild.cpp

@@ -119,7 +119,7 @@ void VsyncBridgeChild::ProcessingError(Result aCode, const char* aReason) {
                      "Processing error in VsyncBridgeChild");
 }
 
-void VsyncBridgeChild::HandleFatalError(const char* aMsg) const {
+void VsyncBridgeChild::HandleFatalError(const char* aMsg) {
   dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid());
 }
 

gfx/ipc/VsyncBridgeChild.h

@@ -31,7 +31,7 @@ class VsyncBridgeChild final : public PVsyncBridgeChild {
 
   void NotifyVsync(const VsyncEvent& aVsync, const layers::LayersId& aLayersId);
 
-  void HandleFatalError(const char* aMsg) const override;
+  void HandleFatalError(const char* aMsg) override;
 
  private:
   VsyncBridgeChild(RefPtr<VsyncIOThreadHolder>, const uint64_t& aProcessToken);

gfx/layers/ipc/CompositorManagerChild.cpp

@@ -211,7 +211,7 @@ void CompositorManagerChild::ActorDestroy(ActorDestroyReason aReason) {
   }
 }
 
-void CompositorManagerChild::HandleFatalError(const char* aMsg) const {
+void CompositorManagerChild::HandleFatalError(const char* aMsg) {
   dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid());
 }
 

gfx/layers/ipc/CompositorManagerChild.h

@@ -83,7 +83,7 @@ class CompositorManagerChild : public PCompositorManagerChild {
 
   void ActorDestroy(ActorDestroyReason aReason) override;
 
-  void HandleFatalError(const char* aMsg) const override;
+  void HandleFatalError(const char* aMsg) override;
 
   void ProcessingError(Result aCode, const char* aReason) override;
 

gfx/layers/ipc/ImageBridgeChild.cpp

@@ -920,7 +920,7 @@ bool ImageBridgeChild::CanSend() const {
   return mCanSend;
 }
 
-void ImageBridgeChild::HandleFatalError(const char* aMsg) const {
+void ImageBridgeChild::HandleFatalError(const char* aMsg) {
   dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid());
 }
 

gfx/layers/ipc/ImageBridgeChild.h

@@ -312,7 +312,7 @@ class ImageBridgeChild final : public PImageBridgeChild,
 
   bool InForwarderThread() override { return InImageBridgeChildThread(); }
 
-  void HandleFatalError(const char* aMsg) const override;
+  void HandleFatalError(const char* aMsg) override;
 
   wr::MaybeExternalImageId GetNextExternalImageId() override;
 

gfx/layers/ipc/UiCompositorControllerChild.cpp

@@ -210,7 +210,7 @@ void UiCompositorControllerChild::ProcessingError(Result aCode,
   }
 }
 
-void UiCompositorControllerChild::HandleFatalError(const char* aMsg) const {
+void UiCompositorControllerChild::HandleFatalError(const char* aMsg) {
   dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid());
 }
 

gfx/layers/ipc/UiCompositorControllerChild.h

@@ -77,7 +77,7 @@ class UiCompositorControllerChild final
  protected:
   void ActorDestroy(ActorDestroyReason aWhy) override;
   void ProcessingError(Result aCode, const char* aReason) override;
-  void HandleFatalError(const char* aMsg) const override;
+  void HandleFatalError(const char* aMsg) override;
   mozilla::ipc::IPCResult RecvToolbarAnimatorMessageFromCompositor(
       const int32_t& aMessage);
   mozilla::ipc::IPCResult RecvRootFrameMetrics(const ScreenPoint& aScrollOffset,

gfx/layers/ipc/VideoBridgeChild.cpp

@@ -166,7 +166,7 @@ bool VideoBridgeChild::IsSameProcess() const {
   return OtherPid() == base::GetCurrentProcId();
 }
 
-void VideoBridgeChild::HandleFatalError(const char* aMsg) const {
+void VideoBridgeChild::HandleFatalError(const char* aMsg) {
   dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid());
 }
 

gfx/layers/ipc/VideoBridgeChild.h

@@ -63,7 +63,7 @@ class VideoBridgeChild final : public PVideoBridgeChild,
   static void Open(Endpoint<PVideoBridgeChild>&& aEndpoint);
 
  protected:
-  void HandleFatalError(const char* aMsg) const override;
+  void HandleFatalError(const char* aMsg) override;
   bool DispatchAllocShmemInternal(size_t aSize, mozilla::ipc::Shmem* aShmem,
                                   bool aUnsafe);
   void ProxyAllocShmemNow(SynchronousTask* aTask, size_t aSize,

gfx/vr/ipc/VRManagerChild.cpp

@@ -589,7 +589,7 @@ void VRManagerChild::StopActivity() {
   Unused << SendStopActivity();
 }
 
-void VRManagerChild::HandleFatalError(const char* aMsg) const {
+void VRManagerChild::HandleFatalError(const char* aMsg) {
   dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid());
 }
 

gfx/vr/ipc/VRManagerChild.h

@@ -102,7 +102,7 @@ class VRManagerChild : public PVRManagerChild {
   void FireDOMVRDisplayPresentChangeEvent(uint32_t aDisplayID);
   void FireDOMVRDisplayConnectEventsForLoad(VRManagerEventObserver* aObserver);
 
-  void HandleFatalError(const char* aMsg) const override;
+  void HandleFatalError(const char* aMsg) override;
   void ActorDestroy(ActorDestroyReason aReason) override;
 
   void RunPuppet(const nsTArray<uint64_t>& aBuffer, dom::Promise* aPromise,

ipc/glue/ProtocolUtils.cpp

@@ -389,17 +389,20 @@ Maybe<IProtocol*> IProtocol::ReadActor(IPC::MessageReader* aReader,
   return Some(listener);
 }
 
-void IProtocol::FatalError(const char* const aErrorMsg) const {
+void IProtocol::FatalError(const char* const aErrorMsg) {
   HandleFatalError(aErrorMsg);
 }
 
-void IProtocol::HandleFatalError(const char* aErrorMsg) const {
+void IProtocol::HandleFatalError(const char* aErrorMsg) {
   if (IProtocol* manager = Manager()) {
     manager->HandleFatalError(aErrorMsg);
     return;
   }
 
   mozilla::ipc::FatalError(aErrorMsg, mSide == ParentSide);
+  if (CanSend()) {
+    GetIPCChannel()->CloseWithError();
+  }
 }
 
 bool IProtocol::AllocShmem(size_t aSize, Shmem* aOutMem) {
@@ -630,6 +633,8 @@ void IToplevelProtocol::NotifyImpendingShutdown() {
 
 void IToplevelProtocol::Close() { GetIPCChannel()->Close(); }
 
+void IToplevelProtocol::CloseWithError() { GetIPCChannel()->CloseWithError(); }
+
 void IToplevelProtocol::SetReplyTimeoutMs(int32_t aTimeoutMs) {
   GetIPCChannel()->SetReplyTimeoutMs(aTimeoutMs);
 }

ipc/glue/ProtocolUtils.h

@@ -242,8 +242,8 @@ class IProtocol : public HasResultCodes {
   bool AllocUnsafeShmem(size_t aSize, Shmem* aOutMem);
   bool DeallocShmem(Shmem& aMem);
 
-  void FatalError(const char* const aErrorMsg) const;
-  virtual void HandleFatalError(const char* aErrorMsg) const;
+  void FatalError(const char* const aErrorMsg);
+  virtual void HandleFatalError(const char* aErrorMsg);
 
  protected:
   virtual ~IProtocol();
@@ -433,6 +433,7 @@ class IToplevelProtocol : public IProtocol {
   void NotifyImpendingShutdown();
 
   void Close();
+  void CloseWithError();
 
   void SetReplyTimeoutMs(int32_t aTimeoutMs);
 

ipc/ipdl/test/cxx/TestActorPunning.cpp

@@ -23,7 +23,7 @@ mozilla::ipc::IPCResult TestActorPunningParent::RecvPun(
 // By default, fatal errors kill the parent process, but this makes it
 // hard to test, so instead we use the previous behavior and kill the
 // child process.
-void TestActorPunningParent::HandleFatalError(const char* aErrorMsg) const {
+void TestActorPunningParent::HandleFatalError(const char* aErrorMsg) {
   if (!!strcmp(aErrorMsg, "Error deserializing 'PTestActorPunningSubParent'")) {
     fail("wrong fatal error");
   }

ipc/ipdl/test/cxx/TestActorPunning.h

@@ -38,7 +38,7 @@ class TestActorPunningParent : public PTestActorPunningParent {
     QuitParent();
   }
 
-  virtual void HandleFatalError(const char* aErrorMsg) const override;
+  virtual void HandleFatalError(const char* aErrorMsg) override;
 };
 
 class TestActorPunningPunnedParent : public PTestActorPunningPunnedParent {

ipc/ipdl/test/cxx/TestBadActor.cpp

@@ -19,7 +19,7 @@ void TestBadActorParent::Main() {
 // By default, fatal errors kill the parent process, but this makes it
 // hard to test, so instead we use the previous behavior and kill the
 // child process.
-void TestBadActorParent::HandleFatalError(const char* aErrorMsg) const {
+void TestBadActorParent::HandleFatalError(const char* aErrorMsg) {
   if (!!strcmp(aErrorMsg, "incoming message racing with actor deletion")) {
     fail("wrong fatal error");
   }

ipc/ipdl/test/cxx/TestBadActor.h

@@ -31,7 +31,7 @@ class TestBadActorParent : public PTestBadActorParent {
     QuitParent();
   }
 
-  virtual void HandleFatalError(const char* aErrorMsg) const override;
+  virtual void HandleFatalError(const char* aErrorMsg) override;
 
   PTestBadActorSubParent* AllocPTestBadActorSubParent();