From 96b42bddb848f7f7dfad5def958bdbf1545c8c47 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 4 Sep 2019 07:54:14 +0900 Subject: [PATCH] fix: implement ses.getBlobData() for NetworkService (#20041) * pass data pipe to JS * implement reading buffer * re-enable ses.getBlobData test * remove AtomBlobReader --- filenames.gni | 4 +- .../browser/api/atom_api_data_pipe_holder.cc | 185 ++++++++++++++++++ shell/browser/api/atom_api_data_pipe_holder.h | 53 +++++ shell/browser/api/atom_api_session.cc | 16 +- shell/browser/api/atom_api_session.h | 1 - shell/browser/atom_blob_reader.cc | 126 ------------ shell/browser/atom_blob_reader.h | 76 ------- shell/browser/atom_browser_context.cc | 10 - shell/browser/atom_browser_context.h | 3 - shell/common/gin_converters/net_converter.cc | 12 +- spec-main/api-session-spec.ts | 2 +- 11 files changed, 259 insertions(+), 229 deletions(-) create mode 100644 shell/browser/api/atom_api_data_pipe_holder.cc create mode 100644 shell/browser/api/atom_api_data_pipe_holder.h delete mode 100644 shell/browser/atom_blob_reader.cc delete mode 100644 shell/browser/atom_blob_reader.h diff --git a/filenames.gni b/filenames.gni index 6e051fc8d3f0..b6151013b50d 100644 --- a/filenames.gni +++ b/filenames.gni @@ -47,6 +47,8 @@ filenames = { "shell/browser/api/atom_api_content_tracing.cc", "shell/browser/api/atom_api_cookies.cc", "shell/browser/api/atom_api_cookies.h", + "shell/browser/api/atom_api_data_pipe_holder.cc", + "shell/browser/api/atom_api_data_pipe_holder.h", "shell/browser/api/atom_api_debugger.cc", "shell/browser/api/atom_api_debugger.h", "shell/browser/api/atom_api_dialog.cc", @@ -129,8 +131,6 @@ filenames = { "shell/browser/atom_autofill_driver_factory.h", "shell/browser/atom_autofill_driver.cc", "shell/browser/atom_autofill_driver.h", - "shell/browser/atom_blob_reader.cc", - "shell/browser/atom_blob_reader.h", "shell/browser/atom_browser_client.cc", "shell/browser/atom_browser_client.h", "shell/browser/atom_browser_context.cc", diff --git a/shell/browser/api/atom_api_data_pipe_holder.cc b/shell/browser/api/atom_api_data_pipe_holder.cc new file mode 100644 index 000000000000..93fb5383d2cc --- /dev/null +++ b/shell/browser/api/atom_api_data_pipe_holder.cc @@ -0,0 +1,185 @@ +// Copyright (c) 2019 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include "shell/browser/api/atom_api_data_pipe_holder.h" + +#include +#include + +#include "base/memory/weak_ptr.h" +#include "base/strings/string_number_conversions.h" +#include "mojo/public/cpp/system/data_pipe.h" +#include "mojo/public/cpp/system/simple_watcher.h" +#include "net/base/net_errors.h" +#include "shell/common/key_weak_map.h" +#include "shell/common/promise_util.h" + +#include "shell/common/node_includes.h" + +namespace electron { + +namespace api { + +namespace { + +// Incremental ID. +int g_next_id = 0; + +// Map that manages all the DataPipeHolder objects. +KeyWeakMap g_weak_map; + +// Utility class to read from data pipe. +class DataPipeReader { + public: + DataPipeReader(util::Promise> promise, + network::mojom::DataPipeGetterPtr data_pipe_getter) + : promise_(std::move(promise)), + data_pipe_getter_(std::move(data_pipe_getter)), + handle_watcher_(FROM_HERE, + mojo::SimpleWatcher::ArmingPolicy::MANUAL, + base::SequencedTaskRunnerHandle::Get()) { + // Get a new data pipe and start. + mojo::DataPipe data_pipe; + data_pipe_getter_->Read(std::move(data_pipe.producer_handle), + base::BindOnce(&DataPipeReader::ReadCallback, + weak_factory_.GetWeakPtr())); + data_pipe_ = std::move(data_pipe.consumer_handle); + handle_watcher_.Watch(data_pipe_.get(), MOJO_HANDLE_SIGNAL_READABLE, + base::BindRepeating(&DataPipeReader::OnHandleReadable, + weak_factory_.GetWeakPtr())); + } + + ~DataPipeReader() = default; + + private: + // Callback invoked by DataPipeGetter::Read. + void ReadCallback(int32_t status, uint64_t size) { + if (status != net::OK) { + OnFailure(); + return; + } + buffer_.resize(size); + head_ = &buffer_.front(); + remaining_size_ = size; + handle_watcher_.ArmOrNotify(); + } + + // Called by |handle_watcher_| when data is available or the pipe was closed, + // and there's a pending Read() call. + void OnHandleReadable(MojoResult result) { + if (result != MOJO_RESULT_OK) { + OnFailure(); + return; + } + + // Read. + uint32_t length = remaining_size_; + result = data_pipe_->ReadData(head_, &length, MOJO_READ_DATA_FLAG_NONE); + if (result == MOJO_RESULT_OK) { // success + remaining_size_ -= length; + head_ += length; + if (remaining_size_ == 0) + OnSuccess(); + } else if (result == MOJO_RESULT_SHOULD_WAIT) { // IO pending + handle_watcher_.ArmOrNotify(); + } else { // error + OnFailure(); + } + } + + void OnFailure() { + promise_.RejectWithErrorMessage("Could not get blob data"); + delete this; + } + + void OnSuccess() { + // Pass the buffer to JS. + // + // Note that the lifetime of the native buffer belongs to us, and we will + // free memory when JS buffer gets garbage collected. + v8::Locker locker(promise_.isolate()); + v8::HandleScope handle_scope(promise_.isolate()); + v8::Local buffer = + node::Buffer::New(promise_.isolate(), &buffer_.front(), buffer_.size(), + &DataPipeReader::FreeBuffer, this) + .ToLocalChecked(); + promise_.Resolve(buffer); + + // Destroy data pipe. + handle_watcher_.Cancel(); + data_pipe_.reset(); + data_pipe_getter_ = nullptr; + } + + static void FreeBuffer(char* data, void* self) { + delete static_cast(self); + } + + util::Promise> promise_; + + network::mojom::DataPipeGetterPtr data_pipe_getter_; + mojo::ScopedDataPipeConsumerHandle data_pipe_; + mojo::SimpleWatcher handle_watcher_; + + // Stores read data. + std::vector buffer_; + + // The head of buffer. + char* head_ = nullptr; + + // Remaining data to read. + uint64_t remaining_size_ = 0; + + base::WeakPtrFactory weak_factory_{this}; + + DISALLOW_COPY_AND_ASSIGN(DataPipeReader); +}; + +} // namespace + +gin::WrapperInfo DataPipeHolder::kWrapperInfo = {gin::kEmbedderNativeGin}; + +DataPipeHolder::DataPipeHolder(const network::DataElement& element) + : id_(base::NumberToString(++g_next_id)), + data_pipe_(element.CloneDataPipeGetter()) {} + +DataPipeHolder::~DataPipeHolder() = default; + +v8::Local DataPipeHolder::ReadAll(v8::Isolate* isolate) { + util::Promise> promise(isolate); + v8::Local handle = promise.GetHandle(); + if (!data_pipe_) { + promise.RejectWithErrorMessage("Could not get blob data"); + return handle; + } + + new DataPipeReader(std::move(promise), std::move(data_pipe_)); + return handle; +} + +// static +gin::Handle DataPipeHolder::Create( + v8::Isolate* isolate, + const network::DataElement& element) { + auto handle = gin::CreateHandle(isolate, new DataPipeHolder(element)); + g_weak_map.Set(isolate, handle->id(), + handle->GetWrapper(isolate).ToLocalChecked()); + return handle; +} + +// static +gin::Handle DataPipeHolder::From(v8::Isolate* isolate, + const std::string& id) { + v8::MaybeLocal object = g_weak_map.Get(isolate, id); + if (!object.IsEmpty()) { + gin::Handle handle; + if (gin::ConvertFromV8(isolate, object.ToLocalChecked(), &handle)) + return handle; + } + return gin::Handle(); +} + +} // namespace api + +} // namespace electron diff --git a/shell/browser/api/atom_api_data_pipe_holder.h b/shell/browser/api/atom_api_data_pipe_holder.h new file mode 100644 index 000000000000..991cf7531790 --- /dev/null +++ b/shell/browser/api/atom_api_data_pipe_holder.h @@ -0,0 +1,53 @@ +// Copyright (c) 2019 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef SHELL_BROWSER_API_ATOM_API_DATA_PIPE_HOLDER_H_ +#define SHELL_BROWSER_API_ATOM_API_DATA_PIPE_HOLDER_H_ + +#include + +#include "gin/handle.h" +#include "gin/wrappable.h" +#include "services/network/public/cpp/data_element.h" +#include "services/network/public/mojom/data_pipe_getter.mojom.h" + +namespace electron { + +namespace api { + +// Retains reference to the data pipe. +class DataPipeHolder : public gin::Wrappable { + public: + static gin::WrapperInfo kWrapperInfo; + + static gin::Handle Create( + v8::Isolate* isolate, + const network::DataElement& element); + static gin::Handle From(v8::Isolate* isolate, + const std::string& id); + + // Read all data at once. + // + // TODO(zcbenz): This is apparently not suitable for really large data, but + // no one has complained about it yet. + v8::Local ReadAll(v8::Isolate* isolate); + + // The unique ID that can be used to receive the object. + const std::string& id() const { return id_; } + + private: + explicit DataPipeHolder(const network::DataElement& element); + ~DataPipeHolder() override; + + std::string id_; + network::mojom::DataPipeGetterPtr data_pipe_; + + DISALLOW_COPY_AND_ASSIGN(DataPipeHolder); +}; + +} // namespace api + +} // namespace electron + +#endif // SHELL_BROWSER_API_ATOM_API_DATA_PIPE_HOLDER_H_ diff --git a/shell/browser/api/atom_api_session.cc b/shell/browser/api/atom_api_session.cc index 6898dd337743..e5ebe6c0412a 100644 --- a/shell/browser/api/atom_api_session.cc +++ b/shell/browser/api/atom_api_session.cc @@ -41,6 +41,7 @@ #include "services/network/network_service.h" #include "services/network/public/cpp/features.h" #include "shell/browser/api/atom_api_cookies.h" +#include "shell/browser/api/atom_api_data_pipe_holder.h" #include "shell/browser/api/atom_api_download_item.h" #include "shell/browser/api/atom_api_net_log.h" #include "shell/browser/api/atom_api_protocol_ns.h" @@ -502,15 +503,14 @@ std::string Session::GetUserAgent() { v8::Local Session::GetBlobData(v8::Isolate* isolate, const std::string& uuid) { - util::Promise> promise(isolate); - v8::Local handle = promise.GetHandle(); + gin::Handle holder = DataPipeHolder::From(isolate, uuid); + if (holder.IsEmpty()) { + util::Promise> promise(isolate); + promise.RejectWithErrorMessage("Could not get blob data handle"); + return promise.GetHandle(); + } - AtomBlobReader* blob_reader = browser_context()->GetBlobReader(); - base::PostTaskWithTraits( - FROM_HERE, {BrowserThread::IO}, - base::BindOnce(&AtomBlobReader::StartReading, - base::Unretained(blob_reader), uuid, std::move(promise))); - return handle; + return holder->ReadAll(isolate); } void Session::DownloadURL(const GURL& url) { diff --git a/shell/browser/api/atom_api_session.h b/shell/browser/api/atom_api_session.h index 7cf6548f9329..1e18ba2733a9 100644 --- a/shell/browser/api/atom_api_session.h +++ b/shell/browser/api/atom_api_session.h @@ -13,7 +13,6 @@ #include "electron/buildflags/buildflags.h" #include "native_mate/handle.h" #include "shell/browser/api/trackable_object.h" -#include "shell/browser/atom_blob_reader.h" #include "shell/browser/net/resolve_proxy_helper.h" #include "shell/common/promise_util.h" diff --git a/shell/browser/atom_blob_reader.cc b/shell/browser/atom_blob_reader.cc deleted file mode 100644 index cb5525d145be..000000000000 --- a/shell/browser/atom_blob_reader.cc +++ /dev/null @@ -1,126 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#include "shell/browser/atom_blob_reader.h" - -#include - -#include "base/task/post_task.h" -#include "content/browser/blob_storage/chrome_blob_storage_context.h" // nogncheck -#include "content/public/browser/browser_task_traits.h" -#include "content/public/browser/browser_thread.h" -#include "net/base/io_buffer.h" -#include "net/base/net_errors.h" -#include "shell/common/node_includes.h" -#include "storage/browser/blob/blob_data_handle.h" -#include "storage/browser/blob/blob_reader.h" -#include "storage/browser/blob/blob_storage_context.h" - -using content::BrowserThread; - -namespace electron { - -namespace { - -void FreeNodeBufferData(char* data, void* hint) { - delete[] data; -} - -void RunPromiseInUI(util::Promise> promise, - char* blob_data, - int size) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - v8::Isolate* isolate = promise.isolate(); - - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - if (blob_data) { - v8::Local buffer = - node::Buffer::New(isolate, blob_data, static_cast(size), - &FreeNodeBufferData, nullptr) - .ToLocalChecked(); - promise.Resolve(buffer); - } else { - promise.RejectWithErrorMessage("Could not get blob data"); - } -} - -} // namespace - -AtomBlobReader::AtomBlobReader(content::ChromeBlobStorageContext* blob_context) - : blob_context_(blob_context) {} - -AtomBlobReader::~AtomBlobReader() {} - -void AtomBlobReader::StartReading(const std::string& uuid, - util::Promise> promise) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - - auto blob_data_handle = blob_context_->context()->GetBlobDataFromUUID(uuid); - if (!blob_data_handle) { - util::Promise>::RejectPromise( - std::move(promise), "Could not get blob data handle"); - return; - } - - auto blob_reader = blob_data_handle->CreateReader(); - BlobReadHelper* blob_read_helper = - new BlobReadHelper(std::move(blob_reader), - base::BindOnce(&RunPromiseInUI, std::move(promise))); - blob_read_helper->Read(); -} - -AtomBlobReader::BlobReadHelper::BlobReadHelper( - std::unique_ptr blob_reader, - BlobReadHelper::CompletionCallback callback) - : blob_reader_(std::move(blob_reader)), - completion_callback_(std::move(callback)) {} - -AtomBlobReader::BlobReadHelper::~BlobReadHelper() {} - -void AtomBlobReader::BlobReadHelper::Read() { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - - storage::BlobReader::Status size_status = blob_reader_->CalculateSize( - base::BindOnce(&AtomBlobReader::BlobReadHelper::DidCalculateSize, - base::Unretained(this))); - if (size_status != storage::BlobReader::Status::IO_PENDING) - DidCalculateSize(net::OK); -} - -void AtomBlobReader::BlobReadHelper::DidCalculateSize(int result) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - - if (result != net::OK) { - DidReadBlobData(nullptr, 0); - return; - } - - uint64_t total_size = blob_reader_->total_size(); - int bytes_read = 0; - scoped_refptr blob_data = - new net::IOBuffer(static_cast(total_size)); - auto callback = - base::BindRepeating(&AtomBlobReader::BlobReadHelper::DidReadBlobData, - base::Unretained(this), base::RetainedRef(blob_data)); - storage::BlobReader::Status read_status = - blob_reader_->Read(blob_data.get(), total_size, &bytes_read, callback); - if (read_status != storage::BlobReader::Status::IO_PENDING) - callback.Run(bytes_read); -} - -void AtomBlobReader::BlobReadHelper::DidReadBlobData( - const scoped_refptr& blob_data, - int size) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - - char* data = new char[size]; - memcpy(data, blob_data->data(), size); - base::PostTaskWithTraits( - FROM_HERE, {BrowserThread::UI}, - base::BindOnce(std::move(completion_callback_), data, size)); - delete this; -} - -} // namespace electron diff --git a/shell/browser/atom_blob_reader.h b/shell/browser/atom_blob_reader.h deleted file mode 100644 index 06e792b1e140..000000000000 --- a/shell/browser/atom_blob_reader.h +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#ifndef SHELL_BROWSER_ATOM_BLOB_READER_H_ -#define SHELL_BROWSER_ATOM_BLOB_READER_H_ - -#include -#include - -#include "base/callback.h" -#include "shell/common/promise_util.h" - -namespace content { -class ChromeBlobStorageContext; -} - -namespace net { -class IOBuffer; -} - -namespace storage { -class BlobDataHandle; -class BlobReader; -} // namespace storage - -namespace v8 { -template -class Local; -class Value; -} // namespace v8 - -namespace electron { - -// A class to keep track of the blob context. All methods, -// except Ctor are expected to be called on IO thread. -class AtomBlobReader { - public: - explicit AtomBlobReader(content::ChromeBlobStorageContext* blob_context); - ~AtomBlobReader(); - - void StartReading(const std::string& uuid, - electron::util::Promise> promise); - - private: - // A self-destroyed helper class to read the blob data. - // Must be accessed on IO thread. - class BlobReadHelper { - public: - using CompletionCallback = base::OnceCallback; - - BlobReadHelper(std::unique_ptr blob_reader, - BlobReadHelper::CompletionCallback callback); - ~BlobReadHelper(); - - void Read(); - - private: - void DidCalculateSize(int result); - void DidReadBlobData(const scoped_refptr& blob_data, - int bytes_read); - - std::unique_ptr blob_reader_; - BlobReadHelper::CompletionCallback completion_callback_; - - DISALLOW_COPY_AND_ASSIGN(BlobReadHelper); - }; - - scoped_refptr blob_context_; - - DISALLOW_COPY_AND_ASSIGN(AtomBlobReader); -}; - -} // namespace electron - -#endif // SHELL_BROWSER_ATOM_BLOB_READER_H_ diff --git a/shell/browser/atom_browser_context.cc b/shell/browser/atom_browser_context.cc index a9827d4f009f..beea5cd2b877 100644 --- a/shell/browser/atom_browser_context.cc +++ b/shell/browser/atom_browser_context.cc @@ -29,7 +29,6 @@ #include "net/base/escape.h" #include "services/network/public/cpp/features.h" #include "services/network/public/cpp/wrapper_shared_url_loader_factory.h" -#include "shell/browser/atom_blob_reader.h" #include "shell/browser/atom_browser_client.h" #include "shell/browser/atom_browser_main_parts.h" #include "shell/browser/atom_download_manager_delegate.h" @@ -256,15 +255,6 @@ std::string AtomBrowserContext::GetUserAgent() const { return user_agent_; } -AtomBlobReader* AtomBrowserContext::GetBlobReader() { - if (!blob_reader_.get()) { - content::ChromeBlobStorageContext* blob_context = - content::ChromeBlobStorageContext::GetFor(this); - blob_reader_.reset(new AtomBlobReader(blob_context)); - } - return blob_reader_.get(); -} - predictors::PreconnectManager* AtomBrowserContext::GetPreconnectManager() { if (!preconnect_manager_.get()) { preconnect_manager_.reset(new predictors::PreconnectManager(nullptr, this)); diff --git a/shell/browser/atom_browser_context.h b/shell/browser/atom_browser_context.h index e8d41d315bf5..685d3a147fc8 100644 --- a/shell/browser/atom_browser_context.h +++ b/shell/browser/atom_browser_context.h @@ -40,7 +40,6 @@ class AtomExtensionSystem; namespace electron { -class AtomBlobReader; class AtomBrowserContext; class AtomDownloadManagerDelegate; class AtomPermissionManager; @@ -90,7 +89,6 @@ class AtomBrowserContext std::string GetUserAgent() const; bool CanUseHttpCache() const; int GetMaxCacheSize() const; - AtomBlobReader* GetBlobReader(); ResolveProxyHelper* GetResolveProxyHelper(); predictors::PreconnectManager* GetPreconnectManager(); scoped_refptr GetURLLoaderFactory(); @@ -163,7 +161,6 @@ class AtomBrowserContext std::unique_ptr download_manager_delegate_; std::unique_ptr guest_manager_; std::unique_ptr permission_manager_; - std::unique_ptr blob_reader_; std::unique_ptr media_device_id_salt_; scoped_refptr resolve_proxy_helper_; scoped_refptr storage_policy_; diff --git a/shell/common/gin_converters/net_converter.cc b/shell/common/gin_converters/net_converter.cc index 1491fabe545c..1f4ec7bfe8e9 100644 --- a/shell/common/gin_converters/net_converter.cc +++ b/shell/common/gin_converters/net_converter.cc @@ -22,6 +22,7 @@ #include "net/cert/x509_util.h" #include "net/http/http_response_headers.h" #include "services/network/public/cpp/resource_request.h" +#include "shell/browser/api/atom_api_data_pipe_holder.h" #include "shell/common/gin_converters/gurl_converter.h" #include "shell/common/gin_converters/std_converter.h" #include "shell/common/gin_converters/value_converter_gin_adapter.h" @@ -264,9 +265,16 @@ v8::Local Converter::ToV8( element.length()) .ToLocalChecked()); break; - case network::mojom::DataElementType::kBlob: - upload_data.Set("blobUUID", element.blob_uuid()); + case network::mojom::DataElementType::kDataPipe: { + // TODO(zcbenz): After the NetworkService refactor, the old blobUUID API + // becomes unecessarily complex, we should deprecate the getBlobData API + // and return the DataPipeHolder wrapper directly. + auto holder = electron::api::DataPipeHolder::Create(isolate, element); + upload_data.Set("blobUUID", holder->id()); + // The lifetime of data pipe is bound to the uploadData object. + upload_data.Set("dataPipe", holder); break; + } default: NOTREACHED() << "Found unsupported data element"; } diff --git a/spec-main/api-session-spec.ts b/spec-main/api-session-spec.ts index 95884c0468f3..b5b25eab313f 100644 --- a/spec-main/api-session-spec.ts +++ b/spec-main/api-session-spec.ts @@ -368,7 +368,7 @@ describe('session module', () => { }) }) - describe.skip('ses.getBlobData()', () => { + describe('ses.getBlobData()', () => { const scheme = 'cors-blob' const protocol = session.defaultSession.protocol const url = `${scheme}://host`