From 6e5dd735f6c8daabd5920c22a3fcb67aad7b88bc Mon Sep 17 00:00:00 2001 From: Robo Date: Sat, 6 Oct 2018 01:59:57 +0530 Subject: [PATCH] refactor: enable weak ptr unwrap sequence dcheck (#14816) * refactor: enable weak ptr unwrap sequence dcheck * spec: remove WeakPtrDeathTest.* from disabled list --- atom/browser/api/stream_subscriber.cc | 3 +- .../browser/net/asar/asar_protocol_handler.cc | 1 + atom/browser/net/js_asker.cc | 57 +++----- atom/browser/net/js_asker.h | 103 +++----------- .../browser/net/url_request_async_asar_job.cc | 60 +++++++- atom/browser/net/url_request_async_asar_job.h | 10 +- atom/browser/net/url_request_buffer_job.cc | 57 +++++++- atom/browser/net/url_request_buffer_job.h | 9 +- atom/browser/net/url_request_fetch_job.cc | 119 +++++++++++----- atom/browser/net/url_request_fetch_job.h | 22 +-- atom/browser/net/url_request_stream_job.cc | 128 +++++++++++++----- atom/browser/net/url_request_stream_job.h | 30 ++-- atom/browser/net/url_request_string_job.cc | 59 +++++++- atom/browser/net/url_request_string_job.h | 9 +- patches/common/chromium/.patches.yaml | 1 - patches/common/chromium/dcheck.patch | 15 -- spec/configs/unittests.yml | 6 - 17 files changed, 431 insertions(+), 258 deletions(-) diff --git a/atom/browser/api/stream_subscriber.cc b/atom/browser/api/stream_subscriber.cc index ec2b924a4aa9..431dfcbe776a 100644 --- a/atom/browser/api/stream_subscriber.cc +++ b/atom/browser/api/stream_subscriber.cc @@ -91,7 +91,8 @@ void StreamSubscriber::OnEnd(mate::Arguments* args) { void StreamSubscriber::OnError(mate::Arguments* args) { content::BrowserThread::PostTask( content::BrowserThread::IO, FROM_HERE, - base::Bind(&atom::URLRequestStreamJob::OnError, url_job_)); + base::Bind(&atom::URLRequestStreamJob::OnError, url_job_, + net::ERR_FAILED)); } void StreamSubscriber::RemoveAllListeners() { diff --git a/atom/browser/net/asar/asar_protocol_handler.cc b/atom/browser/net/asar/asar_protocol_handler.cc index c2f53fa9d108..8ff9b475ae25 100644 --- a/atom/browser/net/asar/asar_protocol_handler.cc +++ b/atom/browser/net/asar/asar_protocol_handler.cc @@ -5,6 +5,7 @@ #include "atom/browser/net/asar/asar_protocol_handler.h" #include "atom/browser/net/asar/url_request_asar_job.h" +#include "base/task_runner.h" #include "net/base/filename_util.h" #include "net/base/net_errors.h" diff --git a/atom/browser/net/js_asker.cc b/atom/browser/net/js_asker.cc index c64c3be139ac..1e3511fc5f60 100644 --- a/atom/browser/net/js_asker.cc +++ b/atom/browser/net/js_asker.cc @@ -4,61 +4,42 @@ #include "atom/browser/net/js_asker.h" -#include #include -#include #include "atom/common/native_mate_converters/callback.h" -#include "atom/common/native_mate_converters/v8_value_converter.h" namespace atom { -namespace internal { +JsAsker::JsAsker() = default; -namespace { +JsAsker::~JsAsker() = default; -// The callback which is passed to |handler|. -void HandlerCallback(const BeforeStartCallback& before_start, - const ResponseCallback& callback, - mate::Arguments* args) { - // If there is no argument passed then we failed. - v8::Local value; - if (!args->GetNext(&value)) { - content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, - base::BindOnce(callback, false, nullptr)); - return; - } - - // Give the job a chance to parse V8 value. - before_start.Run(args->isolate(), value); - - // Pass whatever user passed to the actaul request job. - V8ValueConverter converter; - v8::Local context = args->isolate()->GetCurrentContext(); - std::unique_ptr options(converter.FromV8Value(value, context)); - content::BrowserThread::PostTask( - content::BrowserThread::IO, FROM_HERE, - base::BindOnce(callback, true, std::move(options))); +void JsAsker::SetHandlerInfo( + v8::Isolate* isolate, + net::URLRequestContextGetter* request_context_getter, + const JavaScriptHandler& handler) { + isolate_ = isolate; + request_context_getter_ = request_context_getter; + handler_ = handler; } -} // namespace - -void AskForOptions(v8::Isolate* isolate, - const JavaScriptHandler& handler, - std::unique_ptr request_details, - const BeforeStartCallback& before_start, - const ResponseCallback& callback) { +// static +void JsAsker::AskForOptions( + v8::Isolate* isolate, + const JavaScriptHandler& handler, + std::unique_ptr request_details, + const BeforeStartCallback& before_start) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); v8::Locker locker(isolate); v8::HandleScope handle_scope(isolate); v8::Local context = isolate->GetCurrentContext(); v8::Context::Scope context_scope(context); handler.Run(*(request_details.get()), - mate::ConvertToV8(isolate, base::Bind(&HandlerCallback, - before_start, callback))); + mate::ConvertToV8(isolate, before_start)); } -bool IsErrorOptions(base::Value* value, int* error) { +// static +bool JsAsker::IsErrorOptions(base::Value* value, int* error) { if (value->is_dict()) { base::DictionaryValue* dict = static_cast(value); if (dict->GetInteger("error", error)) @@ -70,6 +51,4 @@ bool IsErrorOptions(base::Value* value, int* error) { return false; } -} // namespace internal - } // namespace atom diff --git a/atom/browser/net/js_asker.h b/atom/browser/net/js_asker.h index 5a8139cfa25b..8c76eab51697 100644 --- a/atom/browser/net/js_asker.h +++ b/atom/browser/net/js_asker.h @@ -6,118 +6,49 @@ #define ATOM_BROWSER_NET_JS_ASKER_H_ #include -#include -#include "atom/common/native_mate_converters/net_converter.h" #include "base/callback.h" -#include "base/memory/ref_counted.h" -#include "base/memory/weak_ptr.h" #include "base/values.h" -#include "content/public/browser/browser_thread.h" -#include "net/base/net_errors.h" -#include "net/http/http_response_headers.h" -#include "net/http/http_status_code.h" +#include "native_mate/arguments.h" #include "net/url_request/url_request_context_getter.h" -#include "net/url_request/url_request_job.h" #include "v8/include/v8.h" namespace atom { using JavaScriptHandler = base::Callback)>; +using BeforeStartCallback = base::Callback; -namespace internal { - -using BeforeStartCallback = - base::Callback)>; -using ResponseCallback = - base::Callback options)>; - -// Ask handler for options in UI thread. -void AskForOptions(v8::Isolate* isolate, - const JavaScriptHandler& handler, - std::unique_ptr request_details, - const BeforeStartCallback& before_start, - const ResponseCallback& callback); - -// Test whether the |options| means an error. -bool IsErrorOptions(base::Value* value, int* error); - -} // namespace internal - -template -class JsAsker : public RequestJob { +class JsAsker { public: - JsAsker(net::URLRequest* request, net::NetworkDelegate* network_delegate) - : RequestJob(request, network_delegate), weak_factory_(this) {} + JsAsker(); + ~JsAsker(); // Called by |CustomProtocolHandler| to store handler related information. void SetHandlerInfo(v8::Isolate* isolate, net::URLRequestContextGetter* request_context_getter, - const JavaScriptHandler& handler) { - isolate_ = isolate; - request_context_getter_ = request_context_getter; - handler_ = handler; - } + const JavaScriptHandler& handler); - // Subclass should do initailze work here. - virtual void BeforeStartInUI(v8::Isolate*, v8::Local) {} - virtual void StartAsync(std::unique_ptr options) = 0; + // Ask handler for options in UI thread. + static void AskForOptions( + v8::Isolate* isolate, + const JavaScriptHandler& handler, + std::unique_ptr request_details, + const BeforeStartCallback& before_start); + + // Test whether the |options| means an error. + static bool IsErrorOptions(base::Value* value, int* error); net::URLRequestContextGetter* request_context_getter() const { return request_context_getter_; } + v8::Isolate* isolate() { return isolate_; } + JavaScriptHandler handler() { return handler_; } private: - // RequestJob: - void Start() override { - auto request_details = std::make_unique(); - request_start_time_ = base::TimeTicks::Now(); - FillRequestDetails(request_details.get(), RequestJob::request()); - content::BrowserThread::PostTask( - content::BrowserThread::UI, FROM_HERE, - base::BindOnce( - &internal::AskForOptions, isolate_, handler_, - std::move(request_details), - base::Bind(&JsAsker::BeforeStartInUI, weak_factory_.GetWeakPtr()), - base::Bind(&JsAsker::OnResponse, weak_factory_.GetWeakPtr()))); - } - - int GetResponseCode() const override { return net::HTTP_OK; } - - // NOTE: We have to implement this method or risk a crash in blink for - // redirects! - void GetLoadTimingInfo(net::LoadTimingInfo* load_timing_info) const override { - load_timing_info->send_start = request_start_time_; - load_timing_info->send_end = request_start_time_; - load_timing_info->request_start = request_start_time_; - load_timing_info->receive_headers_end = response_start_time_; - } - - void GetResponseInfo(net::HttpResponseInfo* info) override { - info->headers = new net::HttpResponseHeaders(""); - } - - // Called when the JS handler has sent the response, we need to decide whether - // to start, or fail the job. - void OnResponse(bool success, std::unique_ptr value) { - response_start_time_ = base::TimeTicks::Now(); - int error = net::ERR_NOT_IMPLEMENTED; - if (success && value && !internal::IsErrorOptions(value.get(), &error)) { - StartAsync(std::move(value)); - } else { - RequestJob::NotifyStartError( - net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); - } - } - v8::Isolate* isolate_; net::URLRequestContextGetter* request_context_getter_; JavaScriptHandler handler_; - base::TimeTicks request_start_time_; - base::TimeTicks response_start_time_; - - base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(JsAsker); }; diff --git a/atom/browser/net/url_request_async_asar_job.cc b/atom/browser/net/url_request_async_asar_job.cc index 531d9ea8851b..646ac9ac50e9 100644 --- a/atom/browser/net/url_request_async_asar_job.cc +++ b/atom/browser/net/url_request_async_asar_job.cc @@ -6,19 +6,70 @@ #include #include +#include #include "atom/common/atom_constants.h" +#include "atom/common/native_mate_converters/net_converter.h" +#include "atom/common/native_mate_converters/v8_value_converter.h" #include "base/strings/utf_string_conversions.h" #include "base/task_scheduler/post_task.h" +#include "content/public/browser/browser_thread.h" namespace atom { +namespace { + +void BeforeStartInUI(base::WeakPtr job, + mate::Arguments* args) { + v8::Local value; + int error = net::OK; + std::unique_ptr request_options = nullptr; + + if (args->GetNext(&value)) { + V8ValueConverter converter; + v8::Local context = args->isolate()->GetCurrentContext(); + request_options.reset(converter.FromV8Value(value, context)); + } + + if (request_options) { + JsAsker::IsErrorOptions(request_options.get(), &error); + } else { + error = net::ERR_NOT_IMPLEMENTED; + } + + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestAsyncAsarJob::StartAsync, job, + std::move(request_options), error)); +} + +} // namespace + URLRequestAsyncAsarJob::URLRequestAsyncAsarJob( net::URLRequest* request, net::NetworkDelegate* network_delegate) - : JsAsker(request, network_delegate) {} + : asar::URLRequestAsarJob(request, network_delegate), weak_factory_(this) {} + +URLRequestAsyncAsarJob::~URLRequestAsyncAsarJob() = default; + +void URLRequestAsyncAsarJob::Start() { + auto request_details = std::make_unique(); + FillRequestDetails(request_details.get(), request()); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::BindOnce(&JsAsker::AskForOptions, base::Unretained(isolate()), + handler(), std::move(request_details), + base::Bind(&BeforeStartInUI, weak_factory_.GetWeakPtr()))); +} + +void URLRequestAsyncAsarJob::StartAsync(std::unique_ptr options, + int error) { + if (error != net::OK) { + NotifyStartError( + net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); + return; + } -void URLRequestAsyncAsarJob::StartAsync(std::unique_ptr options) { std::string file_path; if (options->is_dict()) { auto* path_value = @@ -46,6 +97,11 @@ void URLRequestAsyncAsarJob::StartAsync(std::unique_ptr options) { } } +void URLRequestAsyncAsarJob::Kill() { + weak_factory_.InvalidateWeakPtrs(); + URLRequestAsarJob::Kill(); +} + void URLRequestAsyncAsarJob::GetResponseInfo(net::HttpResponseInfo* info) { std::string status("HTTP/1.1 200 OK"); auto* headers = new net::HttpResponseHeaders(status); diff --git a/atom/browser/net/url_request_async_asar_job.h b/atom/browser/net/url_request_async_asar_job.h index 2cf70d04e911..f97dcc1570ea 100644 --- a/atom/browser/net/url_request_async_asar_job.h +++ b/atom/browser/net/url_request_async_asar_job.h @@ -13,17 +13,21 @@ namespace atom { // Like URLRequestAsarJob, but asks the JavaScript handler for file path. -class URLRequestAsyncAsarJob : public JsAsker { +class URLRequestAsyncAsarJob : public asar::URLRequestAsarJob, public JsAsker { public: URLRequestAsyncAsarJob(net::URLRequest*, net::NetworkDelegate*); + ~URLRequestAsyncAsarJob() override; - // JsAsker: - void StartAsync(std::unique_ptr options) override; + void StartAsync(std::unique_ptr options, int error); // URLRequestJob: + void Start() override; void GetResponseInfo(net::HttpResponseInfo* info) override; + void Kill() override; private: + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(URLRequestAsyncAsarJob); }; diff --git a/atom/browser/net/url_request_buffer_job.cc b/atom/browser/net/url_request_buffer_job.cc index 974619a0f815..e896a913067f 100644 --- a/atom/browser/net/url_request_buffer_job.cc +++ b/atom/browser/net/url_request_buffer_job.cc @@ -6,10 +6,14 @@ #include #include +#include #include "atom/common/atom_constants.h" +#include "atom/common/native_mate_converters/net_converter.h" +#include "atom/common/native_mate_converters/v8_value_converter.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" +#include "content/public/browser/browser_thread.h" #include "net/base/mime_util.h" #include "net/base/net_errors.h" @@ -25,16 +29,58 @@ std::string GetExtFromURL(const GURL& url) { return spec.substr(index + 1, spec.size() - index - 1); } +void BeforeStartInUI(base::WeakPtr job, + mate::Arguments* args) { + v8::Local value; + int error = net::OK; + std::unique_ptr request_options = nullptr; + + if (args->GetNext(&value)) { + V8ValueConverter converter; + v8::Local context = args->isolate()->GetCurrentContext(); + request_options.reset(converter.FromV8Value(value, context)); + } + + if (request_options) { + JsAsker::IsErrorOptions(request_options.get(), &error); + } else { + error = net::ERR_NOT_IMPLEMENTED; + } + + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestBufferJob::StartAsync, job, + std::move(request_options), error)); +} + } // namespace URLRequestBufferJob::URLRequestBufferJob(net::URLRequest* request, net::NetworkDelegate* network_delegate) - : JsAsker(request, network_delegate), - status_code_(net::HTTP_NOT_IMPLEMENTED) {} + : net::URLRequestSimpleJob(request, network_delegate), + status_code_(net::HTTP_NOT_IMPLEMENTED), + weak_factory_(this) {} URLRequestBufferJob::~URLRequestBufferJob() = default; -void URLRequestBufferJob::StartAsync(std::unique_ptr options) { +void URLRequestBufferJob::Start() { + auto request_details = std::make_unique(); + FillRequestDetails(request_details.get(), request()); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::BindOnce(&JsAsker::AskForOptions, base::Unretained(isolate()), + handler(), std::move(request_details), + base::Bind(&BeforeStartInUI, weak_factory_.GetWeakPtr()))); +} + +void URLRequestBufferJob::StartAsync(std::unique_ptr options, + int error) { + if (error != net::OK) { + NotifyStartError( + net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); + return; + } + const base::Value* binary = nullptr; if (options->is_dict()) { base::DictionaryValue* dict = @@ -68,6 +114,11 @@ void URLRequestBufferJob::StartAsync(std::unique_ptr options) { net::URLRequestSimpleJob::Start(); } +void URLRequestBufferJob::Kill() { + weak_factory_.InvalidateWeakPtrs(); + net::URLRequestSimpleJob::Kill(); +} + void URLRequestBufferJob::GetResponseInfo(net::HttpResponseInfo* info) { std::string status("HTTP/1.1 200 OK"); status.append(base::IntToString(status_code_)); diff --git a/atom/browser/net/url_request_buffer_job.h b/atom/browser/net/url_request_buffer_job.h index 69474753baa0..25ca13b31078 100644 --- a/atom/browser/net/url_request_buffer_job.h +++ b/atom/browser/net/url_request_buffer_job.h @@ -15,16 +15,17 @@ namespace atom { -class URLRequestBufferJob : public JsAsker { +class URLRequestBufferJob : public JsAsker, public net::URLRequestSimpleJob { public: URLRequestBufferJob(net::URLRequest*, net::NetworkDelegate*); ~URLRequestBufferJob() override; - // JsAsker: - void StartAsync(std::unique_ptr options) override; + void StartAsync(std::unique_ptr options, int error); // URLRequestJob: + void Start() override; void GetResponseInfo(net::HttpResponseInfo* info) override; + void Kill() override; // URLRequestSimpleJob: int GetRefCountedData(std::string* mime_type, @@ -38,6 +39,8 @@ class URLRequestBufferJob : public JsAsker { scoped_refptr data_; net::HttpStatusCode status_code_; + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(URLRequestBufferJob); }; diff --git a/atom/browser/net/url_request_fetch_job.cc b/atom/browser/net/url_request_fetch_job.cc index cd6a8726e034..52a5f7359747 100644 --- a/atom/browser/net/url_request_fetch_job.cc +++ b/atom/browser/net/url_request_fetch_job.cc @@ -7,12 +7,16 @@ #include #include #include +#include #include "atom/browser/api/atom_api_session.h" #include "atom/browser/atom_browser_context.h" +#include "atom/common/native_mate_converters/net_converter.h" +#include "atom/common/native_mate_converters/v8_value_converter.h" #include "base/guid.h" #include "base/memory/ptr_util.h" #include "base/strings/string_util.h" +#include "content/public/browser/browser_thread.h" #include "native_mate/dictionary.h" #include "net/base/io_buffer.h" #include "net/base/net_errors.h" @@ -76,43 +80,85 @@ class ResponsePiper : public net::URLFetcherResponseWriter { DISALLOW_COPY_AND_ASSIGN(ResponsePiper); }; +void BeforeStartInUI(base::WeakPtr job, + mate::Arguments* args) { + // Pass whatever user passed to the actaul request job. + v8::Local value; + mate::Dictionary options; + if (!args->GetNext(&value) || + !mate::ConvertFromV8(args->isolate(), value, &options)) { + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestFetchJob::OnError, job, + net::ERR_NOT_IMPLEMENTED)); + return; + } + + scoped_refptr url_request_context_getter; + scoped_refptr custom_browser_context; + // When |session| is set to |null| we use a new request context for fetch + // job. + if (options.Get("session", &value)) { + if (value->IsNull()) { + // We have to create the URLRequestContextGetter on UI thread. + custom_browser_context = + AtomBrowserContext::From(base::GenerateGUID(), true); + url_request_context_getter = custom_browser_context->GetRequestContext(); + } else { + mate::Handle session; + if (mate::ConvertFromV8(args->isolate(), value, &session) && + !session.IsEmpty()) { + AtomBrowserContext* browser_context = session->browser_context(); + url_request_context_getter = browser_context->GetRequestContext(); + } + } + } + + V8ValueConverter converter; + v8::Local context = args->isolate()->GetCurrentContext(); + std::unique_ptr request_options( + converter.FromV8Value(value, context)); + + int error = net::OK; + if (!request_options || !request_options->is_dict()) + error = net::ERR_NOT_IMPLEMENTED; + + JsAsker::IsErrorOptions(request_options.get(), &error); + + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestFetchJob::StartAsync, job, + base::RetainedRef(url_request_context_getter), + base::RetainedRef(custom_browser_context), + std::move(request_options), error)); +} + } // namespace URLRequestFetchJob::URLRequestFetchJob(net::URLRequest* request, net::NetworkDelegate* network_delegate) - : JsAsker(request, network_delegate) {} + : net::URLRequestJob(request, network_delegate), weak_factory_(this) {} URLRequestFetchJob::~URLRequestFetchJob() = default; -void URLRequestFetchJob::BeforeStartInUI(v8::Isolate* isolate, - v8::Local value) { - mate::Dictionary options; - if (!mate::ConvertFromV8(isolate, value, &options)) - return; - - // When |session| is set to |null| we use a new request context for fetch job. - v8::Local val; - if (options.Get("session", &val)) { - if (val->IsNull()) { - // We have to create the URLRequestContextGetter on UI thread. - custom_browser_context_ = - AtomBrowserContext::From(base::GenerateGUID(), true); - url_request_context_getter_ = - custom_browser_context_->GetRequestContext(); - } else { - mate::Handle session; - if (mate::ConvertFromV8(isolate, val, &session) && !session.IsEmpty()) { - AtomBrowserContext* browser_context = session->browser_context(); - url_request_context_getter_ = browser_context->GetRequestContext(); - } - } - } +void URLRequestFetchJob::Start() { + auto request_details = std::make_unique(); + FillRequestDetails(request_details.get(), request()); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::BindOnce(&JsAsker::AskForOptions, base::Unretained(isolate()), + handler(), std::move(request_details), + base::Bind(&BeforeStartInUI, weak_factory_.GetWeakPtr()))); } -void URLRequestFetchJob::StartAsync(std::unique_ptr options) { - if (!options->is_dict()) { - NotifyStartError(net::URLRequestStatus(net::URLRequestStatus::FAILED, - net::ERR_NOT_IMPLEMENTED)); +void URLRequestFetchJob::StartAsync( + scoped_refptr url_request_context_getter, + scoped_refptr browser_context, + std::unique_ptr options, + int error) { + if (error != net::OK) { + NotifyStartError( + net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); return; } @@ -144,8 +190,8 @@ void URLRequestFetchJob::StartAsync(std::unique_ptr options) { fetcher_->SaveResponseWithWriter(base::WrapUnique(new ResponsePiper(this))); // A request context getter is passed by the user. - if (url_request_context_getter_) - fetcher_->SetRequestContext(url_request_context_getter_.get()); + if (url_request_context_getter) + fetcher_->SetRequestContext(url_request_context_getter.get()); else fetcher_->SetRequestContext(request_context_getter()); @@ -168,9 +214,13 @@ void URLRequestFetchJob::StartAsync(std::unique_ptr options) { request()->extra_request_headers().ToString()); fetcher_->Start(); - // URLFetcher has a refernce to the context, which - // will be cleared when the request is destroyed. - url_request_context_getter_ = nullptr; + + if (browser_context) + custom_browser_context_ = browser_context; +} + +void URLRequestFetchJob::OnError(int error) { + NotifyStartError(net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); } void URLRequestFetchJob::HeadersCompleted() { @@ -201,7 +251,8 @@ int URLRequestFetchJob::DataAvailable(net::IOBuffer* buffer, } void URLRequestFetchJob::Kill() { - JsAsker::Kill(); + weak_factory_.InvalidateWeakPtrs(); + net::URLRequestJob::Kill(); fetcher_.reset(); custom_browser_context_ = nullptr; } diff --git a/atom/browser/net/url_request_fetch_job.h b/atom/browser/net/url_request_fetch_job.h index 62af6178fa48..9966b3fc80b7 100644 --- a/atom/browser/net/url_request_fetch_job.h +++ b/atom/browser/net/url_request_fetch_job.h @@ -9,21 +9,29 @@ #include #include "atom/browser/net/js_asker.h" -#include "content/browser/streams/stream.h" -#include "content/browser/streams/stream_read_observer.h" +#include "base/memory/weak_ptr.h" #include "net/url_request/url_fetcher_delegate.h" #include "net/url_request/url_request_context_getter.h" +#include "net/url_request/url_request_job.h" namespace atom { class AtomBrowserContext; -class URLRequestFetchJob : public JsAsker, +class URLRequestFetchJob : public JsAsker, + public net::URLRequestJob, public net::URLFetcherDelegate { public: URLRequestFetchJob(net::URLRequest*, net::NetworkDelegate*); ~URLRequestFetchJob() override; + void StartAsync( + scoped_refptr request_context_getter, + scoped_refptr browser_context, + std::unique_ptr options, + int error); + void OnError(int error); + // Called by response writer. void HeadersCompleted(); int DataAvailable(net::IOBuffer* buffer, @@ -31,11 +39,8 @@ class URLRequestFetchJob : public JsAsker, const net::CompletionCallback& callback); protected: - // JsAsker: - void BeforeStartInUI(v8::Isolate*, v8::Local) override; - void StartAsync(std::unique_ptr options) override; - // net::URLRequestJob: + void Start() override; void Kill() override; int ReadRawData(net::IOBuffer* buf, int buf_size) override; bool GetMimeType(std::string* mime_type) const override; @@ -54,7 +59,6 @@ class URLRequestFetchJob : public JsAsker, void ClearWriteBuffer(); scoped_refptr custom_browser_context_; - scoped_refptr url_request_context_getter_; std::unique_ptr fetcher_; std::unique_ptr response_info_; @@ -67,6 +71,8 @@ class URLRequestFetchJob : public JsAsker, int write_num_bytes_ = 0; net::CompletionCallback write_callback_; + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(URLRequestFetchJob); }; diff --git a/atom/browser/net/url_request_stream_job.cc b/atom/browser/net/url_request_stream_job.cc index 4761c68835f2..8dcb22283e82 100644 --- a/atom/browser/net/url_request_stream_job.cc +++ b/atom/browser/net/url_request_stream_job.cc @@ -17,38 +17,28 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" #include "base/time/time.h" +#include "native_mate/dictionary.h" #include "net/base/net_errors.h" #include "net/filter/gzip_source_stream.h" namespace atom { -URLRequestStreamJob::URLRequestStreamJob(net::URLRequest* request, - net::NetworkDelegate* network_delegate) - : JsAsker(request, network_delegate), - pending_buf_(nullptr), - pending_buf_size_(0), - ended_(false), - has_error_(false), - response_headers_(nullptr), - weak_factory_(this) {} +namespace { -URLRequestStreamJob::~URLRequestStreamJob() { - if (subscriber_) { - content::BrowserThread::DeleteSoon(content::BrowserThread::UI, FROM_HERE, - std::move(subscriber_)); - } -} - -void URLRequestStreamJob::BeforeStartInUI(v8::Isolate* isolate, - v8::Local value) { - if (value->IsNull() || value->IsUndefined() || !value->IsObject()) { +void BeforeStartInUI(base::WeakPtr job, + mate::Arguments* args) { + v8::Local value; + int error = net::OK; + bool ended = false; + if (!args->GetNext(&value) || !value->IsObject()) { // Invalid opts. - ended_ = true; - has_error_ = true; + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestStreamJob::OnError, job, net::ERR_FAILED)); return; } - mate::Dictionary opts(isolate, v8::Local::Cast(value)); + mate::Dictionary opts(args->isolate(), v8::Local::Cast(value)); int status_code; if (!opts.Get("statusCode", &status_code)) { // assume HTTP OK if statusCode is not passed. @@ -60,11 +50,12 @@ void URLRequestStreamJob::BeforeStartInUI(v8::Isolate* isolate, status.append( net::GetHttpReasonPhrase(static_cast(status_code))); status.append("\0\0", 2); - response_headers_ = new net::HttpResponseHeaders(status); + scoped_refptr response_headers( + new net::HttpResponseHeaders(status)); if (opts.Get("headers", &value)) { - mate::Converter::FromV8(isolate, value, - response_headers_.get()); + mate::Converter::FromV8(args->isolate(), value, + response_headers.get()); } if (!opts.Get("data", &value)) { @@ -73,28 +64,77 @@ void URLRequestStreamJob::BeforeStartInUI(v8::Isolate* isolate, } else if (value->IsNullOrUndefined()) { // "data" was explicitly passed as null or undefined, assume the user wants // to send an empty body. - ended_ = true; + ended = true; + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestStreamJob::StartAsync, job, nullptr, + base::RetainedRef(response_headers), ended, error)); return; } - mate::Dictionary data(isolate, v8::Local::Cast(value)); + mate::Dictionary data(args->isolate(), v8::Local::Cast(value)); if (!data.Get("on", &value) || !value->IsFunction() || !data.Get("removeListener", &value) || !value->IsFunction()) { // If data is passed but it is not a stream, signal an error. - ended_ = true; - has_error_ = true; + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestStreamJob::OnError, job, net::ERR_FAILED)); return; } - subscriber_.reset(new mate::StreamSubscriber(isolate, data.GetHandle(), - weak_factory_.GetWeakPtr())); + auto subscriber = std::make_unique( + args->isolate(), data.GetHandle(), job); + + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestStreamJob::StartAsync, job, + std::move(subscriber), base::RetainedRef(response_headers), + ended, error)); } -void URLRequestStreamJob::StartAsync(std::unique_ptr options) { - if (has_error_) { - OnError(); +} // namespace + +URLRequestStreamJob::URLRequestStreamJob(net::URLRequest* request, + net::NetworkDelegate* network_delegate) + : net::URLRequestJob(request, network_delegate), + pending_buf_(nullptr), + pending_buf_size_(0), + ended_(false), + response_headers_(nullptr), + weak_factory_(this) {} + +URLRequestStreamJob::~URLRequestStreamJob() { + if (subscriber_) { + content::BrowserThread::DeleteSoon(content::BrowserThread::UI, FROM_HERE, + std::move(subscriber_)); + } +} + +void URLRequestStreamJob::Start() { + auto request_details = std::make_unique(); + FillRequestDetails(request_details.get(), request()); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::BindOnce(&JsAsker::AskForOptions, base::Unretained(isolate()), + handler(), std::move(request_details), + base::Bind(&BeforeStartInUI, weak_factory_.GetWeakPtr()))); +} + +void URLRequestStreamJob::StartAsync( + std::unique_ptr subscriber, + scoped_refptr response_headers, + bool ended, + int error) { + if (error != net::OK) { + NotifyStartError( + net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); return; } + + ended_ = ended; + response_headers_ = response_headers; + subscriber_ = std::move(subscriber); + request_start_time_ = base::TimeTicks::Now(); NotifyHeadersComplete(); } @@ -122,12 +162,13 @@ void URLRequestStreamJob::OnEnd() { ReadRawDataComplete(0); } -void URLRequestStreamJob::OnError() { - NotifyStartError( - net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED)); +void URLRequestStreamJob::OnError(int error) { + NotifyStartError(net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); } int URLRequestStreamJob::ReadRawData(net::IOBuffer* dest, int dest_size) { + response_start_time_ = base::TimeTicks::Now(); + if (ended_) return 0; @@ -185,6 +226,19 @@ void URLRequestStreamJob::GetResponseInfo(net::HttpResponseInfo* info) { info->headers = response_headers_; } +void URLRequestStreamJob::GetLoadTimingInfo( + net::LoadTimingInfo* load_timing_info) const { + load_timing_info->send_start = request_start_time_; + load_timing_info->send_end = request_start_time_; + load_timing_info->request_start = request_start_time_; + load_timing_info->receive_headers_end = response_start_time_; +} + +void URLRequestStreamJob::Kill() { + weak_factory_.InvalidateWeakPtrs(); + net::URLRequestJob::Kill(); +} + int URLRequestStreamJob::BufferCopy(std::vector* source, net::IOBuffer* target, int target_size) { diff --git a/atom/browser/net/url_request_stream_job.h b/atom/browser/net/url_request_stream_job.h index 8c1ed83c8bec..e56e9f169929 100644 --- a/atom/browser/net/url_request_stream_job.h +++ b/atom/browser/net/url_request_stream_job.h @@ -11,43 +11,42 @@ #include "atom/browser/api/stream_subscriber.h" #include "atom/browser/net/js_asker.h" -#include "base/memory/ref_counted_memory.h" -#include "native_mate/persistent_dictionary.h" +#include "base/memory/scoped_refptr.h" #include "net/base/io_buffer.h" #include "net/http/http_status_code.h" -#include "net/url_request/url_request_context_getter.h" -#include "v8/include/v8.h" +#include "net/url_request/url_request_job.h" namespace atom { -class URLRequestStreamJob : public JsAsker { +class URLRequestStreamJob : public JsAsker, public net::URLRequestJob { public: URLRequestStreamJob(net::URLRequest* request, net::NetworkDelegate* network_delegate); ~URLRequestStreamJob() override; + void StartAsync(std::unique_ptr subscriber, + scoped_refptr response_headers, + bool ended, + int error); + void OnData(std::vector&& buffer); // NOLINT void OnEnd(); - void OnError(); - - // URLRequestJob - void GetResponseInfo(net::HttpResponseInfo* info) override; + void OnError(int error); protected: // URLRequestJob + void Start() override; int ReadRawData(net::IOBuffer* buf, int buf_size) override; void DoneReading() override; void DoneReadingRedirectResponse() override; std::unique_ptr SetUpSourceStream() override; bool GetMimeType(std::string* mime_type) const override; int GetResponseCode() const override; + void GetResponseInfo(net::HttpResponseInfo* info) override; + void GetLoadTimingInfo(net::LoadTimingInfo* load_timing_info) const override; + void Kill() override; private: - // JSAsker - void BeforeStartInUI(v8::Isolate*, v8::Local) override; - void StartAsync(std::unique_ptr options) override; - void OnResponse(bool success, std::unique_ptr value); - int BufferCopy(std::vector* source, net::IOBuffer* target, int target_size); @@ -60,7 +59,8 @@ class URLRequestStreamJob : public JsAsker { std::vector write_buffer_; bool ended_; - bool has_error_; + base::TimeTicks request_start_time_; + base::TimeTicks response_start_time_; scoped_refptr response_headers_; std::unique_ptr subscriber_; diff --git a/atom/browser/net/url_request_string_job.cc b/atom/browser/net/url_request_string_job.cc index ef6422a57de6..678fc20b33b4 100644 --- a/atom/browser/net/url_request_string_job.cc +++ b/atom/browser/net/url_request_string_job.cc @@ -6,19 +6,69 @@ #include #include +#include #include "atom/common/atom_constants.h" +#include "atom/common/native_mate_converters/net_converter.h" +#include "atom/common/native_mate_converters/v8_value_converter.h" +#include "content/public/browser/browser_thread.h" #include "net/base/net_errors.h" namespace atom { +namespace { + +void BeforeStartInUI(base::WeakPtr job, + mate::Arguments* args) { + v8::Local value; + int error = net::OK; + std::unique_ptr request_options = nullptr; + + if (args->GetNext(&value)) { + V8ValueConverter converter; + v8::Local context = args->isolate()->GetCurrentContext(); + request_options.reset(converter.FromV8Value(value, context)); + } + + if (request_options) { + JsAsker::IsErrorOptions(request_options.get(), &error); + } else { + error = net::ERR_NOT_IMPLEMENTED; + } + + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestStringJob::StartAsync, job, + std::move(request_options), error)); +} + +} // namespace + URLRequestStringJob::URLRequestStringJob(net::URLRequest* request, net::NetworkDelegate* network_delegate) - : JsAsker(request, network_delegate) {} + : net::URLRequestSimpleJob(request, network_delegate), + weak_factory_(this) {} URLRequestStringJob::~URLRequestStringJob() = default; -void URLRequestStringJob::StartAsync(std::unique_ptr options) { +void URLRequestStringJob::Start() { + auto request_details = std::make_unique(); + FillRequestDetails(request_details.get(), request()); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::BindOnce(&JsAsker::AskForOptions, base::Unretained(isolate()), + handler(), std::move(request_details), + base::Bind(&BeforeStartInUI, weak_factory_.GetWeakPtr()))); +} + +void URLRequestStringJob::StartAsync(std::unique_ptr options, + int error) { + if (error != net::OK) { + NotifyStartError( + net::URLRequestStatus(net::URLRequestStatus::FAILED, error)); + return; + } + if (options->is_dict()) { base::DictionaryValue* dict = static_cast(options.get()); @@ -31,6 +81,11 @@ void URLRequestStringJob::StartAsync(std::unique_ptr options) { net::URLRequestSimpleJob::Start(); } +void URLRequestStringJob::Kill() { + weak_factory_.InvalidateWeakPtrs(); + net::URLRequestSimpleJob::Kill(); +} + void URLRequestStringJob::GetResponseInfo(net::HttpResponseInfo* info) { std::string status("HTTP/1.1 200 OK"); auto* headers = new net::HttpResponseHeaders(status); diff --git a/atom/browser/net/url_request_string_job.h b/atom/browser/net/url_request_string_job.h index 6f0ff3fbc2ea..5fc12e1cb31e 100644 --- a/atom/browser/net/url_request_string_job.h +++ b/atom/browser/net/url_request_string_job.h @@ -13,16 +13,17 @@ namespace atom { -class URLRequestStringJob : public JsAsker { +class URLRequestStringJob : public JsAsker, public net::URLRequestSimpleJob { public: URLRequestStringJob(net::URLRequest*, net::NetworkDelegate*); ~URLRequestStringJob() override; - // JsAsker: - void StartAsync(std::unique_ptr options) override; + void StartAsync(std::unique_ptr options, int error); // URLRequestJob: + void Start() override; void GetResponseInfo(net::HttpResponseInfo* info) override; + void Kill() override; // URLRequestSimpleJob: int GetData(std::string* mime_type, @@ -35,6 +36,8 @@ class URLRequestStringJob : public JsAsker { std::string charset_; std::string data_; + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(URLRequestStringJob); }; diff --git a/patches/common/chromium/.patches.yaml b/patches/common/chromium/.patches.yaml index 472d4edff979..30be3e3aadd7 100644 --- a/patches/common/chromium/.patches.yaml +++ b/patches/common/chromium/.patches.yaml @@ -42,7 +42,6 @@ patches: These files have debug checks explicitly commented out: - base/memory/weak_ptr.cc base/process/kill_win.cc components/viz/service/display/program_binding.h content/browser/frame_host/navigation_controller_impl.cc diff --git a/patches/common/chromium/dcheck.patch b/patches/common/chromium/dcheck.patch index 8c7ef57bcf69..918e7c5fb956 100644 --- a/patches/common/chromium/dcheck.patch +++ b/patches/common/chromium/dcheck.patch @@ -11,21 +11,6 @@ index 29960599a5c6..7352201e7b66 100644 #define DCHECK(condition) \ LAZY_STREAM(LOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition)) \ -diff --git a/base/memory/weak_ptr.cc b/base/memory/weak_ptr.cc -index d2a7d89e5667..def40703ea25 100644 ---- a/base/memory/weak_ptr.cc -+++ b/base/memory/weak_ptr.cc -@@ -23,8 +23,8 @@ void WeakReference::Flag::Invalidate() { - } - - bool WeakReference::Flag::IsValid() const { -- DCHECK(sequence_checker_.CalledOnValidSequence()) -- << "WeakPtrs must be checked on the same sequenced thread."; -+ // DCHECK(sequence_checker_.CalledOnValidSequence()) -+ // << "WeakPtrs must be checked on the same sequenced thread."; - return is_valid_; - } - diff --git a/base/process/kill_win.cc b/base/process/kill_win.cc index 7a664429bcd3..26f49dc3d1e7 100644 --- a/base/process/kill_win.cc diff --git a/spec/configs/unittests.yml b/spec/configs/unittests.yml index 0e01fb7752ef..19811377dd8a 100644 --- a/spec/configs/unittests.yml +++ b/spec/configs/unittests.yml @@ -4,12 +4,6 @@ tests: # with lists of disabled tests. Those properties' names # are used only to explain why a group of tests is disabled. disabled: - to_fix: - - WeakPtrDeathTest.NonOwnerThreadDeletesObjectAfterReference - - WeakPtrDeathTest.NonOwnerThreadDeletesWeakPtrAfterReference - - WeakPtrDeathTest.NonOwnerThreadDereferencesWeakPtrAfterReference - - WeakPtrDeathTest.NonOwnerThreadReferencesObjectAfterDeletion - - WeakPtrDeathTest.WeakPtrCopyDoesNotChangeThreadBinding undecided: - FilePathTest* - PartitionReallocReturnNullTest.RepeatedReturnNullDirect