From 70e86b5ffcc1f289ed8dead19ebb1712cc1af364 Mon Sep 17 00:00:00 2001 From: Pavel Beloborodov <73575789+boocmp@users.noreply.github.com> Date: Wed, 17 Aug 2022 18:03:01 +0700 Subject: [PATCH] Do not share SpeedreaderJsHandler betweeen gin handles. (#14642) --- .../speedreader/speedreader_browsertest.cc | 8 +++++ .../renderer/speedreader_js_handler.cc | 35 +++++++++++++------ .../renderer/speedreader_js_handler.h | 12 +++---- .../speedreader_render_frame_observer.cc | 18 ++-------- .../speedreader_render_frame_observer.h | 6 +--- 5 files changed, 43 insertions(+), 36 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 0fd9eb09f25..a64baf15e12 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -331,6 +331,14 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ShowOriginalPage) { NavigateToPageSynchronously(kTestPageReadable); auto* web_contents = ActiveWebContents(); + constexpr const char kCheckNoApiInMainWorld[] = + R"js( + document.speedreader === undefined + )js"; + EXPECT_TRUE(content::EvalJs(web_contents, kCheckNoApiInMainWorld, + content::EXECUTE_SCRIPT_DEFAULT_OPTIONS) + .ExtractBool()); + constexpr const char kClickLinkAndGetTitle[] = R"js( (function() { diff --git a/components/speedreader/renderer/speedreader_js_handler.cc b/components/speedreader/renderer/speedreader_js_handler.cc index c51cb0e0e89..ccb1fc0e828 100644 --- a/components/speedreader/renderer/speedreader_js_handler.cc +++ b/components/speedreader/renderer/speedreader_js_handler.cc @@ -5,6 +5,7 @@ #include "brave/components/speedreader/renderer/speedreader_js_handler.h" +#include "brave/components/speedreader/common/constants.h" #include "brave/components/speedreader/common/speedreader.mojom.h" #include "content/public/renderer/render_frame.h" #include "gin/converter.h" @@ -14,6 +15,11 @@ #include "mojo/public/cpp/bindings/associated_remote.h" #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" #include "third_party/blink/public/web/blink.h" +#include "third_party/blink/public/web/web_local_frame.h" + +namespace { +constexpr const char kSpeedreader[] = "speedreader"; +} namespace speedreader { @@ -24,30 +30,39 @@ SpeedreaderJSHandler::SpeedreaderJSHandler(content::RenderFrame* render_frame) SpeedreaderJSHandler::~SpeedreaderJSHandler() = default; -void SpeedreaderJSHandler::AddJavaScriptObjectToFrame( - v8::Local context) { +// static +void SpeedreaderJSHandler::Install(content::RenderFrame* render_frame) { v8::Isolate* isolate = blink::MainThreadIsolate(); v8::HandleScope handle_scope(isolate); + + v8::Local context = + render_frame->GetWebFrame()->GetScriptContextFromWorldId( + isolate, kIsolatedWorldId); if (context.IsEmpty()) return; v8::Context::Scope context_scope(context); v8::Local global = context->Global(); - gin::Handle handler = gin::CreateHandle(isolate, this); + // check object existence + v8::Local speedreader_value = + global->Get(context, gin::StringToV8(isolate, kSpeedreader)) + .ToLocalChecked(); + if (!speedreader_value->IsUndefined()) + return; + + gin::Handle handler = + gin::CreateHandle(isolate, new SpeedreaderJSHandler(render_frame)); if (handler.IsEmpty()) return; v8::PropertyDescriptor desc(handler.ToV8(), false); desc.set_configurable(false); - std::ignore = - global->DefineProperty(isolate->GetCurrentContext(), - gin::StringToV8(isolate, "speedreader"), desc); -} - -void SpeedreaderJSHandler::ResetRemote(content::RenderFrame* render_frame) { - render_frame_ = render_frame; + global + ->DefineProperty(isolate->GetCurrentContext(), + gin::StringToV8(isolate, kSpeedreader), desc) + .Check(); } gin::ObjectTemplateBuilder SpeedreaderJSHandler::GetObjectTemplateBuilder( diff --git a/components/speedreader/renderer/speedreader_js_handler.h b/components/speedreader/renderer/speedreader_js_handler.h index 23a9ab4034c..08d70f22de6 100644 --- a/components/speedreader/renderer/speedreader_js_handler.h +++ b/components/speedreader/renderer/speedreader_js_handler.h @@ -12,22 +12,22 @@ namespace speedreader { -class SpeedreaderJSHandler : public gin::Wrappable { +class SpeedreaderJSHandler final : public gin::Wrappable { public: static gin::WrapperInfo kWrapperInfo; - explicit SpeedreaderJSHandler(content::RenderFrame* render_frame); SpeedreaderJSHandler(const SpeedreaderJSHandler&) = delete; SpeedreaderJSHandler& operator=(const SpeedreaderJSHandler&) = delete; - ~SpeedreaderJSHandler() override; - void AddJavaScriptObjectToFrame(v8::Local context); - void ResetRemote(content::RenderFrame* render_frame); + static void Install(content::RenderFrame* render_frame); private: + explicit SpeedreaderJSHandler(content::RenderFrame* render_frame); + ~SpeedreaderJSHandler() final; + // gin::WrappableBase gin::ObjectTemplateBuilder GetObjectTemplateBuilder( - v8::Isolate* isolate) override; + v8::Isolate* isolate) final; // A function to be called from JS void ShowOriginalPage(v8::Isolate* isolate); diff --git a/components/speedreader/renderer/speedreader_render_frame_observer.cc b/components/speedreader/renderer/speedreader_render_frame_observer.cc index ec3c931db0b..4f39afdfacf 100644 --- a/components/speedreader/renderer/speedreader_render_frame_observer.cc +++ b/components/speedreader/renderer/speedreader_render_frame_observer.cc @@ -5,7 +5,6 @@ #include "brave/components/speedreader/renderer/speedreader_render_frame_observer.h" -#include "brave/components/speedreader/common/constants.h" #include "brave/components/speedreader/common/url_readable_hints.h" #include "brave/components/speedreader/renderer/speedreader_js_handler.h" #include "content/public/renderer/render_frame.h" @@ -24,21 +23,10 @@ void SpeedreaderRenderFrameObserver::DidStartNavigation( is_speedreadable_url_ = IsURLLooksReadable(url); } -void SpeedreaderRenderFrameObserver::DidCreateScriptContext( - v8::Local context, - int32_t world_id) { - if (!is_speedreadable_url_ || !render_frame()->IsMainFrame() || - world_id != kIsolatedWorldId) { +void SpeedreaderRenderFrameObserver::DidClearWindowObject() { + if (!is_speedreadable_url_ || !render_frame()->IsMainFrame()) return; - } - - if (!native_javascript_handle_) { - native_javascript_handle_.reset(new SpeedreaderJSHandler(render_frame())); - } else { - native_javascript_handle_->ResetRemote(render_frame()); - } - - native_javascript_handle_->AddJavaScriptObjectToFrame(context); + SpeedreaderJSHandler::Install(render_frame()); } void SpeedreaderRenderFrameObserver::OnDestruct() { diff --git a/components/speedreader/renderer/speedreader_render_frame_observer.h b/components/speedreader/renderer/speedreader_render_frame_observer.h index ca59516b07c..507697de17d 100644 --- a/components/speedreader/renderer/speedreader_render_frame_observer.h +++ b/components/speedreader/renderer/speedreader_render_frame_observer.h @@ -26,17 +26,13 @@ class SpeedreaderRenderFrameObserver : public content::RenderFrameObserver { void DidStartNavigation( const GURL& url, absl::optional navigation_type) override; - void DidCreateScriptContext(v8::Local context, - int32_t world_id) override; + void DidClearWindowObject() override; private: // RenderFrameObserver implementation. void OnDestruct() override; bool is_speedreadable_url_ = false; - - // Handle to "handler" JavaScript object functionality. - std::unique_ptr native_javascript_handle_; }; } // namespace speedreader