Skip to content

Commit

Permalink
refactor: enable weak ptr unwrap sequence dcheck (electron#14816)
Browse files Browse the repository at this point in the history 
* refactor: enable weak ptr unwrap sequence dcheck

* spec: remove WeakPtrDeathTest.* from disabled list
  • Loading branch information
@deepak1556 @ckerr
deepak1556 authored and ckerr committed Oct 5, 2018
1 parent 6437815 commit 6e5dd73
Show file tree
Hide file tree
Showing 17 changed files with 427 additions and 254 deletions.
3 changes: 2 additions & 1 deletion atom/browser/api/stream_subscriber.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
1 change: 1 addition & 0 deletions atom/browser/net/asar/asar_protocol_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
57 changes: 18 additions & 39 deletions atom/browser/net/js_asker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,61 +4,42 @@

#include "atom/browser/net/js_asker.h"

#include <memory>
#include <utility>
#include <vector>

#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<v8::Value> 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<v8::Context> context = args->isolate()->GetCurrentContext();
std::unique_ptr<base::Value> 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<base::DictionaryValue> request_details,
const BeforeStartCallback& before_start,
const ResponseCallback& callback) {
// static
void JsAsker::AskForOptions(
v8::Isolate* isolate,
const JavaScriptHandler& handler,
std::unique_ptr<base::DictionaryValue> request_details,
const BeforeStartCallback& before_start) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
v8::Locker locker(isolate);
v8::HandleScope handle_scope(isolate);
v8::Local<v8::Context> 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<base::DictionaryValue*>(value);
if (dict->GetInteger("error", error))
Expand All @@ -70,6 +51,4 @@ bool IsErrorOptions(base::Value* value, int* error) {
return false;
}

} // namespace internal

} // namespace atom
103 changes: 17 additions & 86 deletions atom/browser/net/js_asker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,118 +6,49 @@
#define ATOM_BROWSER_NET_JS_ASKER_H_

#include <memory>
#include <utility>

#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<void(const base::DictionaryValue&, v8::Local<v8::Value>)>;
using BeforeStartCallback = base::Callback<void(mate::Arguments* args)>;

namespace internal {

using BeforeStartCallback =
base::Callback<void(v8::Isolate*, v8::Local<v8::Value>)>;
using ResponseCallback =
base::Callback<void(bool, std::unique_ptr<base::Value> options)>;

// Ask handler for options in UI thread.
void AskForOptions(v8::Isolate* isolate,
const JavaScriptHandler& handler,
std::unique_ptr<base::DictionaryValue> 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 <typename RequestJob>
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);

// Ask handler for options in UI thread.
static void AskForOptions(
v8::Isolate* isolate,
const JavaScriptHandler& handler,
std::unique_ptr<base::DictionaryValue> request_details,
const BeforeStartCallback& before_start);

// Subclass should do initailze work here.
virtual void BeforeStartInUI(v8::Isolate*, v8::Local<v8::Value>) {}
virtual void StartAsync(std::unique_ptr<base::Value> options) = 0;
// 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<base::DictionaryValue>();
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<base::Value> 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<JsAsker> weak_factory_;

DISALLOW_COPY_AND_ASSIGN(JsAsker);
};
Expand Down
60 changes: 58 additions & 2 deletions atom/browser/net/url_request_async_asar_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,70 @@

#include <memory>
#include <string>
#include <utility>

#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<URLRequestAsyncAsarJob> job,
mate::Arguments* args) {
v8::Local<v8::Value> value;
int error = net::OK;
std::unique_ptr<base::Value> request_options = nullptr;

if (args->GetNext(&value)) {
V8ValueConverter converter;
v8::Local<v8::Context> 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<asar::URLRequestAsarJob>(request, network_delegate) {}
: asar::URLRequestAsarJob(request, network_delegate), weak_factory_(this) {}

URLRequestAsyncAsarJob::~URLRequestAsyncAsarJob() = default;

void URLRequestAsyncAsarJob::Start() {
auto request_details = std::make_unique<base::DictionaryValue>();
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<base::Value> options,
int error) {
if (error != net::OK) {
NotifyStartError(
net::URLRequestStatus(net::URLRequestStatus::FAILED, error));
return;
}

void URLRequestAsyncAsarJob::StartAsync(std::unique_ptr<base::Value> options) {
std::string file_path;
if (options->is_dict()) {
auto* path_value =
Expand Down Expand Up @@ -46,6 +97,11 @@ void URLRequestAsyncAsarJob::StartAsync(std::unique_ptr<base::Value> 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);
Expand Down
10 changes: 7 additions & 3 deletions atom/browser/net/url_request_async_asar_job.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,21 @@
namespace atom {

// Like URLRequestAsarJob, but asks the JavaScript handler for file path.
class URLRequestAsyncAsarJob : public JsAsker<asar::URLRequestAsarJob> {
class URLRequestAsyncAsarJob : public asar::URLRequestAsarJob, public JsAsker {
public:
URLRequestAsyncAsarJob(net::URLRequest*, net::NetworkDelegate*);
~URLRequestAsyncAsarJob() override;

// JsAsker:
void StartAsync(std::unique_ptr<base::Value> options) override;
void StartAsync(std::unique_ptr<base::Value> options, int error);

// URLRequestJob:
void Start() override;
void GetResponseInfo(net::HttpResponseInfo* info) override;
void Kill() override;

private:
base::WeakPtrFactory<URLRequestAsyncAsarJob> weak_factory_;

DISALLOW_COPY_AND_ASSIGN(URLRequestAsyncAsarJob);
};

Expand Down
Loading

0 comments on commit 6e5dd73

Please sign in to comment.