From b781cb1dbffd90b49ec9dabfdeb0aaf95ecc7200 Mon Sep 17 00:00:00 2001 From: Kenichi Ishibashi Date: Mon, 12 Jun 2023 09:23:55 +0000 Subject: [PATCH] [M115 merge] Disallow DevTools main thread debugger when multiple main frames live in a renderer DevToolsMainThreadDebugger indirectly depends on ScopedPagePauser. Since ScopedPagePauser works only for one main frame, DevToolsMainThreadDebugger doesn't work properly when multiple main frames exist in a renderer. This CL adds a feature flag to enable or disable DevTools debugger when multiple frames exist, to mitigate risks to pause main frames unexpectedly. This merge also contains warning message update [1] and typo fix [2]. [1] https://chromium-review.googlesource.com/c/chromium/src/+/4595420 [2] https://chromium-review.googlesource.com/c/chromium/src/+/4601249 (cherry picked from commit 868828d28f0847ea6ce90dcf9f61469eb475ab13) Bug: 1449114 Change-Id: I61f072d13f9e64d4ce4361ce29f37c5673456a8f Fuchsia-Binary-Size: Size increase is unavoidable. Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4582356 Reviewed-by: Kentaro Hara Commit-Queue: Kenichi Ishibashi Reviewed-by: Danil Somsikov Cr-Original-Commit-Position: refs/heads/main@{#1153761} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4603376 Cr-Commit-Position: refs/branch-heads/5790@{#620} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- third_party/blink/common/features.cc | 4 ++ third_party/blink/public/common/features.h | 5 ++ .../core/inspector/main_thread_debugger.cc | 34 +++++++++++- .../core/inspector/main_thread_debugger.h | 2 + .../inspector/main_thread_debugger_test.cc | 54 +++++++++++++++++++ 5 files changed, 98 insertions(+), 1 deletion(-) diff --git a/third_party/blink/common/features.cc b/third_party/blink/common/features.cc index 4d1d19bd12e00..5ed6f2ce43c03 100644 --- a/third_party/blink/common/features.cc +++ b/third_party/blink/common/features.cc @@ -1896,5 +1896,9 @@ BASE_FEATURE(kDirectCompositorThreadIpc, "DirectCompositorThreadIpc", base::FEATURE_DISABLED_BY_DEFAULT); +BASE_FEATURE(kAllowDevToolsMainThreadDebuggerForMultipleMainFrames, + "AllowDevToolsMainThreadDebuggerForMultipleMainFrames", + base::FEATURE_ENABLED_BY_DEFAULT); + } // namespace features } // namespace blink diff --git a/third_party/blink/public/common/features.h b/third_party/blink/public/common/features.h index 77d6a1742cd31..eda86304f2743 100644 --- a/third_party/blink/public/common/features.h +++ b/third_party/blink/public/common/features.h @@ -1150,6 +1150,11 @@ BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kMainThreadHighPriorityImageLoading); // hopping through the IO thread first. BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kDirectCompositorThreadIpc); +// Allows running DevTools main thread debugger even when a renderer process +// hosts multiple main frames. +BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE( + kAllowDevToolsMainThreadDebuggerForMultipleMainFrames); + } // namespace features } // namespace blink diff --git a/third_party/blink/renderer/core/inspector/main_thread_debugger.cc b/third_party/blink/renderer/core/inspector/main_thread_debugger.cc index b5d7d4fd76a13..3e4c70b7281c6 100644 --- a/third_party/blink/renderer/core/inspector/main_thread_debugger.cc +++ b/third_party/blink/renderer/core/inspector/main_thread_debugger.cc @@ -32,8 +32,10 @@ #include +#include "base/feature_list.h" #include "base/synchronization/lock.h" #include "build/chromeos_buildflags.h" +#include "third_party/blink/public/common/features.h" #include "third_party/blink/renderer/bindings/core/v8/binding_security.h" #include "third_party/blink/renderer/bindings/core/v8/script_controller.h" #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h" @@ -65,6 +67,7 @@ #include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h" #include "third_party/blink/renderer/platform/bindings/script_state.h" #include "third_party/blink/renderer/platform/bindings/source_location.h" +#include "third_party/blink/renderer/platform/heap/garbage_collected.h" #include "third_party/blink/renderer/platform/instrumentation/use_counter.h" #include "third_party/blink/renderer/platform/wtf/text/string_builder.h" @@ -334,7 +337,36 @@ void MainThreadDebugger::endEnsureAllContextsInGroup(int context_group_id) { bool MainThreadDebugger::canExecuteScripts(int context_group_id) { LocalFrame* frame = WeakIdentifierMap::Lookup(context_group_id); - return frame->DomWindow()->CanExecuteScripts(kNotAboutToExecuteScript); + if (!frame->DomWindow()->CanExecuteScripts(kNotAboutToExecuteScript)) { + return false; + } + + if (base::FeatureList::IsEnabled( + features::kAllowDevToolsMainThreadDebuggerForMultipleMainFrames)) { + return true; + } + + size_t num_main_frames = 0; + for (auto& page : Page::OrdinaryPages()) { + if (page->MainFrame() && page->MainFrame()->IsOutermostMainFrame()) { + ++num_main_frames; + } + } + + if (num_main_frames > 1) { + String message = String( + "DevTools debugger is disabled because it is attached to a process " + "that hosts multiple top-level frames, where DevTools debugger doesn't " + "work properly. Please relaunch the browser with " + "--disable-features=ProcessPerSiteUpToMainFrameThreshold to enable " + "debugger."); + frame->Console().AddMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kJavaScript, + mojom::ConsoleMessageLevel::kError, message)); + return false; + } + + return true; } void MainThreadDebugger::runIfWaitingForDebugger(int context_group_id) { diff --git a/third_party/blink/renderer/core/inspector/main_thread_debugger.h b/third_party/blink/renderer/core/inspector/main_thread_debugger.h index d0a997ecfa8e5..3cb056671b0ac 100644 --- a/third_party/blink/renderer/core/inspector/main_thread_debugger.h +++ b/third_party/blink/renderer/core/inspector/main_thread_debugger.h @@ -81,6 +81,8 @@ class CORE_EXPORT MainThreadDebugger final : public ThreadDebuggerCommonImpl { void ExceptionThrown(ExecutionContext*, ErrorEvent*); private: + FRIEND_TEST_ALL_PREFIXES(MainThreadDebuggerMultipleMainFramesTest, Allow); + void ReportConsoleMessage(ExecutionContext*, mojom::ConsoleMessageSource, mojom::ConsoleMessageLevel, diff --git a/third_party/blink/renderer/core/inspector/main_thread_debugger_test.cc b/third_party/blink/renderer/core/inspector/main_thread_debugger_test.cc index d0a76e41f361d..ab05c6263146d 100644 --- a/third_party/blink/renderer/core/inspector/main_thread_debugger_test.cc +++ b/third_party/blink/renderer/core/inspector/main_thread_debugger_test.cc @@ -5,9 +5,16 @@ #include "third_party/blink/renderer/core/inspector/main_thread_debugger.h" #include + +#include "base/test/scoped_feature_list.h" #include "testing/gtest/include/gtest/gtest.h" +#include "third_party/blink/public/common/features.h" #include "third_party/blink/renderer/core/dom/document.h" +#include "third_party/blink/renderer/core/frame/local_frame.h" #include "third_party/blink/renderer/core/frame/local_frame_view.h" +#include "third_party/blink/renderer/core/frame/settings.h" +#include "third_party/blink/renderer/core/page/page.h" +#include "third_party/blink/renderer/core/testing/dummy_page_holder.h" #include "third_party/blink/renderer/core/testing/page_test_base.h" namespace blink { @@ -33,4 +40,51 @@ TEST_F(MainThreadDebuggerTest, HitBreakPointDuringLifecycle) { EXPECT_FALSE(document.Lifecycle().LifecyclePostponed()); } +class MainThreadDebuggerMultipleMainFramesTest + : public MainThreadDebuggerTest, + public testing::WithParamInterface { + public: + MainThreadDebuggerMultipleMainFramesTest() { + if (IsDebuggerAllowed()) { + scoped_feature_list_.InitAndEnableFeature( + features::kAllowDevToolsMainThreadDebuggerForMultipleMainFrames); + } else { + scoped_feature_list_.InitAndDisableFeature( + features::kAllowDevToolsMainThreadDebuggerForMultipleMainFrames); + } + } + ~MainThreadDebuggerMultipleMainFramesTest() override = default; + + void SetUp() override { + second_dummy_page_holder_ = std::make_unique(); + MainThreadDebuggerTest::SetUp(); + } + + Page& GetSecondPage() { return second_dummy_page_holder_->GetPage(); } + + bool IsDebuggerAllowed() { return GetParam(); } + + private: + base::test::ScopedFeatureList scoped_feature_list_; + std::unique_ptr second_dummy_page_holder_; +}; + +INSTANTIATE_TEST_SUITE_P(All, + MainThreadDebuggerMultipleMainFramesTest, + testing::Bool()); + +TEST_P(MainThreadDebuggerMultipleMainFramesTest, Allow) { + Page::InsertOrdinaryPageForTesting(&GetPage()); + Page::InsertOrdinaryPageForTesting(&GetSecondPage()); + GetFrame().GetSettings()->SetScriptEnabled(true); + auto* debugger = MainThreadDebugger::Instance(); + int context_group_id = debugger->ContextGroupId(&GetFrame()); + + if (IsDebuggerAllowed()) { + ASSERT_TRUE(debugger->canExecuteScripts(context_group_id)); + } else { + ASSERT_FALSE(debugger->canExecuteScripts(context_group_id)); + } +} + } // namespace blink