Bug 1424922 - Prevent calling PDFiumParent::Close twice. r=dvander

We call PDFiumParent::Close twice under certain conditions. Once in
PDFiumProcessParent::Delete, and once in PDFiumProcessParent's dtor. So we may
hit MOZ_ABORT which tell us that we are trying to close a closed channel.

This patch prevents hitting this abort by:
1. Only close the channel in PDFiumProcessParent::Delete, remove another call
   in PDFiumProcessParent's dtor. (Please see the change in
   PDFiumProcessParent.cpp).
2. Remove PDFiumParent::AbortConversion and relative code. We can just use
   PDFiumParent::EndConversion instead. When calling PDFiumParent::Close, we
   actually close the IPC channel *synchronously*, which means there is no need
   to register a callback by PDFiumParent::AbortConversion to receive
   actor-destroy callback.

MozReview-Commit-ID: 9i5j6t54J3h

--HG--
extra : rebase_source : 5f74ebc1ecc29e9983c30ca2dd63e0b49bd24a50

gfx/thebes/PrintTargetEMF.cpp

@@ -22,7 +22,6 @@ PrintTargetEMF::PrintTargetEMF(HDC aDC, const IntSize& aSize)
   : PrintTarget(/* not using cairo_surface_t */ nullptr, aSize)
   , mPDFiumProcess(nullptr)
   , mPrinterDC(aDC)
-  , mWaitingForEMFConversion(false)
   , mChannelBroken(false)
 {
 }
@@ -30,7 +29,7 @@ PrintTargetEMF::PrintTargetEMF(HDC aDC, const IntSize& aSize)
 PrintTargetEMF::~PrintTargetEMF()
 {
   if (mPDFiumProcess) {
-    mPDFiumProcess->Delete(mWaitingForEMFConversion);
+    mPDFiumProcess->Delete();
   }
 }
 
@@ -137,7 +136,6 @@ PrintTargetEMF::EndPage()
   }
 
   PR_Close(prfile);
-  mWaitingForEMFConversion = true;
 
   return NS_OK;
 }
@@ -173,7 +171,6 @@ PrintTargetEMF::ConvertToEMFDone(const nsresult& aResult,
   MOZ_ASSERT(!mChannelBroken, "It is not possible to get conversion callback "
                               "after the channel was broken.");
 
-  mWaitingForEMFConversion = false;
   if (NS_SUCCEEDED(aResult)) {
     if (::StartPage(mPrinterDC) > 0) {
       mozilla::widget::WindowsEMF emf;

gfx/thebes/PrintTargetEMF.h

@@ -68,7 +68,6 @@ private:
   RefPtr<PrintTargetSkPDF> mRefTarget;
   PDFiumProcessParent*     mPDFiumProcess;
   HDC mPrinterDC;
-  bool mWaitingForEMFConversion;
   bool mChannelBroken;
 };
 

widget/windows/PDFiumParent.cpp

@@ -29,15 +29,12 @@ PDFiumParent::Init(IPC::Channel* aChannel, base::ProcessId aPid)
 void
 PDFiumParent::ActorDestroy(ActorDestroyReason aWhy)
 {
+  // mTarget is not nullptr, which means the print job is not done
+  // (EndConversion is not called yet). The IPC channel is broken for some
+  // reasons. We should tell mTarget to abort this print job.
   if (mTarget) {
     mTarget->ChannelIsBroken();
   }
-
-  if (mConversionDoneCallback) {
-    // Since this printing job was aborted, we do not need to report EMF buffer
-    // back to mTarget.
-    mConversionDoneCallback();
-  }
 }
 
 mozilla::ipc::IPCResult
@@ -47,25 +44,16 @@ PDFiumParent::RecvConvertToEMFDone(const nsresult& aResult,
   MOZ_ASSERT(aEMFContents.IsReadable());
 
   if (mTarget) {
-    MOZ_ASSERT(!mConversionDoneCallback);
     mTarget->ConvertToEMFDone(aResult, Move(aEMFContents));
   }
 
   return IPC_OK();
 }
 
-void
-PDFiumParent::AbortConversion(ConversionDoneCallback aCallback)
-{
-  // There is no need to report EMF contents back to mTarget since the print
-  // job was aborted, unset mTarget.
-  mTarget = nullptr;
-  mConversionDoneCallback = aCallback;
-}
-
 void PDFiumParent::EndConversion()
 {
-  // The printing job is finished correctly, mTarget is no longer needed.
+  // The printing job is done(all pages printed, or print job cancel, or print
+  // job abort), reset mTarget since it may not valid afterward.
   mTarget = nullptr;
 }
 

widget/windows/PDFiumParent.h

@@ -23,13 +23,11 @@ class PDFiumParent final : public PPDFiumParent,
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PDFiumParent)
 
   typedef mozilla::gfx::PrintTargetEMF PrintTargetEMF;
-  typedef std::function<void()> ConversionDoneCallback;
 
   explicit PDFiumParent(PrintTargetEMF* aTarget);
 
   bool Init(IPC::Channel* aChannel, base::ProcessId aPid);
 
-  void AbortConversion(ConversionDoneCallback aCallback);
   void EndConversion();
 
   FORWARD_SHMEM_ALLOCATOR_TO(PPDFiumParent)
@@ -45,7 +43,6 @@ private:
   void DeallocPPDFiumParent() override;
 
   PrintTargetEMF* mTarget;
-  ConversionDoneCallback mConversionDoneCallback;
 };
 
 } // namespace widget

widget/windows/PDFiumProcessParent.cpp

@@ -26,10 +26,6 @@ PDFiumProcessParent::PDFiumProcessParent()
 PDFiumProcessParent::~PDFiumProcessParent()
 {
   MOZ_COUNT_DTOR(PDFiumProcessParent);
-
-  if (mPDFiumParentActor) {
-    mPDFiumParentActor->Close();
-  }
 }
 
 bool
@@ -49,33 +45,23 @@ PDFiumProcessParent::Launch(PrintTargetEMF* aTarget)
 }
 
 void
-PDFiumProcessParent::Delete(bool aWaitingForEMFConversion)
+PDFiumProcessParent::Delete()
 {
-  if (aWaitingForEMFConversion) {
-    // Can not kill the PDFium process yet since we are still waiting for a
-    // EMF conversion response.
-    mPDFiumParentActor->AbortConversion([this]() { Delete(false); });
-    mPDFiumParentActor->Close();
-    return;
-  }
+  // Make sure we do close the IPC channel on the same thread with the one
+  // that we create the channel.
+  if (!mLaunchThread || mLaunchThread == NS_GetCurrentThread()) {
+    if (mPDFiumParentActor) {
+      mPDFiumParentActor->EndConversion();
+      mPDFiumParentActor->Close();
+    }
 
-  // PDFiumProcessParent::Launch is not called, protocol is not created.
-  // It is safe to destroy this object on any thread.
-  if (!mLaunchThread) {
-    delete this;
-    return;
-  }
-
-  if (mLaunchThread == NS_GetCurrentThread()) {
     delete this;
     return;
   }
 
   mLaunchThread->Dispatch(
-    NewNonOwningRunnableMethod<bool>("PDFiumProcessParent::Delete",
-                                     this,
-                                     &PDFiumProcessParent::Delete,
-                                     false));
+    NewNonOwningRunnableMethod("PDFiumProcessParent::Delete", this,
+                               &PDFiumProcessParent::Delete));
 }
 
 } // namespace widget

widget/windows/PDFiumProcessParent.h

@@ -37,7 +37,7 @@ public:
 
   bool Launch(PrintTargetEMF* aTarget);
 
-  void Delete(bool aWaitingForEMFConversion);
+  void Delete();
 
   bool CanShutdown() override { return true; }