From d821a6a373ecf086a2ef0d233ace7f3431e47732 Mon Sep 17 00:00:00 2001
From: Benedikt Meurer <bmeurer@chromium.org>
Date: Fri, 29 Apr 2022 13:07:01 +0200
Subject: [PATCH] [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}
---
 src/debug/debug-interface.cc                  | 105 +++++++++++-------
 src/debug/debug-interface.h                   |   5 +-
 src/inspector/v8-debugger-agent-impl.cc       |  18 +--
 src/inspector/v8-debugger-script.cc           |   2 +-
 src/inspector/v8-debugger-script.h            |   3 +-
 test/cctest/test-debug.cc                     |  46 +++++---
 .../debugger/breakpoints-expected.txt         |   2 +-
 test/inspector/debugger/breakpoints.js        |   2 +-
 .../regress-crbug-1319828-expected.txt        |  18 +++
 .../debugger/regress-crbug-1319828.js         |  65 +++++++++++
 10 files changed, 191 insertions(+), 75 deletions(-)
 create mode 100644 test/inspector/debugger/regress-crbug-1319828-expected.txt
 create mode 100644 test/inspector/debugger/regress-crbug-1319828.js

diff --git a/src/debug/debug-interface.cc b/src/debug/debug-interface.cc
index 90207eae12..a2d7e42265 100644
--- a/src/debug/debug-interface.cc
+++ b/src/debug/debug-interface.cc
@@ -535,16 +535,17 @@ bool Script::GetPossibleBreakpoints(
 #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 @@ bool Script::GetPossibleBreakpoints(
   }
 
   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 @@ Location Script::GetSourceLocation(int offset) const {
   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 @@ bool Script::SetBreakpoint(Local<String> condition, Location* location,
                            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 4f6e7c663d..0bce930cec 100644
--- a/src/debug/debug-interface.h
+++ b/src/debug/debug-interface.h
@@ -215,7 +215,10 @@ class V8_EXPORT_PRIVATE Script {
       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 7f6ca65320..112bd3cb0f 100644
--- a/src/inspector/v8-debugger-agent-impl.cc
+++ b/src/inspector/v8-debugger-agent-impl.cc
@@ -180,8 +180,8 @@ bool positionComparator(const std::pair<int, int>& a,
 
 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 @@ void adjustBreakpointLocation(const V8DebuggerScript& script,
   }
 
   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 @@ V8DebuggerAgentImpl::setBreakpointImpl(const String16& breakpointId,
   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 d115912f82..2b295e76ca 100644
--- a/src/inspector/v8-debugger-script.cc
+++ b/src/inspector/v8-debugger-script.cc
@@ -231,7 +231,7 @@ class ActualScript : public V8DebuggerScript {
     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 eb80a14c04..e4823e2d58 100644
--- a/src/inspector/v8-debugger-script.h
+++ b/src/inspector/v8-debugger-script.h
@@ -90,8 +90,7 @@ class V8DebuggerScript {
       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 4c70451d59..aceaae552f 100644
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -4776,26 +4776,40 @@ TEST(SourceInfo) {
   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 9d0bae5666..c9b59e3441 100644
--- a/test/inspector/debugger/breakpoints-expected.txt
+++ b/test/inspector/debugger/breakpoints-expected.txt
@@ -63,4 +63,4 @@ Adding breakpoint
 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 ce9ab47665..261f67fd7e 100644
--- a/test/inspector/debugger/breakpoints.js
+++ b/test/inspector/debugger/breakpoints.js
@@ -93,7 +93,7 @@ function foo2(arg) {
 }
 //# 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 0000000000..f1742e1608
--- /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 0000000000..72bf5c8a69
--- /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(),
+    ]);
+  },
+]);