From 1729a9868c9ca0fe6f68400296e1efc6fda3efd1 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 2 Jul 2024 14:02:49 +0200 Subject: [PATCH] fix: use `BlockedRequest` struct to handle `webRequest` data (#42647) Fixes an issue where Chromium could crash on a dangling unretained pointer in one of several webRequest functions. This was happening as a result of the fact that we had outstanding blocking requests continue to reference state owned by ProxyingWebsocket and ProxyingURLLoaderFactory after the requests were destroyed. This had been going on for a few years, and was likely leading to some ongoing memory issues. To fix this, we need to ensure that all state is cleaned up in OnRequestWillBeDestroyed. I chose to create a new BlockedRequest struct to do so, which approximates the approach that upstream takes. The complexities of doing so also made our templated approach more trouble than it felt worth, so i pried that apart into separate handlers. --- shell/browser/api/electron_api_web_request.cc | 373 +++++++++++++----- shell/browser/api/electron_api_web_request.h | 38 +- spec/api-web-request-spec.ts | 4 +- 3 files changed, 304 insertions(+), 111 deletions(-) diff --git a/shell/browser/api/electron_api_web_request.cc b/shell/browser/api/electron_api_web_request.cc index 2ffc346bbf..0ad852279a 100644 --- a/shell/browser/api/electron_api_web_request.cc +++ b/shell/browser/api/electron_api_web_request.cc @@ -185,37 +185,41 @@ void FillDetails(gin_helper::Dictionary* details, Arg arg, Args... args) { FillDetails(details, args...); } -// Fill the native types with the result from the response object. -void ReadFromResponse(v8::Isolate* isolate, - gin::Dictionary* response, - GURL* new_location) { - response->Get("redirectURL", new_location); -} +// Modified from extensions/browser/api/web_request/web_request_api_helpers.cc. +std::pair, std::set> +CalculateOnBeforeSendHeadersDelta(const net::HttpRequestHeaders* old_headers, + const net::HttpRequestHeaders* new_headers) { + // Newly introduced or overridden request headers. + std::set modified_request_headers; + // Keys of request headers to be deleted. + std::set deleted_request_headers; -void ReadFromResponse(v8::Isolate* isolate, - gin::Dictionary* response, - net::HttpRequestHeaders* headers) { - v8::Local value; - if (response->Get("requestHeaders", &value) && value->IsObject()) { - headers->Clear(); - gin::Converter::FromV8(isolate, value, headers); - } -} + // The event listener might not have passed any new headers if it + // just wanted to cancel the request. + if (new_headers) { + // Find deleted headers. + { + net::HttpRequestHeaders::Iterator i(*old_headers); + while (i.GetNext()) { + if (!new_headers->HasHeader(i.name())) { + deleted_request_headers.insert(i.name()); + } + } + } -void ReadFromResponse(v8::Isolate* isolate, - gin::Dictionary* response, - const std::pair*, - const std::string>& headers) { - std::string status_line; - if (!response->Get("statusLine", &status_line)) - status_line = headers.second; - v8::Local value; - if (response->Get("responseHeaders", &value) && value->IsObject()) { - *headers.first = new net::HttpResponseHeaders(""); - (*headers.first)->ReplaceStatusLine(status_line); - gin::Converter::FromV8(isolate, value, - (*headers.first).get()); + // Find modified headers. + { + net::HttpRequestHeaders::Iterator i(*new_headers); + while (i.GetNext()) { + std::string value; + if (!old_headers->GetHeader(i.name(), &value) || i.value() != value) { + modified_request_headers.insert(i.name()); + } + } + } } + + return std::make_pair(modified_request_headers, deleted_request_headers); } } // namespace @@ -260,6 +264,24 @@ bool WebRequest::RequestFilter::MatchesRequest( return MatchesURL(info->url) && MatchesType(info->web_request_type); } +struct WebRequest::BlockedRequest { + BlockedRequest() = default; + raw_ptr request = nullptr; + net::CompletionOnceCallback callback; + // Only used for onBeforeSendHeaders. + BeforeSendHeadersCallback before_send_headers_callback; + // Only used for onBeforeSendHeaders. + raw_ptr request_headers = nullptr; + // Only used for onHeadersReceived. + scoped_refptr original_response_headers; + // Only used for onHeadersReceived. + raw_ptr> override_response_headers = + nullptr; + std::string status_line; + // Only used for onBeforeRequest. + raw_ptr new_url = nullptr; +}; + WebRequest::SimpleListenerInfo::SimpleListenerInfo(RequestFilter filter_, SimpleListener listener_) : filter(std::move(filter_)), listener(listener_) {} @@ -320,19 +342,152 @@ int WebRequest::OnBeforeRequest(extensions::WebRequestInfo* info, const network::ResourceRequest& request, net::CompletionOnceCallback callback, GURL* new_url) { - return HandleResponseEvent(ResponseEvent::kOnBeforeRequest, info, - std::move(callback), new_url, request); + return HandleOnBeforeRequestResponseEvent(info, request, std::move(callback), + new_url); +} + +int WebRequest::HandleOnBeforeRequestResponseEvent( + extensions::WebRequestInfo* request_info, + const network::ResourceRequest& request, + net::CompletionOnceCallback callback, + GURL* new_url) { + const auto iter = response_listeners_.find(ResponseEvent::kOnBeforeRequest); + if (iter == std::end(response_listeners_)) + return net::OK; + + const auto& info = iter->second; + if (!info.filter.MatchesRequest(request_info)) + return net::OK; + + BlockedRequest blocked_request; + blocked_request.callback = std::move(callback); + blocked_request.new_url = new_url; + blocked_requests_[request_info->id] = std::move(blocked_request); + + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + v8::HandleScope handle_scope(isolate); + gin_helper::Dictionary details(isolate, v8::Object::New(isolate)); + FillDetails(&details, request_info, request, *new_url); + + ResponseCallback response = + base::BindOnce(&WebRequest::OnBeforeRequestListenerResult, + base::Unretained(this), request_info->id); + info.listener.Run(gin::ConvertToV8(isolate, details), std::move(response)); + return net::ERR_IO_PENDING; +} + +void WebRequest::OnBeforeRequestListenerResult(uint64_t id, + v8::Local response) { + const auto iter = blocked_requests_.find(id); + if (iter == std::end(blocked_requests_)) + return; + + auto& request = iter->second; + + int result = net::OK; + if (response->IsObject()) { + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + gin::Dictionary dict(isolate, response.As()); + + bool cancel = false; + dict.Get("cancel", &cancel); + if (cancel) { + result = net::ERR_BLOCKED_BY_CLIENT; + } else { + dict.Get("redirectURL", request.new_url.get()); + } + } + + base::SequencedTaskRunner::GetCurrentDefault()->PostTask( + FROM_HERE, base::BindOnce(std::move(request.callback), result)); + blocked_requests_.erase(iter); } int WebRequest::OnBeforeSendHeaders(extensions::WebRequestInfo* info, const network::ResourceRequest& request, BeforeSendHeadersCallback callback, net::HttpRequestHeaders* headers) { - return HandleResponseEvent( - ResponseEvent::kOnBeforeSendHeaders, info, - base::BindOnce(std::move(callback), std::set(), - std::set()), - headers, request, *headers); + return HandleOnBeforeSendHeadersResponseEvent(info, request, + std::move(callback), headers); +} + +int WebRequest::HandleOnBeforeSendHeadersResponseEvent( + extensions::WebRequestInfo* request_info, + const network::ResourceRequest& request, + BeforeSendHeadersCallback callback, + net::HttpRequestHeaders* headers) { + const auto iter = + response_listeners_.find(ResponseEvent::kOnBeforeSendHeaders); + if (iter == std::end(response_listeners_)) + return net::OK; + + const auto& info = iter->second; + if (!info.filter.MatchesRequest(request_info)) + return net::OK; + + BlockedRequest blocked_request; + blocked_request.before_send_headers_callback = std::move(callback); + blocked_request.request_headers = headers; + blocked_requests_[request_info->id] = std::move(blocked_request); + + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + v8::HandleScope handle_scope(isolate); + gin_helper::Dictionary details(isolate, v8::Object::New(isolate)); + FillDetails(&details, request_info, request, *headers); + + ResponseCallback response = + base::BindOnce(&WebRequest::OnBeforeSendHeadersListenerResult, + base::Unretained(this), request_info->id); + info.listener.Run(gin::ConvertToV8(isolate, details), std::move(response)); + return net::ERR_IO_PENDING; +} + +void WebRequest::OnBeforeSendHeadersListenerResult( + uint64_t id, + v8::Local response) { + const auto iter = blocked_requests_.find(id); + if (iter == std::end(blocked_requests_)) + return; + + auto& request = iter->second; + + net::HttpRequestHeaders* old_headers = request.request_headers; + net::HttpRequestHeaders new_headers; + + int result = net::OK; + bool user_modified_headers = false; + if (response->IsObject()) { + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + gin::Dictionary dict(isolate, response.As()); + + bool cancel = false; + dict.Get("cancel", &cancel); + if (cancel) { + result = net::ERR_BLOCKED_BY_CLIENT; + } else { + v8::Local value; + if (dict.Get("requestHeaders", &value) && value->IsObject()) { + user_modified_headers = true; + gin::Converter::FromV8(isolate, value, + &new_headers); + } + } + } + + // If the user passes |cancel|, |new_headers| should be nullptr. + const auto updated_headers = CalculateOnBeforeSendHeadersDelta( + old_headers, + result == net::ERR_BLOCKED_BY_CLIENT ? nullptr : &new_headers); + + // Leave |request.request_headers| unchanged if the user didn't modify it. + if (user_modified_headers) + request.request_headers->Swap(&new_headers); + + base::SequencedTaskRunner::GetCurrentDefault()->PostTask( + FROM_HERE, + base::BindOnce(std::move(request.before_send_headers_callback), + updated_headers.first, updated_headers.second, result)); + blocked_requests_.erase(iter); } int WebRequest::OnHeadersReceived( @@ -342,12 +497,86 @@ int WebRequest::OnHeadersReceived( const net::HttpResponseHeaders* original_response_headers, scoped_refptr* override_response_headers, GURL* allowed_unsafe_redirect_url) { - const std::string& status_line = - original_response_headers ? original_response_headers->GetStatusLine() - : std::string(); - return HandleResponseEvent( - ResponseEvent::kOnHeadersReceived, info, std::move(callback), - std::make_pair(override_response_headers, status_line), request); + return HandleOnHeadersReceivedResponseEvent( + info, request, std::move(callback), original_response_headers, + override_response_headers); +} + +int WebRequest::HandleOnHeadersReceivedResponseEvent( + extensions::WebRequestInfo* request_info, + const network::ResourceRequest& request, + net::CompletionOnceCallback callback, + const net::HttpResponseHeaders* original_response_headers, + scoped_refptr* override_response_headers) { + const auto iter = response_listeners_.find(ResponseEvent::kOnHeadersReceived); + if (iter == std::end(response_listeners_)) + return net::OK; + + const auto& info = iter->second; + if (!info.filter.MatchesRequest(request_info)) + return net::OK; + + BlockedRequest blocked_request; + blocked_request.callback = std::move(callback); + blocked_request.override_response_headers = override_response_headers; + blocked_request.status_line = original_response_headers + ? original_response_headers->GetStatusLine() + : std::string(); + blocked_requests_[request_info->id] = std::move(blocked_request); + + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + v8::HandleScope handle_scope(isolate); + gin_helper::Dictionary details(isolate, v8::Object::New(isolate)); + FillDetails(&details, request_info, request); + + ResponseCallback response = + base::BindOnce(&WebRequest::OnHeadersReceivedListenerResult, + base::Unretained(this), request_info->id); + info.listener.Run(gin::ConvertToV8(isolate, details), std::move(response)); + return net::ERR_IO_PENDING; +} + +void WebRequest::OnHeadersReceivedListenerResult( + uint64_t id, + v8::Local response) { + const auto iter = blocked_requests_.find(id); + if (iter == std::end(blocked_requests_)) + return; + + auto& request = iter->second; + + int result = net::OK; + bool user_modified_headers = false; + scoped_refptr override_headers( + new net::HttpResponseHeaders("")); + if (response->IsObject()) { + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + gin::Dictionary dict(isolate, response.As()); + + bool cancel = false; + dict.Get("cancel", &cancel); + if (cancel) { + result = net::ERR_BLOCKED_BY_CLIENT; + } else { + std::string status_line; + if (!dict.Get("statusLine", &status_line)) + status_line = request.status_line; + v8::Local value; + if (dict.Get("responseHeaders", &value) && value->IsObject()) { + user_modified_headers = true; + override_headers->ReplaceStatusLine(status_line); + gin::Converter::FromV8( + isolate, value, override_headers.get()); + } + } + } + + if (user_modified_headers) + request.override_response_headers->swap(override_headers); + + base::SequencedTaskRunner::GetCurrentDefault()->PostTask( + FROM_HERE, base::BindOnce(std::move(request.callback), result)); + blocked_requests_.erase(iter); } void WebRequest::OnSendHeaders(extensions::WebRequestInfo* info, @@ -371,7 +600,7 @@ void WebRequest::OnResponseStarted(extensions::WebRequestInfo* info, void WebRequest::OnErrorOccurred(extensions::WebRequestInfo* info, const network::ResourceRequest& request, int net_error) { - callbacks_.erase(info->id); + blocked_requests_.erase(info->id); HandleSimpleEvent(SimpleEvent::kOnErrorOccurred, info, request, net_error); } @@ -379,13 +608,13 @@ void WebRequest::OnErrorOccurred(extensions::WebRequestInfo* info, void WebRequest::OnCompleted(extensions::WebRequestInfo* info, const network::ResourceRequest& request, int net_error) { - callbacks_.erase(info->id); + blocked_requests_.erase(info->id); HandleSimpleEvent(SimpleEvent::kOnCompleted, info, request, net_error); } void WebRequest::OnRequestWillBeDestroyed(extensions::WebRequestInfo* info) { - callbacks_.erase(info->id); + blocked_requests_.erase(info->id); } template @@ -479,62 +708,6 @@ void WebRequest::HandleSimpleEvent(SimpleEvent event, info.listener.Run(gin::ConvertToV8(isolate, details)); } -template -int WebRequest::HandleResponseEvent(ResponseEvent event, - extensions::WebRequestInfo* request_info, - net::CompletionOnceCallback callback, - Out out, - Args... args) { - const auto iter = response_listeners_.find(event); - if (iter == std::end(response_listeners_)) - return net::OK; - - const auto& info = iter->second; - if (!info.filter.MatchesRequest(request_info)) - return net::OK; - - callbacks_[request_info->id] = std::move(callback); - - v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); - v8::HandleScope handle_scope(isolate); - gin_helper::Dictionary details(isolate, v8::Object::New(isolate)); - FillDetails(&details, request_info, args...); - - ResponseCallback response = - base::BindOnce(&WebRequest::OnListenerResult, base::Unretained(this), - request_info->id, out); - info.listener.Run(gin::ConvertToV8(isolate, details), std::move(response)); - return net::ERR_IO_PENDING; -} - -template -void WebRequest::OnListenerResult(uint64_t id, - T out, - v8::Local response) { - const auto iter = callbacks_.find(id); - if (iter == std::end(callbacks_)) - return; - - int result = net::OK; - if (response->IsObject()) { - v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); - gin::Dictionary dict(isolate, response.As()); - - bool cancel = false; - dict.Get("cancel", &cancel); - if (cancel) - result = net::ERR_BLOCKED_BY_CLIENT; - else - ReadFromResponse(isolate, &dict, out); - } - - // The ProxyingURLLoaderFactory expects the callback to be executed - // asynchronously, because it used to work on IO thread before NetworkService. - base::SequencedTaskRunner::GetCurrentDefault()->PostTask( - FROM_HERE, base::BindOnce(std::move(callbacks_[id]), result)); - callbacks_.erase(iter); -} - // static gin::Handle WebRequest::FromOrCreate( v8::Isolate* isolate, diff --git a/shell/browser/api/electron_api_web_request.h b/shell/browser/api/electron_api_web_request.h index c11267b2fb..6444458535 100644 --- a/shell/browser/api/electron_api_web_request.h +++ b/shell/browser/api/electron_api_web_request.h @@ -84,6 +84,10 @@ class WebRequest : public gin::Wrappable, public WebRequestAPI { WebRequest(v8::Isolate* isolate, content::BrowserContext* browser_context); ~WebRequest() override; + // Contains info about requests that are blocked waiting for a response from + // the user. + struct BlockedRequest; + enum class SimpleEvent { kOnSendHeaders, kOnBeforeRedirect, @@ -91,6 +95,7 @@ class WebRequest : public gin::Wrappable, public WebRequestAPI { kOnCompleted, kOnErrorOccurred, }; + enum class ResponseEvent { kOnBeforeRequest, kOnBeforeSendHeaders, @@ -113,15 +118,30 @@ class WebRequest : public gin::Wrappable, public WebRequestAPI { void HandleSimpleEvent(SimpleEvent event, extensions::WebRequestInfo* info, Args... args); - template - int HandleResponseEvent(ResponseEvent event, - extensions::WebRequestInfo* info, - net::CompletionOnceCallback callback, - Out out, - Args... args); - template - void OnListenerResult(uint64_t id, T out, v8::Local response); + int HandleOnBeforeRequestResponseEvent( + extensions::WebRequestInfo* info, + const network::ResourceRequest& request, + net::CompletionOnceCallback callback, + GURL* redirect_url); + int HandleOnBeforeSendHeadersResponseEvent( + extensions::WebRequestInfo* info, + const network::ResourceRequest& request, + BeforeSendHeadersCallback callback, + net::HttpRequestHeaders* headers); + int HandleOnHeadersReceivedResponseEvent( + extensions::WebRequestInfo* info, + const network::ResourceRequest& request, + net::CompletionOnceCallback callback, + const net::HttpResponseHeaders* original_response_headers, + scoped_refptr* override_response_headers); + + void OnBeforeRequestListenerResult(uint64_t id, + v8::Local response); + void OnBeforeSendHeadersListenerResult(uint64_t id, + v8::Local response); + void OnHeadersReceivedListenerResult(uint64_t id, + v8::Local response); class RequestFilter { public: @@ -164,7 +184,7 @@ class WebRequest : public gin::Wrappable, public WebRequestAPI { std::map simple_listeners_; std::map response_listeners_; - std::map callbacks_; + std::map blocked_requests_; // Weak-ref, it manages us. raw_ptr browser_context_; diff --git a/spec/api-web-request-spec.ts b/spec/api-web-request-spec.ts index 3afaba1cd9..e57c7a2063 100644 --- a/spec/api-web-request-spec.ts +++ b/spec/api-web-request-spec.ts @@ -328,7 +328,7 @@ describe('webRequest module', () => { ses.webRequest.onBeforeSendHeaders((details, callback) => { const requestHeaders = details.requestHeaders; requestHeaders.Accept = '*/*;test/header'; - callback({ requestHeaders: requestHeaders }); + callback({ requestHeaders }); }); const { data } = await ajax('no-cors://fake-host/redirect'); expect(data).to.equal('header-received'); @@ -341,7 +341,7 @@ describe('webRequest module', () => { ses.webRequest.onBeforeSendHeaders((details, callback) => { const requestHeaders = details.requestHeaders; requestHeaders.Origin = 'http://new-origin'; - callback({ requestHeaders: requestHeaders }); + callback({ requestHeaders }); }); const { data } = await ajax(defaultURL); expect(data).to.equal('/new/origin');