[inspector] Fix mapping between location and offset.
We weren't really translating between location (line and column number)
and source position (character offset) consistently, especially when it
came to inline <script>s. There were also inconsistencies between what
Debugger.getPossibleBreakpoints and Debugger.setBreakpointByUrl would
do.
With this CL, we are now consistently operating under the following
assumptions:
(1) For inline <scripts>s with a //@ sourceURL annotation, we assume
that the line and column number that comes in via the protocol is
in terms of the source text of the script.
(2) For inline <script>s without said annotation, we assume that the
line and column numbers are in terms of the surrounding document.
This is finally aligned with how the DevTools front-end operates.
Fixed: chromium:1319828
Change-Id: I98c4ef04b34a97caf060ff4f32690b135edb6ee6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3610622
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80292}
diff --git a/src/debug/debug-interface.cc b/src/debug/debug-interface.cc
index 90207ea..a2d7e42 100644
--- a/src/debug/debug-interface.cc
+++ b/src/debug/debug-interface.cc
@@ -535,16 +535,17 @@
#endif // V8_ENABLE_WEBASSEMBLY
i::Isolate* isolate = script->GetIsolate();
- i::Script::InitLineEnds(isolate, script);
- CHECK(script->line_ends().IsFixedArray());
- i::Handle<i::FixedArray> line_ends =
- i::Handle<i::FixedArray>::cast(i::handle(script->line_ends(), isolate));
- CHECK(line_ends->length());
- int start_offset = GetSourceOffset(start);
- int end_offset = end.IsEmpty()
- ? GetSmiValue(line_ends, line_ends->length() - 1) + 1
- : GetSourceOffset(end);
+ int start_offset, end_offset;
+ if (!GetSourceOffset(start, GetSourceOffsetMode::kClamp).To(&start_offset)) {
+ return false;
+ }
+ if (end.IsEmpty()) {
+ end_offset = std::numeric_limits<int>::max();
+ } else if (!GetSourceOffset(end, GetSourceOffsetMode::kClamp)
+ .To(&end_offset)) {
+ return false;
+ }
if (start_offset >= end_offset) return true;
std::vector<i::BreakLocation> v8_locations;
@@ -555,53 +556,72 @@
}
std::sort(v8_locations.begin(), v8_locations.end(), CompareBreakLocation);
- int current_line_end_index = 0;
for (const auto& v8_location : v8_locations) {
- int offset = v8_location.position();
- while (offset > GetSmiValue(line_ends, current_line_end_index)) {
- ++current_line_end_index;
- CHECK(current_line_end_index < line_ends->length());
- }
- int line_offset = 0;
-
- if (current_line_end_index > 0) {
- line_offset = GetSmiValue(line_ends, current_line_end_index - 1) + 1;
- }
- locations->emplace_back(
- current_line_end_index + script->line_offset(),
- offset - line_offset +
- (current_line_end_index == 0 ? script->column_offset() : 0),
- v8_location.type());
+ Location location = GetSourceLocation(v8_location.position());
+ locations->emplace_back(location.GetLineNumber(),
+ location.GetColumnNumber(), v8_location.type());
}
return true;
}
-int Script::GetSourceOffset(const Location& location) const {
+Maybe<int> Script::GetSourceOffset(const Location& location,
+ GetSourceOffsetMode mode) const {
i::Handle<i::Script> script = Utils::OpenHandle(this);
#if V8_ENABLE_WEBASSEMBLY
if (script->type() == i::Script::TYPE_WASM) {
DCHECK_EQ(0, location.GetLineNumber());
- return location.GetColumnNumber();
+ return Just(location.GetColumnNumber());
}
#endif // V8_ENABLE_WEBASSEMBLY
- int line = std::max(location.GetLineNumber() - script->line_offset(), 0);
+ int line = location.GetLineNumber();
int column = location.GetColumnNumber();
- if (line == 0) {
- column = std::max(0, column - script->column_offset());
+ if (!script->HasSourceURLComment()) {
+ // Line/column number for inline <script>s with sourceURL annotation
+ // are supposed to be related to the <script> tag, otherwise they
+ // are relative to the parent file. Keep this in sync with the logic
+ // in GetSourceLocation() below.
+ line -= script->line_offset();
+ if (line == 0) column -= script->column_offset();
}
i::Script::InitLineEnds(script->GetIsolate(), script);
- CHECK(script->line_ends().IsFixedArray());
i::Handle<i::FixedArray> line_ends = i::Handle<i::FixedArray>::cast(
i::handle(script->line_ends(), script->GetIsolate()));
- CHECK(line_ends->length());
- if (line >= line_ends->length())
- return GetSmiValue(line_ends, line_ends->length() - 1);
- int line_offset = GetSmiValue(line_ends, line);
- if (line == 0) return std::min(column, line_offset);
- int prev_line_offset = GetSmiValue(line_ends, line - 1);
- return std::min(prev_line_offset + column + 1, line_offset);
+ if (line < 0) {
+ if (mode == GetSourceOffsetMode::kClamp) {
+ return Just(0);
+ }
+ return Nothing<int>();
+ }
+ if (line >= line_ends->length()) {
+ if (mode == GetSourceOffsetMode::kClamp) {
+ return Just(GetSmiValue(line_ends, line_ends->length() - 1));
+ }
+ return Nothing<int>();
+ }
+ if (column < 0) {
+ if (mode != GetSourceOffsetMode::kClamp) {
+ return Nothing<int>();
+ }
+ column = 0;
+ }
+ int offset = column;
+ if (line > 0) {
+ int prev_line_end_offset = GetSmiValue(line_ends, line - 1);
+ offset += prev_line_end_offset + 1;
+ }
+ int line_end_offset = GetSmiValue(line_ends, line);
+ if (offset > line_end_offset) {
+ // Be permissive with columns that don't exist,
+ // as long as they are clearly within the range
+ // of the script.
+ if (line < line_ends->length() - 1 || mode == GetSourceOffsetMode::kClamp) {
+ return Just(line_end_offset);
+ }
+ return Nothing<int>();
+ }
+ return Just(offset);
}
Location Script::GetSourceLocation(int offset) const {
@@ -609,6 +629,10 @@
i::Script::PositionInfo info;
i::Script::GetPositionInfo(script, offset, &info, i::Script::WITH_OFFSET);
if (script->HasSourceURLComment()) {
+ // Line/column number for inline <script>s with sourceURL annotation
+ // are supposed to be related to the <script> tag, otherwise they
+ // are relative to the parent file. Keep this in sync with the logic
+ // in GetSourceOffset() above.
info.line -= script->line_offset();
if (info.line == 0) info.column -= script->column_offset();
}
@@ -627,7 +651,10 @@
BreakpointId* id) const {
i::Handle<i::Script> script = Utils::OpenHandle(this);
i::Isolate* isolate = script->GetIsolate();
- int offset = GetSourceOffset(*location);
+ int offset;
+ if (!GetSourceOffset(*location).To(&offset)) {
+ return false;
+ }
if (!isolate->debug()->SetBreakPointForScript(
script, Utils::OpenHandle(*condition), &offset, id)) {
return false;
diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h
index 4f6e7c6..0bce930 100644
--- a/src/debug/debug-interface.h
+++ b/src/debug/debug-interface.h
@@ -215,7 +215,10 @@
const debug::Location& start, const debug::Location& end,
bool restrict_to_function,
std::vector<debug::BreakLocation>* locations) const;
- int GetSourceOffset(const debug::Location& location) const;
+ enum class GetSourceOffsetMode { kStrict, kClamp };
+ Maybe<int> GetSourceOffset(
+ const debug::Location& location,
+ GetSourceOffsetMode mode = GetSourceOffsetMode::kStrict) const;
v8::debug::Location GetSourceLocation(int offset) const;
bool SetScriptSource(v8::Local<v8::String> newSource, bool preview,
LiveEditResult* result) const;
diff --git a/src/inspector/v8-debugger-agent-impl.cc b/src/inspector/v8-debugger-agent-impl.cc
index 7f6ca65..112bd3c 100644
--- a/src/inspector/v8-debugger-agent-impl.cc
+++ b/src/inspector/v8-debugger-agent-impl.cc
@@ -180,8 +180,8 @@
String16 breakpointHint(const V8DebuggerScript& script, int lineNumber,
int columnNumber) {
- int offset = script.offset(lineNumber, columnNumber);
- if (offset == V8DebuggerScript::kNoOffset) return String16();
+ int offset;
+ if (!script.offset(lineNumber, columnNumber).To(&offset)) return String16();
String16 hint =
script.source(offset, kBreakpointHintMaxLength).stripWhiteSpace();
for (size_t i = 0; i < hint.length(); ++i) {
@@ -206,8 +206,8 @@
}
if (hint.isEmpty()) return;
- intptr_t sourceOffset = script.offset(*lineNumber, *columnNumber);
- if (sourceOffset == V8DebuggerScript::kNoOffset) return;
+ int sourceOffset;
+ if (!script.offset(*lineNumber, *columnNumber).To(&sourceOffset)) return;
intptr_t searchRegionOffset = std::max(
sourceOffset - kBreakpointHintMaxSearchOffset, static_cast<intptr_t>(0));
@@ -948,16 +948,6 @@
ScriptsMap::iterator scriptIterator = m_scripts.find(scriptId);
if (scriptIterator == m_scripts.end()) return nullptr;
V8DebuggerScript* script = scriptIterator->second.get();
- if (lineNumber < script->startLine() || script->endLine() < lineNumber) {
- return nullptr;
- }
- if (lineNumber == script->startLine() &&
- columnNumber < script->startColumn()) {
- return nullptr;
- }
- if (lineNumber == script->endLine() && script->endColumn() < columnNumber) {
- return nullptr;
- }
v8::debug::BreakpointId debuggerBreakpointId;
v8::debug::Location location(lineNumber, columnNumber);
diff --git a/src/inspector/v8-debugger-script.cc b/src/inspector/v8-debugger-script.cc
index d115912..2b295e7 100644
--- a/src/inspector/v8-debugger-script.cc
+++ b/src/inspector/v8-debugger-script.cc
@@ -231,7 +231,7 @@
v8::debug::ResetBlackboxedStateCache(m_isolate, m_script.Get(m_isolate));
}
- int offset(int lineNumber, int columnNumber) const override {
+ v8::Maybe<int> offset(int lineNumber, int columnNumber) const override {
v8::HandleScope scope(m_isolate);
return m_script.Get(m_isolate)->GetSourceOffset(
v8::debug::Location(lineNumber, columnNumber));
diff --git a/src/inspector/v8-debugger-script.h b/src/inspector/v8-debugger-script.h
index eb80a14..e4823e2 100644
--- a/src/inspector/v8-debugger-script.h
+++ b/src/inspector/v8-debugger-script.h
@@ -90,8 +90,7 @@
std::vector<v8::debug::BreakLocation>* locations) = 0;
virtual void resetBlackboxedStateCache() = 0;
- static const int kNoOffset = -1;
- virtual int offset(int lineNumber, int columnNumber) const = 0;
+ virtual v8::Maybe<int> offset(int lineNumber, int columnNumber) const = 0;
virtual v8::debug::Location location(int offset) const = 0;
virtual bool setBreakpoint(const String16& condition,
diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc
index 4c70451..aceaae5 100644
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -4776,26 +4776,40 @@
CHECK_EQ(script->GetSourceLocation(start_d).GetColumnNumber(), 10);
// Test offsets.
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(1, 10)), start_a);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(2, 13)), start_b);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(3, 0)), start_b + 5);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(3, 2)), start_b + 7);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(4, 0)), start_b + 16);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(5, 12)), start_c);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(6, 0)), start_c + 6);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(7, 0)), start_c + 19);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(8, 0)), start_c + 35);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(9, 0)), start_c + 48);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(10, 0)), start_c + 64);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(11, 0)), start_c + 70);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(12, 10)), start_d);
- CHECK_EQ(script->GetSourceOffset(v8::debug::Location(13, 0)), start_d + 6);
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(1, 10)),
+ v8::Just(start_a));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(2, 13)),
+ v8::Just(start_b));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(3, 0)),
+ v8::Just(start_b + 5));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(3, 2)),
+ v8::Just(start_b + 7));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(4, 0)),
+ v8::Just(start_b + 16));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(5, 12)),
+ v8::Just(start_c));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(6, 0)),
+ v8::Just(start_c + 6));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(7, 0)),
+ v8::Just(start_c + 19));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(8, 0)),
+ v8::Just(start_c + 35));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(9, 0)),
+ v8::Just(start_c + 48));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(10, 0)),
+ v8::Just(start_c + 64));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(11, 0)),
+ v8::Just(start_c + 70));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(12, 10)),
+ v8::Just(start_d));
+ CHECK_EQ(script->GetSourceOffset(v8::debug::Location(13, 0)),
+ v8::Just(start_d + 6));
for (int i = 1; i <= num_lines_d; ++i) {
CHECK_EQ(script->GetSourceOffset(v8::debug::Location(start_line_d + i, 0)),
- 6 + (i * line_length_d) + start_d);
+ v8::Just(6 + (i * line_length_d) + start_d));
}
CHECK_EQ(script->GetSourceOffset(v8::debug::Location(start_line_d + 17, 0)),
- start_d + 158);
+ v8::Nothing<int>());
// Make sure invalid inputs work properly.
const int last_position = static_cast<int>(strlen(source)) - 1;
diff --git a/test/inspector/debugger/breakpoints-expected.txt b/test/inspector/debugger/breakpoints-expected.txt
index 9d0bae5..c9b59e3 100644
--- a/test/inspector/debugger/breakpoints-expected.txt
+++ b/test/inspector/debugger/breakpoints-expected.txt
@@ -63,4 +63,4 @@
evaluating foo1(0):
hit expected breakpoint
evaluating foo2(0):
- not paused
+ hit expected breakpoint
diff --git a/test/inspector/debugger/breakpoints.js b/test/inspector/debugger/breakpoints.js
index ce9ab47..261f67f 100644
--- a/test/inspector/debugger/breakpoints.js
+++ b/test/inspector/debugger/breakpoints.js
@@ -93,7 +93,7 @@
}
//# sourceURL=test2.js`, 5);
await evaluate('foo1(0)', breakpointId);
- await evaluate('foo2(0)');
+ await evaluate('foo2(0)', breakpointId);
}
]);
diff --git a/test/inspector/debugger/regress-crbug-1319828-expected.txt b/test/inspector/debugger/regress-crbug-1319828-expected.txt
new file mode 100644
index 0000000..f1742e1
--- /dev/null
+++ b/test/inspector/debugger/regress-crbug-1319828-expected.txt
@@ -0,0 +1,18 @@
+Regression test for crbug.com/1319828
+
+Running test: testDebuggerGetPossibleBreakpoints
+
+function foo() {
+ |_|console.|C|log('Hello World!');
+|R|}
+
+
+Running test: testDebuggerSetBreakpointByUrl
+function foo() {
+ #console.log('Hello World!');
+}
+
+function foo() {
+ console.#log('Hello World!');
+}
+
diff --git a/test/inspector/debugger/regress-crbug-1319828.js b/test/inspector/debugger/regress-crbug-1319828.js
new file mode 100644
index 0000000..72bf5c8
--- /dev/null
+++ b/test/inspector/debugger/regress-crbug-1319828.js
@@ -0,0 +1,65 @@
+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+let {session, contextGroup, Protocol} =
+ InspectorTest.start('Regression test for crbug.com/1319828');
+
+const source = `
+function foo() {
+ console.log('Hello World!');
+}
+//# sourceURL=foo.js`;
+contextGroup.addScript(source, 42, 3);
+session.setupScriptMap();
+
+InspectorTest.runAsyncTestSuite([
+ async function testDebuggerGetPossibleBreakpoints() {
+ const [{params: {scriptId}}] = await Promise.all([
+ Protocol.Debugger.onceScriptParsed(),
+ Protocol.Runtime.enable(),
+ Protocol.Debugger.enable(),
+ ]);
+ const {result: {locations}} =
+ await Protocol.Debugger.getPossibleBreakpoints({
+ start: {lineNumber: 1, columnNumber: 0, scriptId},
+ end: {lineNumber: 3, columnNumber: 1, scriptId},
+ });
+ await session.logBreakLocations(locations);
+ await Promise.all([
+ Protocol.Debugger.disable(),
+ Protocol.Runtime.disable(),
+ ]);
+ },
+
+ async function testDebuggerSetBreakpointByUrl() {
+ await Promise.all([
+ Protocol.Runtime.enable(),
+ Protocol.Debugger.enable(),
+ ]);
+ let {result: {breakpointId, locations}} =
+ await Protocol.Debugger.setBreakpointByUrl({
+ url: 'foo.js',
+ lineNumber: 2,
+ columnNumber: 0,
+ });
+ await Promise.all([
+ session.logSourceLocations(locations),
+ Protocol.Debugger.removeBreakpoint({breakpointId}),
+ ]);
+ ({result: {breakpointId, locations}} =
+ await Protocol.Debugger.setBreakpointByUrl({
+ url: 'foo.js',
+ lineNumber: 2,
+ columnNumber: 9,
+ }));
+ await Promise.all([
+ session.logSourceLocations(locations),
+ Protocol.Debugger.removeBreakpoint({breakpointId}),
+ ]);
+ await Promise.all([
+ Protocol.Debugger.disable(),
+ Protocol.Runtime.disable(),
+ ]);
+ },
+]);
