Skip to content

Commit

Permalink
[M115 merge] Disallow DevTools main thread debugger when multiple mai…
Browse files Browse the repository at this point in the history
…n 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 868828d)

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 <haraken@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Danil Somsikov <dsv@chromium.org>
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: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
bashi authored and Chromium LUCI CQ committed Jun 12, 2023
1 parent 0dad824 commit b781cb1
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 1 deletion.
4 changes: 4 additions & 0 deletions third_party/blink/common/features.cc
Expand Up @@ -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
5 changes: 5 additions & 0 deletions third_party/blink/public/common/features.h
Expand Up @@ -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

Expand Down
Expand Up @@ -32,8 +32,10 @@

#include <memory>

#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"
Expand Down Expand Up @@ -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"

Expand Down Expand Up @@ -334,7 +337,36 @@ void MainThreadDebugger::endEnsureAllContextsInGroup(int context_group_id) {

bool MainThreadDebugger::canExecuteScripts(int context_group_id) {
LocalFrame* frame = WeakIdentifierMap<LocalFrame>::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<ConsoleMessage>(
mojom::ConsoleMessageSource::kJavaScript,
mojom::ConsoleMessageLevel::kError, message));
return false;
}

return true;
}

void MainThreadDebugger::runIfWaitingForDebugger(int context_group_id) {
Expand Down
Expand Up @@ -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,
Expand Down
Expand Up @@ -5,9 +5,16 @@
#include "third_party/blink/renderer/core/inspector/main_thread_debugger.h"

#include <memory>

#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 {
Expand All @@ -33,4 +40,51 @@ TEST_F(MainThreadDebuggerTest, HitBreakPointDuringLifecycle) {
EXPECT_FALSE(document.Lifecycle().LifecyclePostponed());
}

class MainThreadDebuggerMultipleMainFramesTest
: public MainThreadDebuggerTest,
public testing::WithParamInterface<bool> {
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<DummyPageHolder>();
MainThreadDebuggerTest::SetUp();
}

Page& GetSecondPage() { return second_dummy_page_holder_->GetPage(); }

bool IsDebuggerAllowed() { return GetParam(); }

private:
base::test::ScopedFeatureList scoped_feature_list_;
std::unique_ptr<DummyPageHolder> 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

0 comments on commit b781cb1

Please sign in to comment.