Bug 1424922 - Prevent calling PDFiumParent::Close twice. r=dvander
authorcku <cku@mozilla.com>
Wed, 13 Dec 2017 12:42:59 +0800
changeset 450799 8320a9f9de3e4ef593cf8c5a96c832ed5c28c9fc
parent 450798 44e41af5142433bd87a37656f2e41f617db5cf0f
child 450800 3e65a15f6d0c0899a4ce78f11a39ddec5ec289df
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander
bugs1424922
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
gfx/thebes/PrintTargetEMF.cpp
gfx/thebes/PrintTargetEMF.h
widget/windows/PDFiumParent.cpp
widget/windows/PDFiumParent.h
widget/windows/PDFiumProcessParent.cpp
widget/windows/PDFiumProcessParent.h
--- a/gfx/thebes/PrintTargetEMF.cpp
+++ b/gfx/thebes/PrintTargetEMF.cpp
@@ -17,25 +17,24 @@ using mozilla::ipc::FileDescriptor;
 
 namespace mozilla {
 namespace gfx {
 
 PrintTargetEMF::PrintTargetEMF(HDC aDC, const IntSize& aSize)
   : PrintTarget(/* not using cairo_surface_t */ nullptr, aSize)
   , mPDFiumProcess(nullptr)
   , mPrinterDC(aDC)
-  , mWaitingForEMFConversion(false)
   , mChannelBroken(false)
 {
 }
 
 PrintTargetEMF::~PrintTargetEMF()
 {
   if (mPDFiumProcess) {
-    mPDFiumProcess->Delete(mWaitingForEMFConversion);
+    mPDFiumProcess->Delete();
   }
 }
 
 /* static */ already_AddRefed<PrintTargetEMF>
 PrintTargetEMF::CreateOrNull(HDC aDC, const IntSize& aSizeInPoints)
 {
   return do_AddRef(new PrintTargetEMF(aDC, aSizeInPoints));
 }
@@ -132,17 +131,16 @@ PrintTargetEMF::EndPage()
   if (!mPDFiumProcess->GetActor()->SendConvertToEMF(descriptor,
                                         ::GetDeviceCaps(mPrinterDC, HORZRES),
                                         ::GetDeviceCaps(mPrinterDC, VERTRES)))
   {
     return NS_ERROR_FAILURE;
   }
 
   PR_Close(prfile);
-  mWaitingForEMFConversion = true;
 
   return NS_OK;
 }
 
 already_AddRefed<DrawTarget>
 PrintTargetEMF::MakeDrawTarget(const IntSize& aSize,
                                DrawEventRecorder* aRecorder)
 {
@@ -168,17 +166,16 @@ PrintTargetEMF::GetReferenceDrawTarget(D
 void
 PrintTargetEMF::ConvertToEMFDone(const nsresult& aResult,
                                  mozilla::ipc::Shmem&& aEMF)
 {
   MOZ_ASSERT_IF(NS_FAILED(aResult), aEMF.Size<uint8_t>() == 0);
   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;
       emf.InitFromFileContents(aEMF.get<BYTE>(), aEMF.Size<BYTE>());
       RECT printRect = {0, 0, ::GetDeviceCaps(mPrinterDC, HORZRES),
                         ::GetDeviceCaps(mPrinterDC, VERTRES)};
       DebugOnly<bool> ret = emf.Playback(mPrinterDC, printRect);
       MOZ_ASSERT(ret);
--- a/gfx/thebes/PrintTargetEMF.h
+++ b/gfx/thebes/PrintTargetEMF.h
@@ -63,16 +63,15 @@ private:
   ~PrintTargetEMF() override;
 
   nsString mTitle;
   RefPtr<PrintTargetSkPDF> mTargetForCurrentPage;
   nsCOMPtr<nsIFile>        mPDFFileForOnePage;
   RefPtr<PrintTargetSkPDF> mRefTarget;
   PDFiumProcessParent*     mPDFiumProcess;
   HDC mPrinterDC;
-  bool mWaitingForEMFConversion;
   bool mChannelBroken;
 };
 
 } // namespace gfx
 } // namespace mozilla
 
 #endif /* MOZILLA_GFX_PRINTTARGETEMF_H */
--- a/widget/windows/PDFiumParent.cpp
+++ b/widget/windows/PDFiumParent.cpp
@@ -24,53 +24,41 @@ PDFiumParent::Init(IPC::Channel* aChanne
 
   AddRef();
   return true;
 }
 
 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
 PDFiumParent::RecvConvertToEMFDone(const nsresult& aResult,
                                    mozilla::ipc::Shmem&& aEMFContents)
 {
   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;
 }
 
 void
 PDFiumParent::OnChannelConnected(int32_t pid)
 {
   SetOtherProcessId(pid);
 }
--- a/widget/windows/PDFiumParent.h
+++ b/widget/windows/PDFiumParent.h
@@ -18,37 +18,34 @@ namespace mozilla {
 namespace widget {
 
 class PDFiumParent final : public PPDFiumParent,
                            public mozilla::ipc::IShmemAllocator
 {
   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)
 private:
   ~PDFiumParent() {}
 
   // PPDFiumParent functions.
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   mozilla::ipc::IPCResult RecvConvertToEMFDone(const nsresult& aResult,
                                                mozilla::ipc::Shmem&& aEMFContents) override;
   void OnChannelConnected(int32_t pid) override;
   void DeallocPPDFiumParent() override;
 
   PrintTargetEMF* mTarget;
-  ConversionDoneCallback mConversionDoneCallback;
 };
 
 } // namespace widget
 } // namespace mozilla
 
 #endif // PDFIUMPARENT_H_
--- a/widget/windows/PDFiumProcessParent.cpp
+++ b/widget/windows/PDFiumProcessParent.cpp
@@ -21,20 +21,16 @@ PDFiumProcessParent::PDFiumProcessParent
   : GeckoChildProcessHost(GeckoProcessType_PDFium)
 {
   MOZ_COUNT_CTOR(PDFiumProcessParent);
 }
 
 PDFiumProcessParent::~PDFiumProcessParent()
 {
   MOZ_COUNT_DTOR(PDFiumProcessParent);
-
-  if (mPDFiumParentActor) {
-    mPDFiumParentActor->Close();
-  }
 }
 
 bool
 PDFiumProcessParent::Launch(PrintTargetEMF* aTarget)
 {
   mLaunchThread = NS_GetCurrentThread();
 
   if (!SyncLaunch()) {
@@ -44,39 +40,29 @@ PDFiumProcessParent::Launch(PrintTargetE
   // Open the top level protocol for PDFium process.
   MOZ_ASSERT(!mPDFiumParentActor);
   mPDFiumParentActor = new PDFiumParent(aTarget);
   return mPDFiumParentActor->Init(GetChannel(),
                             base::GetProcId(GetChildProcessHandle()));
 }
 
 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
 } // namespace mozilla
--- a/widget/windows/PDFiumProcessParent.h
+++ b/widget/windows/PDFiumProcessParent.h
@@ -32,17 +32,17 @@ class PDFiumProcessParent final : public
 public:
   typedef mozilla::gfx::PrintTargetEMF PrintTargetEMF;
 
   PDFiumProcessParent();
   ~PDFiumProcessParent();
 
   bool Launch(PrintTargetEMF* aTarget);
 
-  void Delete(bool aWaitingForEMFConversion);
+  void Delete();
 
   bool CanShutdown() override { return true; }
 
   PDFiumParent* GetActor() const { return mPDFiumParentActor; }
 private:
 
   DISALLOW_COPY_AND_ASSIGN(PDFiumProcessParent);