From c9ba9a819bf112d7485e6dc4a48d9c912b23e6fd Mon Sep 17 00:00:00 2001 From: Andrea Marchesini Date: Wed, 25 Nov 2015 17:48:19 +0000 Subject: [PATCH] Bug 1211266 - Remote blobs coming from a different thread and a different manager must be kept alive until the creation of depending RemoteBlobs is not completed, r=bent, f=gerard-majax --- dom/base/StructuredCloneHolder.cpp | 9 +- dom/ipc/Blob.cpp | 127 ++++++++++++++++------------- dom/ipc/BlobChild.h | 3 + dom/ipc/ContentParent.cpp | 12 +++ dom/ipc/ContentParent.h | 3 + dom/ipc/PBlob.ipdl | 5 ++ ipc/glue/BackgroundParentImpl.cpp | 13 +++ ipc/glue/BackgroundParentImpl.h | 4 + 8 files changed, 117 insertions(+), 59 deletions(-) diff --git a/dom/base/StructuredCloneHolder.cpp b/dom/base/StructuredCloneHolder.cpp index 6fc6229f2bb2..b069f7d7b1f7 100644 --- a/dom/base/StructuredCloneHolder.cpp +++ b/dom/base/StructuredCloneHolder.cpp @@ -558,14 +558,15 @@ EnsureBlobForBackgroundManager(BlobImpl* aBlobImpl, PBackgroundChild* aManager = nullptr) { MOZ_ASSERT(aBlobImpl); + RefPtr blobImpl = aBlobImpl; if (!aManager) { aManager = BackgroundChild::GetForCurrentThread(); - MOZ_ASSERT(aManager); + if (!aManager) { + return blobImpl.forget(); + } } - RefPtr blobImpl = aBlobImpl; - const nsTArray>* subBlobImpls = aBlobImpl->GetSubBlobImpls(); @@ -670,6 +671,8 @@ WriteBlob(JSStructuredCloneWriter* aWriter, RefPtr blobImpl = EnsureBlobForBackgroundManager(aBlob->Impl()); MOZ_ASSERT(blobImpl); + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(blobImpl->SetMutable(false))); + // We store the position of the blobImpl in the array as index. if (JS_WriteUint32Pair(aWriter, SCTAG_DOM_BLOB, aHolder->BlobImpls().Length())) { diff --git a/dom/ipc/Blob.cpp b/dom/ipc/Blob.cpp index 489a777fe5d1..7c4ba3eae9db 100644 --- a/dom/ipc/Blob.cpp +++ b/dom/ipc/Blob.cpp @@ -1753,6 +1753,12 @@ protected: BlobChild* mActor; nsCOMPtr mActorTarget; + // We use this pointer to keep a live a blobImpl coming from a different + // process until this one is fully created. We set it to null when + // SendCreatedFromKnownBlob() is received. This is used only with KnownBlob + // params in the CTOR of a IPC BlobImpl. + RefPtr mDifferentProcessBlobImpl; + RefPtr mSameProcessBlobImpl; const bool mIsSlice; @@ -1760,31 +1766,20 @@ protected: public: // For File. RemoteBlobImpl(BlobChild* aActor, + BlobImpl* aRemoteBlobImpl, const nsAString& aName, const nsAString& aContentType, uint64_t aLength, int64_t aModDate, - BlobDirState aDirState); + BlobDirState aDirState, + bool aIsSameProcessBlob); // For Blob. RemoteBlobImpl(BlobChild* aActor, - const nsAString& aContentType, - uint64_t aLength); - - // For same-process blobs. - RemoteBlobImpl(BlobChild* aActor, - BlobImpl* aSameProcessBlobImpl, - const nsAString& aName, + BlobImpl* aRemoteBlobImpl, const nsAString& aContentType, uint64_t aLength, - int64_t aModDate, - BlobDirState aDirState); - - // For same-process blobs. - RemoteBlobImpl(BlobChild* aActor, - BlobImpl* aSameProcessBlobImpl, - const nsAString& aContentType, - uint64_t aLength); + bool aIsSameProcessBlob); // For mystery blobs. explicit @@ -1857,6 +1852,13 @@ public: virtual BlobParent* GetBlobParent() override; + void + NullifyDifferentProcessBlobImpl() + { + MOZ_ASSERT(mDifferentProcessBlobImpl); + mDifferentProcessBlobImpl = nullptr; + } + protected: // For SliceImpl. RemoteBlobImpl(const nsAString& aContentType, uint64_t aLength); @@ -2086,56 +2088,43 @@ private: BlobChild:: RemoteBlobImpl::RemoteBlobImpl(BlobChild* aActor, + BlobImpl* aRemoteBlobImpl, const nsAString& aName, const nsAString& aContentType, uint64_t aLength, int64_t aModDate, - BlobDirState aDirState) + BlobDirState aDirState, + bool aIsSameProcessBlob) : BlobImplBase(aName, aContentType, aLength, aModDate, aDirState) , mIsSlice(false) { + if (aIsSameProcessBlob) { + MOZ_ASSERT(aRemoteBlobImpl); + mSameProcessBlobImpl = aRemoteBlobImpl; + MOZ_ASSERT(gProcessType == GeckoProcessType_Default); + } else { + mDifferentProcessBlobImpl = aRemoteBlobImpl; + } + CommonInit(aActor); } BlobChild:: RemoteBlobImpl::RemoteBlobImpl(BlobChild* aActor, - const nsAString& aContentType, - uint64_t aLength) - : BlobImplBase(aContentType, aLength) - , mIsSlice(false) -{ - CommonInit(aActor); -} - -BlobChild:: -RemoteBlobImpl::RemoteBlobImpl(BlobChild* aActor, - BlobImpl* aSameProcessBlobImpl, - const nsAString& aName, + BlobImpl* aRemoteBlobImpl, const nsAString& aContentType, uint64_t aLength, - int64_t aModDate, - BlobDirState aDirState) - : BlobImplBase(aName, aContentType, aLength, aModDate, aDirState) - , mSameProcessBlobImpl(aSameProcessBlobImpl) - , mIsSlice(false) -{ - MOZ_ASSERT(aSameProcessBlobImpl); - MOZ_ASSERT(gProcessType == GeckoProcessType_Default); - - CommonInit(aActor); -} - -BlobChild:: -RemoteBlobImpl::RemoteBlobImpl(BlobChild* aActor, - BlobImpl* aSameProcessBlobImpl, - const nsAString& aContentType, - uint64_t aLength) + bool aIsSameProcessBlob) : BlobImplBase(aContentType, aLength) - , mSameProcessBlobImpl(aSameProcessBlobImpl) , mIsSlice(false) { - MOZ_ASSERT(aSameProcessBlobImpl); - MOZ_ASSERT(gProcessType == GeckoProcessType_Default); + if (aIsSameProcessBlob) { + MOZ_ASSERT(aRemoteBlobImpl); + mSameProcessBlobImpl = aRemoteBlobImpl; + MOZ_ASSERT(gProcessType == GeckoProcessType_Default); + } else { + mDifferentProcessBlobImpl = aRemoteBlobImpl; + } CommonInit(aActor); } @@ -3041,12 +3030,19 @@ BlobChild::CommonInit(BlobChild* aOther, BlobImpl* aBlobImpl) int64_t modDate = otherImpl->GetLastModified(rv); MOZ_ASSERT(!rv.Failed()); - remoteBlob = new RemoteBlobImpl(this, name, contentType, length, modDate, - otherImpl->GetDirState()); + remoteBlob = new RemoteBlobImpl(this, otherImpl, name, contentType, length, + modDate, otherImpl->GetDirState(), + false /* SameProcessBlobImpl */); } else { - remoteBlob = new RemoteBlobImpl(this, contentType, length); + remoteBlob = new RemoteBlobImpl(this, otherImpl, contentType, length, + false /* SameProcessBlobImpl */); } + // This RemoteBlob must be kept alive untill RecvCreatedFromKnownBlob is + // called because the parent will send this notification and we must be able + // to manage it. + remoteBlob->AddRef(); + CommonInit(aOther->ParentID(), remoteBlob); } @@ -3073,7 +3069,8 @@ BlobChild::CommonInit(const ChildBlobConstructorParams& aParams) const NormalBlobConstructorParams& params = blobParams.get_NormalBlobConstructorParams(); remoteBlob = - new RemoteBlobImpl(this, params.contentType(), params.length()); + new RemoteBlobImpl(this, nullptr, params.contentType(), params.length(), + false /* SameProcessBlobImpl */); break; } @@ -3081,11 +3078,13 @@ BlobChild::CommonInit(const ChildBlobConstructorParams& aParams) const FileBlobConstructorParams& params = blobParams.get_FileBlobConstructorParams(); remoteBlob = new RemoteBlobImpl(this, + nullptr, params.name(), params.contentType(), params.length(), params.modDate(), - BlobDirState(params.dirState())); + BlobDirState(params.dirState()), + false /* SameProcessBlobImpl */); break; } @@ -3120,9 +3119,11 @@ BlobChild::CommonInit(const ChildBlobConstructorParams& aParams) contentType, size, lastModifiedDate, - blobImpl->GetDirState()); + blobImpl->GetDirState(), + true /* SameProcessBlobImpl */); } else { - remoteBlob = new RemoteBlobImpl(this, blobImpl, contentType, size); + remoteBlob = new RemoteBlobImpl(this, blobImpl, contentType, size, + true /* SameProcessBlobImpl */); } break; @@ -3592,6 +3593,20 @@ BlobChild::DeallocPBlobStreamChild(PBlobStreamChild* aActor) return true; } +bool +BlobChild::RecvCreatedFromKnownBlob() +{ + MOZ_ASSERT(mRemoteBlobImpl); + + // Releasing the other blob now that this blob is fully created. + mRemoteBlobImpl->NullifyDifferentProcessBlobImpl(); + + // Release the additional reference to ourself that was added in order to + // receive this RecvCreatedFromKnownBlob. + mRemoteBlobImpl->Release(); + return true; +} + /******************************************************************************* * BlobParent ******************************************************************************/ diff --git a/dom/ipc/BlobChild.h b/dom/ipc/BlobChild.h index 1b5627378bce..35853f307dfb 100644 --- a/dom/ipc/BlobChild.h +++ b/dom/ipc/BlobChild.h @@ -220,6 +220,9 @@ private: virtual bool DeallocPBlobStreamChild(PBlobStreamChild* aActor) override; + + virtual bool + RecvCreatedFromKnownBlob() override; }; // Only let ContentChild call BlobChild::Startup() and ensure that diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 06783199264b..5fe81e35cb8e 100755 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -3515,6 +3515,18 @@ ContentParent::DeallocPBlobParent(PBlobParent* aActor) return nsIContentParent::DeallocPBlobParent(aActor); } +bool +ContentParent::RecvPBlobConstructor(PBlobParent* aActor, + const BlobConstructorParams& aParams) +{ + const ParentBlobConstructorParams& params = aParams.get_ParentBlobConstructorParams(); + if (params.blobParams().type() == AnyBlobConstructorParams::TKnownBlobConstructorParams) { + return aActor->SendCreatedFromKnownBlob(); + } + + return true; +} + mozilla::PRemoteSpellcheckEngineParent * ContentParent::AllocPRemoteSpellcheckEngineParent() { diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index 88c18cffccec..1f896e87a416 100644 --- a/dom/ipc/ContentParent.h +++ b/dom/ipc/ContentParent.h @@ -648,6 +648,9 @@ private: override; virtual bool DeallocPBlobParent(PBlobParent* aActor) override; + virtual bool RecvPBlobConstructor(PBlobParent* aActor, + const BlobConstructorParams& params) override; + virtual bool DeallocPCrashReporterParent(PCrashReporterParent* crashreporter) override; virtual bool RecvGetRandomValues(const uint32_t& length, diff --git a/dom/ipc/PBlob.ipdl b/dom/ipc/PBlob.ipdl index b2e57e92ce97..09b78b16308a 100644 --- a/dom/ipc/PBlob.ipdl +++ b/dom/ipc/PBlob.ipdl @@ -45,6 +45,11 @@ parent: // Use only for testing! sync GetFilePath() returns (nsString filePath); + +child: + // This method must be called by the parent when the PBlobParent is fully + // created in order to release the known blob. + CreatedFromKnownBlob(); }; } // namespace dom diff --git a/ipc/glue/BackgroundParentImpl.cpp b/ipc/glue/BackgroundParentImpl.cpp index bbf5c8876565..e9258318dd02 100644 --- a/ipc/glue/BackgroundParentImpl.cpp +++ b/ipc/glue/BackgroundParentImpl.cpp @@ -13,6 +13,7 @@ #include "mozilla/AppProcessChecker.h" #include "mozilla/Assertions.h" #include "mozilla/dom/ContentParent.h" +#include "mozilla/dom/DOMTypes.h" #include "mozilla/dom/NuwaParent.h" #include "mozilla/dom/PBlobParent.h" #include "mozilla/dom/MessagePortParent.h" @@ -243,6 +244,18 @@ BackgroundParentImpl::DeallocPBlobParent(PBlobParent* aActor) return true; } +bool +BackgroundParentImpl::RecvPBlobConstructor(PBlobParent* aActor, + const BlobConstructorParams& aParams) +{ + const ParentBlobConstructorParams& params = aParams; + if (params.blobParams().type() == AnyBlobConstructorParams::TKnownBlobConstructorParams) { + return aActor->SendCreatedFromKnownBlob(); + } + + return true; +} + PFileDescriptorSetParent* BackgroundParentImpl::AllocPFileDescriptorSetParent( const FileDescriptor& aFileDescriptor) diff --git a/ipc/glue/BackgroundParentImpl.h b/ipc/glue/BackgroundParentImpl.h index 1142aa85649b..2dc5b2953d29 100644 --- a/ipc/glue/BackgroundParentImpl.h +++ b/ipc/glue/BackgroundParentImpl.h @@ -64,6 +64,10 @@ protected: virtual bool DeallocPBlobParent(PBlobParent* aActor) override; + virtual bool + RecvPBlobConstructor(PBlobParent* aActor, + const BlobConstructorParams& params) override; + virtual PFileDescriptorSetParent* AllocPFileDescriptorSetParent(const FileDescriptor& aFileDescriptor) override;