From 12c1ffb353be02854fbde74472cd01720db84b4e Mon Sep 17 00:00:00 2001 From: cdesouza-chromium Date: Fri, 24 Mar 2023 12:03:38 +0000 Subject: [PATCH] Use thread local with BraveSearchServiceWorkerHolder (#17656) A CL has been landed in `cr113` to remove `base::ThreadLocal` variants, and replace their use with C++'s `thread_local`. In this change, `base::ThreadLocal` is being replaced with `thread_local` in `BraveSearchServiceWorkerHolder`. Chromium change: https://chromium.googlesource.com/chromium/src/+/4f39823862fe722c32bd72bdb0e126e952574b79 commit 4f39823862fe722c32bd72bdb0e126e952574b79 Author: Peter Kasting Date: Thu Mar 16 01:34:24 2023 +0000 Remove ThreadLocal{Pointer,Boolean}. These are now unused; `thread_local T*/bool` is generally a better option. Bug: 1416710 --- .../brave_search_service_worker_holder.cc | 137 +++++++++++++----- .../brave_search_service_worker_holder.h | 19 +-- 2 files changed, 104 insertions(+), 52 deletions(-) diff --git a/components/brave_search/renderer/brave_search_service_worker_holder.cc b/components/brave_search/renderer/brave_search_service_worker_holder.cc index 4062dd314fe..427b63b14a3 100644 --- a/components/brave_search/renderer/brave_search_service_worker_holder.cc +++ b/components/brave_search/renderer/brave_search_service_worker_holder.cc @@ -5,34 +5,118 @@ #include "brave/components/brave_search/renderer/brave_search_service_worker_holder.h" -#include +#include #include +#include -#include "base/no_destructor.h" +#include "base/auto_reset.h" #include "base/ranges/algorithm.h" +#include "base/threading/thread_checker.h" #include "brave/components/brave_search/common/brave_search_utils.h" #include "brave/components/brave_search/renderer/brave_search_fallback_js_handler.h" +#include "content/public/renderer/worker_thread.h" +#include "third_party/abseil-cpp/absl/base/attributes.h" #include "url/gurl.h" namespace brave_search { -using JSHandlersVector = - std::vector>; +namespace { -JSHandlersVector::iterator FindContext(JSHandlersVector* contexts, - v8::Local v8_context) { - auto context_matches = - [&v8_context]( - const std::unique_ptr& context) { - v8::HandleScope handle_scope(context->GetIsolate()); - v8::Context::Scope context_scope(context->Context()); +class JsHandlersForCurrentThread; - return context->Context() == v8_context; - }; +// A thread local pointer for the js handlers available. +ABSL_CONST_INIT thread_local JsHandlersForCurrentThread* current_js_handlers = + nullptr; - return base::ranges::find_if(*contexts, context_matches); +// A scoping class to create a local thread storage for the JS handlers, storing +// BraveSearchFallbackJSHandler instance in local thread storage, and providing +// ways to add/remove them. The instance self deletes on +// content::WorkerThread::Observer::WillStopCurrentWorkerThread. +class [[maybe_unused, nodiscard]] JsHandlersForCurrentThread + : public content::WorkerThread::Observer { + public: + JsHandlersForCurrentThread(const JsHandlersForCurrentThread&) = delete; + JsHandlersForCurrentThread& operator=(const JsHandlersForCurrentThread&) = + delete; + + ~JsHandlersForCurrentThread() override; + + // Gets the current js handlers, or creates an instance of it for the current + // thread. + static JsHandlersForCurrentThread* Get(); + + // Adds a JS handler to the scope. + void AddJsHandler(std::unique_ptr js_handler); + + // Searches and deletes + void RemoveContext(const v8::Local& v8_context); + + private: + JsHandlersForCurrentThread(); + + // WorkerThread::Observer: + void WillStopCurrentWorkerThread() override; + + // A vector for the handlers being held. + std::vector> js_handlers_; + + // A resetter responsible to make sure the local thread storage is set back to + // nullptr once the scope is gone. + const base::AutoReset resetter_; + + // This object should always be constructed and destructed on the same thread. + THREAD_CHECKER(thread_checker_); +}; + +JsHandlersForCurrentThread::JsHandlersForCurrentThread() + : resetter_(¤t_js_handlers, this) { + content::WorkerThread::AddObserver(this); } +JsHandlersForCurrentThread::~JsHandlersForCurrentThread() { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + content::WorkerThread::RemoveObserver(this); + for (auto& js_handler : js_handlers_) { + js_handler->Invalidate(); + } +} + +// static +JsHandlersForCurrentThread* JsHandlersForCurrentThread::Get() { + if (current_js_handlers) { + return current_js_handlers; + } + + return new JsHandlersForCurrentThread(); +} + +void JsHandlersForCurrentThread::AddJsHandler( + std::unique_ptr js_handler) { + js_handlers_.push_back(std::move(js_handler)); +} + +void JsHandlersForCurrentThread::RemoveContext( + const v8::Local& v8_context) { + DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + js_handlers_.erase( + base::ranges::remove_if( + js_handlers_, + [&v8_context]( + const std::unique_ptr& js_handler) { + v8::HandleScope handle_scope(js_handler->GetIsolate()); + v8::Context::Scope context_scope(js_handler->Context()); + return js_handler->Context() == v8_context; + }), + js_handlers_.end()); +} + +void JsHandlersForCurrentThread::WillStopCurrentWorkerThread() { + DCHECK(current_js_handlers); + delete this; +} + +} // namespace + BraveSearchServiceWorkerHolder::BraveSearchServiceWorkerHolder() : broker_(nullptr) {} @@ -59,13 +143,7 @@ void BraveSearchServiceWorkerHolder::WillEvaluateServiceWorkerOnWorkerThread( new BraveSearchFallbackJSHandler(v8_context, broker_)); js_handler->AddJavaScriptObject(); - JSHandlersVector* js_handlers = js_handlers_tls_.Get(); - if (!js_handlers) { - js_handlers = new JSHandlersVector(); - js_handlers_tls_.Set(js_handlers); - content::WorkerThread::AddObserver(this); - } - js_handlers->push_back(std::move(js_handler)); + JsHandlersForCurrentThread::Get()->AddJsHandler(std::move(js_handler)); } void BraveSearchServiceWorkerHolder:: @@ -79,22 +157,11 @@ void BraveSearchServiceWorkerHolder:: !IsAllowedHost(service_worker_scope)) return; - JSHandlersVector* js_handlers = js_handlers_tls_.Get(); - if (!js_handlers) + if (!current_js_handlers) { return; + } - auto context_it = FindContext(js_handlers, v8_context); - js_handlers->erase(context_it); -} - -void BraveSearchServiceWorkerHolder::WillStopCurrentWorkerThread() { - content::WorkerThread::RemoveObserver(this); - JSHandlersVector* js_handlers = js_handlers_tls_.Get(); - DCHECK(js_handlers); - for (const auto& context : *js_handlers) - context->Invalidate(); - js_handlers_tls_.Set(nullptr); - delete js_handlers; + current_js_handlers->RemoveContext(v8_context); } } // namespace brave_search diff --git a/components/brave_search/renderer/brave_search_service_worker_holder.h b/components/brave_search/renderer/brave_search_service_worker_holder.h index f588bd1997b..6054083b565 100644 --- a/components/brave_search/renderer/brave_search_service_worker_holder.h +++ b/components/brave_search/renderer/brave_search_service_worker_holder.h @@ -6,12 +6,7 @@ #ifndef BRAVE_COMPONENTS_BRAVE_SEARCH_RENDERER_BRAVE_SEARCH_SERVICE_WORKER_HOLDER_H_ #define BRAVE_COMPONENTS_BRAVE_SEARCH_RENDERER_BRAVE_SEARCH_SERVICE_WORKER_HOLDER_H_ -#include -#include - #include "base/memory/raw_ptr.h" -#include "base/threading/thread_local.h" -#include "content/public/renderer/worker_thread.h" #include "v8/include/v8.h" class GURL; @@ -23,16 +18,14 @@ class ThreadSafeBrowserInterfaceBrokerProxy; namespace brave_search { -class BraveSearchFallbackJSHandler; - -class BraveSearchServiceWorkerHolder : public content::WorkerThread::Observer { +class BraveSearchServiceWorkerHolder { public: BraveSearchServiceWorkerHolder(); BraveSearchServiceWorkerHolder(const BraveSearchServiceWorkerHolder&) = delete; BraveSearchServiceWorkerHolder& operator=( const BraveSearchServiceWorkerHolder&) = delete; - ~BraveSearchServiceWorkerHolder() override; + ~BraveSearchServiceWorkerHolder(); void SetBrowserInterfaceBrokerProxy( blink::ThreadSafeBrowserInterfaceBrokerProxy* broker); @@ -49,14 +42,6 @@ class BraveSearchServiceWorkerHolder : public content::WorkerThread::Observer { const GURL& script_url); private: - // WorkerThread::Observer: - void WillStopCurrentWorkerThread() override; - - // Implement thread safety by storing each BraveSearchFallbackJSHandler - // in TLS. The vector is called from worker threads. - base::ThreadLocalPointer< - std::vector>> - js_handlers_tls_; raw_ptr broker_ = nullptr; // not owned };