0

Use WeakPtr for BlobStorageContext in BlobRegistryImpl.

Since BlobStorageContext and BlobRegistryImpl are owned by different
classes it is possible for BlobRegistryImpl to outlive
BlobStorageContext. So rather than keeping a raw pointer to the context,
change that to a weak pointer, and guard every access to it with a null
check.

Bug: 779495
Change-Id: Ie35bc4135c4839cfd4c3e3836f83669349e7401a
Reviewed-on: https://chromium-review.googlesource.com/748912
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513216}
This commit is contained in:
Marijn Kruisselbrink
2017-11-01 19:32:19 +00:00
committed by Commit Bot
parent f65944765c
commit 073fff4d14
4 changed files with 44 additions and 12 deletions

@ -8,6 +8,7 @@
#include "content/browser/child_process_security_policy_impl.h"
#include "content/public/common/content_features.h"
#include "storage/browser/blob/blob_registry_impl.h"
#include "storage/browser/blob/blob_storage_context.h"
#include "storage/browser/fileapi/file_system_context.h"
namespace content {
@ -72,7 +73,8 @@ void BlobRegistryWrapper::InitializeOnIOThread(
scoped_refptr<storage::FileSystemContext> file_system_context) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
blob_registry_ = std::make_unique<storage::BlobRegistryImpl>(
blob_storage_context->context(), std::move(file_system_context));
blob_storage_context->context()->AsWeakPtr(),
std::move(file_system_context));
}
} // namespace content

@ -54,7 +54,7 @@ class BlobURLHandleImpl final : public blink::mojom::BlobURLHandle {
const GURL& url) {
blink::mojom::BlobURLHandlePtr ptr;
mojo::MakeStrongBinding(
base::WrapUnique(new BlobURLHandleImpl(context, url)),
base::WrapUnique(new BlobURLHandleImpl(std::move(context), url)),
mojo::MakeRequest(&ptr));
return ptr;
}
@ -103,7 +103,7 @@ class BlobRegistryImpl::BlobUnderConstruction {
const std::string& uuid() const { return builder_.uuid(); }
private:
BlobStorageContext* context() const { return blob_registry_->context_; }
BlobStorageContext* context() const { return blob_registry_->context_.get(); }
// Marks this blob as broken. If an optional |bad_message_reason| is provided,
// this will also report a BadMessage on the binding over which the initial
@ -115,7 +115,7 @@ class BlobRegistryImpl::BlobUnderConstruction {
DCHECK_EQ(bad_message_reason.empty(), !BlobStatusIsBadIPC(reason));
// The blob might no longer have any references, in which case it may no
// longer exist. If that happens just skip calling cancel.
if (context()->registry().HasEntry(uuid()))
if (context() && context()->registry().HasEntry(uuid()))
context()->CancelBuildingBlob(uuid(), reason);
if (!bad_message_reason.empty())
std::move(bad_message_callback_).Run(bad_message_reason);
@ -199,6 +199,11 @@ class BlobRegistryImpl::BlobUnderConstruction {
};
void BlobRegistryImpl::BlobUnderConstruction::StartTransportation() {
if (!context()) {
MarkAsFinishedAndDeleteSelf();
return;
}
size_t blob_count = 0;
for (size_t i = 0; i < elements_.size(); ++i) {
const auto& element = elements_[i];
@ -282,6 +287,11 @@ void BlobRegistryImpl::BlobUnderConstruction::ResolvedAllBlobUUIDs() {
}
#endif
if (!context()) {
MarkAsFinishedAndDeleteSelf();
return;
}
if (referenced_blob_uuids_.size() == 0) {
ResolvedAllBlobDependencies();
return;
@ -321,6 +331,11 @@ void BlobRegistryImpl::BlobUnderConstruction::ResolvedAllBlobDependencies() {
DCHECK_EQ(resolved_blob_uuid_count_, referenced_blob_uuids_.size());
DCHECK_EQ(ready_dependent_blob_count_, referenced_blob_uuids_.size());
if (!context()) {
MarkAsFinishedAndDeleteSelf();
return;
}
auto blob_uuid_it = referenced_blob_uuids_.begin();
for (const auto& element : elements_) {
if (element->is_bytes()) {
@ -378,6 +393,11 @@ void BlobRegistryImpl::BlobUnderConstruction::OnReadyForTransport(
void BlobRegistryImpl::BlobUnderConstruction::TransportComplete(
BlobStatus result) {
if (!context()) {
MarkAsFinishedAndDeleteSelf();
return;
}
// The blob might no longer have any references, in which case it may no
// longer exist. If that happens just skip calling Complete.
// TODO(mek): Stop building sooner if a blob is no longer referenced.
@ -417,9 +437,9 @@ bool BlobRegistryImpl::BlobUnderConstruction::ContainsCycles(
#endif
BlobRegistryImpl::BlobRegistryImpl(
BlobStorageContext* context,
base::WeakPtr<BlobStorageContext> context,
scoped_refptr<FileSystemContext> file_system_context)
: context_(context),
: context_(std::move(context)),
file_system_context_(std::move(file_system_context)),
weak_ptr_factory_(this) {}
@ -438,6 +458,11 @@ void BlobRegistryImpl::Register(
const std::string& content_disposition,
std::vector<blink::mojom::DataElementPtr> elements,
RegisterCallback callback) {
if (!context_) {
std::move(callback).Run();
return;
}
if (uuid.empty() || context_->registry().HasEntry(uuid) ||
base::ContainsKey(blobs_under_construction_, uuid)) {
bindings_.ReportBadMessage("Invalid UUID passed to BlobRegistry::Register");
@ -496,6 +521,11 @@ void BlobRegistryImpl::Register(
void BlobRegistryImpl::GetBlobFromUUID(blink::mojom::BlobRequest blob,
const std::string& uuid,
GetBlobFromUUIDCallback callback) {
if (!context_) {
std::move(callback).Run();
return;
}
if (uuid.empty()) {
bindings_.ReportBadMessage(
"Invalid UUID passed to BlobRegistry::GetBlobFromUUID");
@ -533,9 +563,9 @@ void BlobRegistryImpl::RegisterURLWithUUID(const GURL& url,
const std::string& uuid) {
// |blob| is unused, but is passed here to be kept alive until
// RegisterBlobURL increments the refcount of it via the uuid.
context_->RegisterPublicBlobURL(url, uuid);
std::move(callback).Run(
BlobURLHandleImpl::Create(context_->AsWeakPtr(), url));
if (context_)
context_->RegisterPublicBlobURL(url, uuid);
std::move(callback).Run(BlobURLHandleImpl::Create(context_, url));
}
} // namespace storage

@ -28,7 +28,7 @@ class STORAGE_EXPORT BlobRegistryImpl : public blink::mojom::BlobRegistry {
virtual bool CanCommitURL(const GURL& url) = 0;
};
BlobRegistryImpl(BlobStorageContext* context,
BlobRegistryImpl(base::WeakPtr<BlobStorageContext> context,
scoped_refptr<FileSystemContext> file_system_context);
~BlobRegistryImpl() override;
@ -61,7 +61,7 @@ class STORAGE_EXPORT BlobRegistryImpl : public blink::mojom::BlobRegistry {
class BlobUnderConstruction;
BlobStorageContext* context_;
base::WeakPtr<BlobStorageContext> context_;
scoped_refptr<FileSystemContext> file_system_context_;
mojo::BindingSet<blink::mojom::BlobRegistry, std::unique_ptr<Delegate>>

@ -105,7 +105,7 @@ class BlobRegistryImplTest : public testing::Test {
std::vector<URLRequestAutoMountHandler>(), data_dir_.GetPath(),
FileSystemOptions(FileSystemOptions::PROFILE_MODE_INCOGNITO,
std::vector<std::string>(), nullptr));
registry_impl_ = base::MakeUnique<BlobRegistryImpl>(context_.get(),
registry_impl_ = base::MakeUnique<BlobRegistryImpl>(context_->AsWeakPtr(),
file_system_context_);
auto delegate = base::MakeUnique<MockDelegate>();
delegate_ptr_ = delegate.get();