[inspector] restore provisional breakpoints smarter

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

 // Copyright 2015 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.
 
 #include "src/inspector/v8-debugger-agent-impl.h"
 
 #include <algorithm>
 
 #include "src/debug/debug-interface.h"
 #include "src/inspector/injected-script.h"
@@ skipping 32 matching lines @@
 static const char asyncCallStackDepth[] = "asyncCallStackDepth";
 static const char blackboxPattern[] = "blackboxPattern";
 static const char debuggerEnabled[] = "debuggerEnabled";
 
 // Breakpoint properties.
 static const char url[] = "url";
 static const char isRegex[] = "isRegex";
 static const char lineNumber[] = "lineNumber";
 static const char columnNumber[] = "columnNumber";
 static const char condition[] = "condition";
+static const char sourceLineHint[] = "sourceLineHint";
+
 static const char skipAllPauses[] = "skipAllPauses";
 
 }  // namespace DebuggerAgentState
 
 static const char kBacktraceObjectGroup[] = "backtrace";
 static const char kDebuggerNotEnabled[] = "Debugger agent is not enabled";
 static const char kDebuggerNotPaused[] =
     "Can only perform operation while paused.";
+static const int kSourceLineHintToCheckOffsets[] = {

[pfeldman, 2017/02/07 19:13:05]

:) I'm sure alph@ would suggest to iterate to 16 and check everything for "i"
and "-i" or something 

[alph, 2017/02/07 22:22:30]

The problem is that you'd then double check the zero. ;-)

+    0, 1, -1, 2, -2, 3, -3, 4, -4, 5, -5, 6, -6, 7, -7, 8, -8, 9, -9, 10, -10,
+    11, -11, 12, -12, 13, -13, 14, -14, 15, -15, 16, -16};
+static const int kSourceLineHintMaxLength = 128;
 
 namespace {
 
 void TranslateWasmStackTraceLocations(Array<CallFrame>* stackTrace,
                                       WasmTranslation* wasmTranslation) {
   for (size_t i = 0, e = stackTrace->length(); i != e; ++i) {
     protocol::Debugger::Location* location = stackTrace->get(i)->getLocation();
     String16 scriptId = location->getScriptId();
     int lineNumber = location->getLineNumber();
     int columnNumber = location->getColumnNumber(-1);
@@ skipping 163 matching lines @@
 
 Response V8DebuggerAgentImpl::setSkipAllPauses(bool skip) {
   m_state->setBoolean(DebuggerAgentState::skipAllPauses, skip);
   m_skipAllPauses = skip;
   return Response::OK();
 }
 
 static std::unique_ptr<protocol::DictionaryValue>
 buildObjectForBreakpointCookie(const String16& url, int lineNumber,
                                int columnNumber, const String16& condition,
-                               bool isRegex) {
+                               bool isRegex, const String16& sourceLineHint) {
   std::unique_ptr<protocol::DictionaryValue> breakpointObject =
       protocol::DictionaryValue::create();
   breakpointObject->setString(DebuggerAgentState::url, url);
   breakpointObject->setInteger(DebuggerAgentState::lineNumber, lineNumber);
   breakpointObject->setInteger(DebuggerAgentState::columnNumber, columnNumber);
   breakpointObject->setString(DebuggerAgentState::condition, condition);
+  breakpointObject->setString(DebuggerAgentState::sourceLineHint,
+                              sourceLineHint);
   breakpointObject->setBoolean(DebuggerAgentState::isRegex, isRegex);
   return breakpointObject;
 }
 
 static bool matches(V8InspectorImpl* inspector, const String16& url,
                     const String16& pattern, bool isRegex) {
   if (isRegex) {
     V8Regex regex(inspector, pattern, true);
     return regex.match(url) != -1;
   }
@@ skipping 27 matching lines @@
   if (!breakpointsCookie) {
     std::unique_ptr<protocol::DictionaryValue> newValue =
         protocol::DictionaryValue::create();
     breakpointsCookie = newValue.get();
     m_state->setObject(DebuggerAgentState::javaScriptBreakpoints,
                        std::move(newValue));
   }
   if (breakpointsCookie->get(breakpointId))
     return Response::Error("Breakpoint at specified location already exists.");
 
-  breakpointsCookie->setObject(
-      breakpointId, buildObjectForBreakpointCookie(
-                        url, lineNumber, columnNumber, condition, isRegex));
-
   ScriptBreakpoint breakpoint(String16(), lineNumber, columnNumber, condition);
+  String16 sourceLineHint;
   for (const auto& script : m_scripts) {
     if (!matches(m_inspector, script.second->sourceURL(), url, isRegex))
       continue;
     breakpoint.script_id = script.first;
-    std::unique_ptr<protocol::Debugger::Location> location =
-        resolveBreakpoint(breakpointId, breakpoint, UserBreakpointSource);
+    sourceLineHint =
+        script.second->lineAt(lineNumber, kSourceLineHintMaxLength);
+    std::unique_ptr<protocol::Debugger::Location> location = resolveBreakpoint(
+        breakpointId, breakpoint, UserBreakpointSource, String16());
     if (location) (*locations)->addItem(std::move(location));
   }
 
+  breakpointsCookie->setObject(
+      breakpointId,
+      buildObjectForBreakpointCookie(url, lineNumber, columnNumber, condition,
+                                     isRegex, sourceLineHint));
+
   *outBreakpointId = breakpointId;
   return Response::OK();
 }
 
 Response V8DebuggerAgentImpl::setBreakpoint(
     std::unique_ptr<protocol::Debugger::Location> location,
     Maybe<String16> optionalCondition, String16* outBreakpointId,
     std::unique_ptr<protocol::Debugger::Location>* actualLocation) {
   ScriptBreakpoint breakpoint(
       location->getScriptId(), location->getLineNumber(),
       location->getColumnNumber(0), optionalCondition.fromMaybe(String16()));
 
   String16 breakpointId =
       generateBreakpointId(breakpoint, UserBreakpointSource);
   if (m_breakpointIdToDebuggerBreakpointIds.find(breakpointId) !=
       m_breakpointIdToDebuggerBreakpointIds.end()) {
     return Response::Error("Breakpoint at specified location already exists.");
   }
-  *actualLocation =
-      resolveBreakpoint(breakpointId, breakpoint, UserBreakpointSource);
+  *actualLocation = resolveBreakpoint(breakpointId, breakpoint,
+                                      UserBreakpointSource, String16());
   if (!*actualLocation) return Response::Error("Could not resolve breakpoint");
   *outBreakpointId = breakpointId;
   return Response::OK();
 }
 
 Response V8DebuggerAgentImpl::removeBreakpoint(const String16& breakpointId) {
   if (!enabled()) return Response::Error(kDebuggerNotEnabled);
   protocol::DictionaryValue* breakpointsCookie =
       m_state->getObject(DebuggerAgentState::javaScriptBreakpoints);
   if (breakpointsCookie) breakpointsCookie->remove(breakpointId);
@@ skipping 109 matching lines @@
   // Ranges array contains positions in script where blackbox state is changed.
   // [(0,0) ... ranges[0]) isn't blackboxed, [ranges[0] ... ranges[1]) is
   // blackboxed...
   return itStartRange == itEndRange &&
          std::distance(ranges.begin(), itStartRange) % 2;
 }
 
 std::unique_ptr<protocol::Debugger::Location>
 V8DebuggerAgentImpl::resolveBreakpoint(const String16& breakpointId,
                                        const ScriptBreakpoint& breakpoint,
-                                       BreakpointSource source) {
+                                       BreakpointSource source,
+                                       const String16& sourceLineHint) {
   v8::HandleScope handles(m_isolate);
   DCHECK(enabled());
   // FIXME: remove these checks once crbug.com/520702 is resolved.
   CHECK(!breakpointId.isEmpty());
   CHECK(!breakpoint.script_id.isEmpty());
   ScriptsMap::iterator scriptIterator = m_scripts.find(breakpoint.script_id);
   if (scriptIterator == m_scripts.end()) return nullptr;
-  if (breakpoint.line_number < scriptIterator->second->startLine() ||
-      scriptIterator->second->endLine() < breakpoint.line_number)
+
+  V8DebuggerScript* script = scriptIterator->second.get();
+  ScriptBreakpoint translatedBreakpoint = breakpoint;
+  if (!sourceLineHint.isEmpty()) {

[pfeldman, 2017/02/07 19:13:05]

Extract into the method that adjusts ScriptBreakpoint line based on the content.
Also, what if column was not 0? Should you compare suffix only? Should you scan
script based on offsets, not lines?

[kozy, 2017/02/08 01:32:56]

I'm not sure. User puts breakpoint at line and if store from offset to next end
line then we will get more collisions but support some rare cases when only
first part of string was changed (could be useful maybe for minified source, but
we have much better solution with formatted script). If we store N symbols from
offset then changing next line will not allow us to restore breakpoint.

+    for (size_t i = 0; i < arraysize(kSourceLineHintToCheckOffsets); ++i) {

[pfeldman, 2017/02/07 19:13:05]

Yeah...

+      int lineNumber =
+          translatedBreakpoint.line_number + kSourceLineHintToCheckOffsets[i];
+      if (lineNumber < script->startLine() || lineNumber > script->endLine()) {

[alph, 2017/02/07 22:22:30]

no {}

[kozy, 2017/02/08 01:32:56]

yes {}, it's V8 code style.

+        continue;
+      }
+      if (script->lineAt(lineNumber, kSourceLineHintMaxLength) ==
+          sourceLineHint) {
+        translatedBreakpoint.line_number = lineNumber;
+        break;
+      }
+    }
+  }
+
+  if (breakpoint.line_number < script->startLine() ||
+      script->endLine() < breakpoint.line_number) {
     return nullptr;
+  }
 
   // Translate from protocol location to v8 location for the debugger.
-  ScriptBreakpoint translatedBreakpoint = breakpoint;
   m_debugger->wasmTranslation()->TranslateProtocolLocationToWasmScriptLocation(
       &translatedBreakpoint.script_id, &translatedBreakpoint.line_number,
       &translatedBreakpoint.column_number);
 
   int actualLineNumber;
   int actualColumnNumber;
   String16 debuggerBreakpointId = m_debugger->setBreakpoint(
       translatedBreakpoint, &actualLineNumber, &actualColumnNumber);
   if (debuggerBreakpointId.isEmpty()) return nullptr;
 
@@ skipping 14 matching lines @@
 Response V8DebuggerAgentImpl::searchInContent(
     const String16& scriptId, const String16& query,
     Maybe<bool> optionalCaseSensitive, Maybe<bool> optionalIsRegex,
     std::unique_ptr<Array<protocol::Debugger::SearchMatch>>* results) {
   v8::HandleScope handles(m_isolate);
   ScriptsMap::iterator it = m_scripts.find(scriptId);
   if (it == m_scripts.end())
     return Response::Error("No script for id: " + scriptId);
 
   std::vector<std::unique_ptr<protocol::Debugger::SearchMatch>> matches =
-      searchInTextByLinesImpl(m_session, it->second->source(m_isolate), query,
+      searchInTextByLinesImpl(m_session, it->second->source(), query,
                               optionalCaseSensitive.fromMaybe(false),
                               optionalIsRegex.fromMaybe(false));
   *results = protocol::Array<protocol::Debugger::SearchMatch>::create();
   for (size_t i = 0; i < matches.size(); ++i)
     (*results)->addItem(std::move(matches[i]));
   return Response::OK();
 }
 
 Response V8DebuggerAgentImpl::setScriptSource(
     const String16& scriptId, const String16& newContent, Maybe<bool> dryRun,
@@ skipping 58 matching lines @@
   return Response::OK();
 }
 
 Response V8DebuggerAgentImpl::getScriptSource(const String16& scriptId,
                                               String16* scriptSource) {
   if (!enabled()) return Response::Error(kDebuggerNotEnabled);
   ScriptsMap::iterator it = m_scripts.find(scriptId);
   if (it == m_scripts.end())
     return Response::Error("No script for id: " + scriptId);
   v8::HandleScope handles(m_isolate);
-  *scriptSource = it->second->source(m_isolate);
+  *scriptSource = it->second->source();
   return Response::OK();
 }
 
 void V8DebuggerAgentImpl::schedulePauseOnNextStatement(
     const String16& breakReason,
     std::unique_ptr<protocol::DictionaryValue> data) {
   if (!enabled() || m_scheduledDebuggerStep == StepInto ||
       m_javaScriptPauseScheduled || isPaused() ||
       !m_debugger->breakpointsActivated())
     return;
@@ skipping 373 matching lines @@
   V8StackTraceImpl* stackTrace = m_debugger->currentAsyncCallChain();
   return stackTrace ? stackTrace->buildInspectorObjectForTail(m_debugger)
                     : nullptr;
 }
 
 bool V8DebuggerAgentImpl::isPaused() const { return m_debugger->isPaused(); }
 
 void V8DebuggerAgentImpl::didParseSource(
     std::unique_ptr<V8DebuggerScript> script, bool success) {
   v8::HandleScope handles(m_isolate);
-  String16 scriptSource = script->source(m_isolate);
+  String16 scriptSource = script->source();
   if (!success) script->setSourceURL(findSourceURL(scriptSource, false));
   if (!success)
     script->setSourceMappingURL(findSourceMapURL(scriptSource, false));
 
   int contextId = script->executionContextId();
   int contextGroupId = m_inspector->contextGroupId(contextId);
   InspectedContext* inspected =
       m_inspector->getContext(contextGroupId, contextId);
   std::unique_ptr<protocol::DictionaryValue> executionContextAuxData;
   if (inspected) {
@@ skipping 22 matching lines @@
   Maybe<String16> sourceMapURLParam = scriptRef->sourceMappingURL();
   Maybe<protocol::DictionaryValue> executionContextAuxDataParam(
       std::move(executionContextAuxData));
   const bool* isLiveEditParam = isLiveEdit ? &isLiveEdit : nullptr;
   const bool* hasSourceURLParam = hasSourceURL ? &hasSourceURL : nullptr;
   const bool* isModuleParam = isModule ? &isModule : nullptr;
   if (success)
     m_frontend.scriptParsed(
         scriptId, scriptURL, scriptRef->startLine(), scriptRef->startColumn(),
         scriptRef->endLine(), scriptRef->endColumn(), contextId,
-        scriptRef->hash(m_isolate), std::move(executionContextAuxDataParam),
+        scriptRef->hash(), std::move(executionContextAuxDataParam),
         isLiveEditParam, std::move(sourceMapURLParam), hasSourceURLParam,
         isModuleParam);
   else
     m_frontend.scriptFailedToParse(
         scriptId, scriptURL, scriptRef->startLine(), scriptRef->startColumn(),
         scriptRef->endLine(), scriptRef->endColumn(), contextId,
-        scriptRef->hash(m_isolate), std::move(executionContextAuxDataParam),
+        scriptRef->hash(), std::move(executionContextAuxDataParam),
         std::move(sourceMapURLParam), hasSourceURLParam, isModuleParam);
 
   if (scriptURL.isEmpty() || !success) return;
 
   protocol::DictionaryValue* breakpointsCookie =
       m_state->getObject(DebuggerAgentState::javaScriptBreakpoints);
   if (!breakpointsCookie) return;
 
   for (size_t i = 0; i < breakpointsCookie->size(); ++i) {
     auto cookie = breakpointsCookie->at(i);
     protocol::DictionaryValue* breakpointObject =
         protocol::DictionaryValue::cast(cookie.second);
     bool isRegex;
     breakpointObject->getBoolean(DebuggerAgentState::isRegex, &isRegex);
     String16 url;
+    String16 sourceLineHint;
     breakpointObject->getString(DebuggerAgentState::url, &url);
     if (!matches(m_inspector, scriptURL, url, isRegex)) continue;
     ScriptBreakpoint breakpoint;
     breakpoint.script_id = scriptId;
     breakpointObject->getInteger(DebuggerAgentState::lineNumber,
                                  &breakpoint.line_number);
     breakpointObject->getInteger(DebuggerAgentState::columnNumber,
                                  &breakpoint.column_number);
     breakpointObject->getString(DebuggerAgentState::condition,
                                 &breakpoint.condition);
-    std::unique_ptr<protocol::Debugger::Location> location =
-        resolveBreakpoint(cookie.first, breakpoint, UserBreakpointSource);
+    breakpointObject->getString(DebuggerAgentState::sourceLineHint,
+                                &sourceLineHint);
+    std::unique_ptr<protocol::Debugger::Location> location = resolveBreakpoint(
+        cookie.first, breakpoint, UserBreakpointSource, sourceLineHint);
     if (location)
       m_frontend.breakpointResolved(cookie.first, std::move(location));
   }
 }
 
 void V8DebuggerAgentImpl::didPause(int contextId,
                                    v8::Local<v8::Value> exception,
                                    const std::vector<String16>& hitBreakpoints,
                                    bool isPromiseRejection, bool isUncaught,
                                    bool isOOMBreak) {
@@ skipping 86 matching lines @@
   m_breakReason = protocol::Debugger::Paused::ReasonEnum::Other;
   m_breakAuxData = nullptr;
 }
 
 void V8DebuggerAgentImpl::setBreakpointAt(const String16& scriptId,
                                           int lineNumber, int columnNumber,
                                           BreakpointSource source,
                                           const String16& condition) {
   ScriptBreakpoint breakpoint(scriptId, lineNumber, columnNumber, condition);
   String16 breakpointId = generateBreakpointId(breakpoint, source);
-  resolveBreakpoint(breakpointId, breakpoint, source);
+  resolveBreakpoint(breakpointId, breakpoint, source, String16());
 }
 
 void V8DebuggerAgentImpl::removeBreakpointAt(const String16& scriptId,
                                              int lineNumber, int columnNumber,
                                              BreakpointSource source) {
   removeBreakpointImpl(generateBreakpointId(
       ScriptBreakpoint(scriptId, lineNumber, columnNumber, String16()),
       source));
 }
 
 void V8DebuggerAgentImpl::reset() {
   if (!enabled()) return;
   m_scheduledDebuggerStep = NoStep;
   m_blackboxedPositions.clear();
   resetBlackboxedStateCache();
   m_scripts.clear();
   m_breakpointIdToDebuggerBreakpointIds.clear();
 }
 
 }  // namespace v8_inspector