Bug 1424922 - Prevent calling PDFiumParent::Close twice.
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
--- 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);