Skip to content

Commit

Permalink
Disallow DevTools main thread debugger when multiple main frames live…
Browse files Browse the repository at this point in the history
… 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
disble DevTools debugger when multiple frames exist, to mitigate risks
to pause main frames unexpectedly.

Bug: 1449114
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I3b990cf27beca949bd9d35f6ff77536c13229f52
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-Commit-Position: refs/heads/main@{#1153761}
  • Loading branch information
bashi authored and Chromium LUCI CQ committed Jun 6, 2023
1 parent d1565f2 commit 868828d
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 1 deletion.
4 changes: 4 additions & 0 deletions third_party/blink/common/features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1915,5 +1915,9 @@ BASE_FEATURE(kCSPWildcardsInPermissionsPolicies,
"CSPWildcardsInPermissionsPolicies",
base::FEATURE_DISABLED_BY_DEFAULT);

BASE_FEATURE(kAllowDevToolsMainThreadDebuggerForMutipleMainFrames,
"AllowDevToolsMainThreadDebuggerForMutipleMainFrames",
base::FEATURE_ENABLED_BY_DEFAULT);

} // namespace features
} // namespace blink
5 changes: 5 additions & 0 deletions third_party/blink/public/common/features.h
Original file line number Diff line number Diff line change
Expand Up @@ -1160,6 +1160,11 @@ BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kDirectCompositorThreadIpc);
// restriction is lifted and wildcards are supported in the port and scheme.
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kCSPWildcardsInPermissionsPolicies);

// Allows running DevTools main thread debugger even when a renderer process
// hosts multiple main frames.
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(
kAllowDevToolsMainThreadDebuggerForMutipleMainFrames);

} // namespace features
} // namespace blink

Expand Down
Original file line number Diff line number Diff line change
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,35 @@ 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::kAllowDevToolsMainThreadDebuggerForMutipleMainFrames)) {
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 enable chrome://flags/#disable-process-reuse "
"and restart the browser 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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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::kAllowDevToolsMainThreadDebuggerForMutipleMainFrames);
} else {
scoped_feature_list_.InitAndDisableFeature(
features::kAllowDevToolsMainThreadDebuggerForMutipleMainFrames);
}
}
~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 868828d

Please sign in to comment.