Skip to content

Commit 9e52d5c

Browse files
sigurdschneiderCommit Bot
sigurdschneider
authored and
Commit Bot
committed
[debugger] Allow termination-on-resume when paused at a breakpoint
This CL implements functionality to allow an embedder to mark a debug scope as terminate-on-resume. This results in a termination exception when that debug scope is left and execution is resumed. Execution of JavaScript remains possible after a debug scope is marked as terminate-on-resume (but before execution of the paused code resumes). This is used by blink to correctly prevent resuming JavaScript execution upon reload while being paused at a breakpoint. This is important for handling reloads while paused at a breakpoint in blink. The resume command terminates blink's nested message loop that is used while to keep the frame responsive while the debugger is paused. But if a reload is triggered while execution is paused on a breakpoint, but before execution is actually resumed from the breakpoint (that means before returning into the V8 JavaScript frames that are paused on the stack below the C++ frames that belong to the nested message loop), we re-enter V8 to do tear-down actions of the old frame. In this case Runtime.terminateExecution() cannot be used before Debugger.resume(), because the tear-down actions that re-enter V8 would trigger the termination exception and crash the browser (because the browser expected the tear-down to succeed). Hence we introduce this flag on V8 that says: It is OK if someone re-enters V8 (to execute JS), but upon resuming from the breakpoint (i.e. returning to the paused frames that are on the stack below), generate a termination exception. We deliberated adding a corresponding logic on the blink side (instead of V8) but we think this is the simplest solution. More details in the design doc: https://docs.google.com/document/d/1aO9v0YhoKNqKleqfACGUpwrBUayLFGqktz9ltdgKHMk Bug: chromium:1004038, chromium:1014415 Change-Id: I896692d4c21cb0acae89c1d783d37ce45b73c113 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1924366 Commit-Queue: Sigurd Schneider <sigurds@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#66084}
1 parent ea56bf5 commit 9e52d5c

14 files changed

Lines changed: 584 additions & 17 deletions

‎include/js_protocol.pdl‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,13 @@ domain Debugger
273273

274274
# Resumes JavaScript execution.
275275
command resume
276+
parameters
277+
# Set to true to terminate execution upon resuming execution. In contrast
278+
# to Runtime.terminateExecution, this will allows to execute further
279+
# JavaScript (i.e. via evaluation) until execution of the paused code
280+
# is actually resumed, at which point termination is triggered.
281+
# If execution is currently not paused, this parameter has no effect.
282+
optional boolean terminateOnResume
276283

277284
# Searches for given string in script content.
278285
command searchInContent

‎include/v8-inspector.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ class V8_EXPORT V8InspectorSession {
145145
virtual void breakProgram(const StringView& breakReason,
146146
const StringView& breakDetails) = 0;
147147
virtual void setSkipAllPauses(bool) = 0;
148-
virtual void resume() = 0;
148+
virtual void resume(bool setTerminateOnResume = false) = 0;
149149
virtual void stepOver() = 0;
150150
virtual std::vector<std::unique_ptr<protocol::Debugger::API::SearchMatch>>
151151
searchInTextByLines(const StringView& text, const StringView& query,

‎src/api/api.cc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9443,6 +9443,12 @@ void debug::BreakRightNow(Isolate* v8_isolate) {
94439443
isolate->debug()->HandleDebugBreak(i::kIgnoreIfAllFramesBlackboxed);
94449444
}
94459445

9446+
void debug::SetTerminateOnResume(Isolate* v8_isolate) {
9447+
i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
9448+
ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
9449+
isolate->debug()->SetTerminateOnResume();
9450+
}
9451+
94469452
bool debug::AllFramesOnStackAreBlackboxed(Isolate* v8_isolate) {
94479453
i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
94489454
ENTER_V8_DO_NOT_USE(isolate);

‎src/debug/debug-interface.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ void PrepareStep(Isolate* isolate, StepAction action);
101101
void ClearStepping(Isolate* isolate);
102102
V8_EXPORT_PRIVATE void BreakRightNow(Isolate* isolate);
103103

104+
// Use `SetTerminateOnResume` to indicate that an TerminateExecution interrupt
105+
// should be set shortly before resuming, i.e. shortly before returning into
106+
// the JavaScript stack frames on the stack. In contrast to setting the
107+
// interrupt with `RequestTerminateExecution` directly, this flag allows
108+
// the isolate to be entered for further JavaScript execution.
109+
V8_EXPORT_PRIVATE void SetTerminateOnResume(Isolate* isolate);
110+
104111
bool AllFramesOnStackAreBlackboxed(Isolate* isolate);
105112

106113
class Script;

‎src/debug/debug.cc‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1718,8 +1718,8 @@ Handle<FixedArray> Debug::GetLoadedScripts() {
17181718
return FixedArray::ShrinkOrEmpty(isolate_, results, length);
17191719
}
17201720

1721-
void Debug::OnThrow(Handle<Object> exception) {
1722-
if (in_debug_scope() || ignore_events()) return;
1721+
base::Optional<Object> Debug::OnThrow(Handle<Object> exception) {
1722+
if (in_debug_scope() || ignore_events()) return {};
17231723
// Temporarily clear any scheduled_exception to allow evaluating
17241724
// JavaScript from the debug event handler.
17251725
HandleScope scope(isolate_);
@@ -1736,6 +1736,14 @@ void Debug::OnThrow(Handle<Object> exception) {
17361736
isolate_->thread_local_top()->scheduled_exception_ = *scheduled_exception;
17371737
}
17381738
PrepareStepOnThrow();
1739+
// If the OnException handler requested termination, then indicated this to
1740+
// our caller Isolate::Throw so it can deal with it immediatelly instead of
1741+
// throwing the original exception.
1742+
if (isolate_->stack_guard()->CheckTerminateExecution()) {
1743+
isolate_->stack_guard()->ClearTerminateExecution();
1744+
return isolate_->TerminateExecution();
1745+
}
1746+
return {};
17391747
}
17401748

17411749
void Debug::OnPromiseReject(Handle<Object> promise, Handle<Object> value) {
@@ -2092,7 +2100,6 @@ DebugScope::DebugScope(Debug* debug)
20922100
// Link recursive debugger entry.
20932101
base::Relaxed_Store(&debug_->thread_local_.current_debug_scope_,
20942102
reinterpret_cast<base::AtomicWord>(this));
2095-
20962103
// Store the previous frame id and return value.
20972104
break_frame_id_ = debug_->break_frame_id();
20982105

@@ -2106,8 +2113,18 @@ DebugScope::DebugScope(Debug* debug)
21062113
debug_->UpdateState();
21072114
}
21082115

2116+
void DebugScope::set_terminate_on_resume() { terminate_on_resume_ = true; }
21092117

21102118
DebugScope::~DebugScope() {
2119+
// Terminate on resume must have been handled by retrieving it, if this is
2120+
// the outer scope.
2121+
if (terminate_on_resume_) {
2122+
if (!prev_) {
2123+
debug_->isolate_->stack_guard()->RequestTerminateExecution();
2124+
} else {
2125+
prev_->set_terminate_on_resume();
2126+
}
2127+
}
21112128
// Leaving this debugger entry.
21122129
base::Relaxed_Store(&debug_->thread_local_.current_debug_scope_,
21132130
reinterpret_cast<base::AtomicWord>(prev_));
@@ -2147,6 +2164,13 @@ void Debug::UpdateDebugInfosForExecutionMode() {
21472164
}
21482165
}
21492166

2167+
void Debug::SetTerminateOnResume() {
2168+
DebugScope* scope = reinterpret_cast<DebugScope*>(
2169+
base::Acquire_Load(&thread_local_.current_debug_scope_));
2170+
CHECK_NOT_NULL(scope);
2171+
scope->set_terminate_on_resume();
2172+
}
2173+
21502174
void Debug::StartSideEffectCheckMode() {
21512175
DCHECK(isolate_->debug_execution_mode() != DebugInfo::kSideEffects);
21522176
isolate_->set_debug_execution_mode(DebugInfo::kSideEffects);

‎src/debug/debug.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,8 @@ class V8_EXPORT_PRIVATE Debug {
217217
// Debug event triggers.
218218
void OnDebugBreak(Handle<FixedArray> break_points_hit);
219219

220-
void OnThrow(Handle<Object> exception);
220+
base::Optional<Object> OnThrow(Handle<Object> exception)
221+
V8_WARN_UNUSED_RESULT;
221222
void OnPromiseReject(Handle<Object> promise, Handle<Object> value);
222223
void OnCompileError(Handle<Script> script);
223224
void OnAfterCompile(Handle<Script> script);
@@ -238,6 +239,8 @@ class V8_EXPORT_PRIVATE Debug {
238239
void ChangeBreakOnException(ExceptionBreakType type, bool enable);
239240
bool IsBreakOnException(ExceptionBreakType type);
240241

242+
void SetTerminateOnResume();
243+
241244
bool SetBreakPointForScript(Handle<Script> script, Handle<String> condition,
242245
int* source_position, int* id);
243246
bool SetBreakpointForFunction(Handle<SharedFunctionInfo> shared,
@@ -564,13 +567,17 @@ class DebugScope {
564567
explicit DebugScope(Debug* debug);
565568
~DebugScope();
566569

570+
void set_terminate_on_resume();
571+
567572
private:
568573
Isolate* isolate() { return debug_->isolate_; }
569574

570575
Debug* debug_;
571576
DebugScope* prev_; // Previous scope if entered recursively.
572577
StackFrameId break_frame_id_; // Previous break frame id.
573578
PostponeInterruptsScope no_interrupts_;
579+
// This is used as a boolean.
580+
bool terminate_on_resume_ = false;
574581
};
575582

576583
// This scope is used to handle return values in nested debug break points.

‎src/execution/isolate.cc‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1523,7 +1523,10 @@ Object Isolate::Throw(Object raw_exception, MessageLocation* location) {
15231523

15241524
// Notify debugger of exception.
15251525
if (is_catchable_by_javascript(raw_exception)) {
1526-
debug()->OnThrow(exception);
1526+
base::Optional<Object> maybe_exception = debug()->OnThrow(exception);
1527+
if (maybe_exception.has_value()) {
1528+
return *maybe_exception;
1529+
}
15271530
}
15281531

15291532
// Generate the message if required.

‎src/inspector/v8-debugger-agent-impl.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1034,10 +1034,11 @@ Response V8DebuggerAgentImpl::pause() {
10341034
return Response::OK();
10351035
}
10361036

1037-
Response V8DebuggerAgentImpl::resume() {
1037+
Response V8DebuggerAgentImpl::resume(Maybe<bool> terminateOnResume) {
10381038
if (!isPaused()) return Response::Error(kDebuggerNotPaused);
10391039
m_session->releaseObjectGroup(kBacktraceObjectGroup);
1040-
m_debugger->continueProgram(m_session->contextGroupId());
1040+
m_debugger->continueProgram(m_session->contextGroupId(),
1041+
terminateOnResume.fromMaybe(false));
10411042
return Response::OK();
10421043
}
10431044

‎src/inspector/v8-debugger-agent-impl.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
9898
Response getWasmBytecode(const String16& scriptId,
9999
protocol::Binary* bytecode) override;
100100
Response pause() override;
101-
Response resume() override;
101+
Response resume(Maybe<bool> terminateOnResume) override;
102102
Response stepOver() override;
103103
Response stepInto(Maybe<bool> inBreakOnAsyncCall) override;
104104
Response stepOut() override;

‎src/inspector/v8-debugger.cc‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,15 @@ void V8Debugger::interruptAndBreak(int targetContextGroupId) {
245245
nullptr);
246246
}
247247

248-
void V8Debugger::continueProgram(int targetContextGroupId) {
248+
void V8Debugger::continueProgram(int targetContextGroupId,
249+
bool terminateOnResume) {
249250
if (m_pausedContextGroupId != targetContextGroupId) return;
250-
if (isPaused()) m_inspector->client()->quitMessageLoopOnPause();
251+
if (isPaused()) {
252+
if (terminateOnResume) {
253+
v8::debug::SetTerminateOnResume(m_isolate);
254+
}
255+
m_inspector->client()->quitMessageLoopOnPause();
256+
}
251257
}
252258

253259
void V8Debugger::breakProgramOnAssert(int targetContextGroupId) {

0 commit comments

Comments
 (0)