HHVM Debugger: Maintain stable variableReference IDs for the same obj…

…ects per-request

Summary:
Variables returned to the client in a VariablesResponse that are of complex types (objects, arrays, vec's, maps, etc) are assigned a variableReference identifier as required by the debug adapter protocol, so that the client can request the object's children in a subsequent command.

Currently, the debugger is returning a new, unique ID for each initial variablesRequest after the target thread steps. This requires less bookkeeping on the back-end, and also protects against certain races like a setVariable command that is processed asynchronously after a target request thread resumes or steps (in which case it may be impossible or unsafe to set the value of the object).

While the debug adapter protocol doesn't specify any restrictions on the variableReferene being reused vs unique, in practice VS Code appears to be using this to remember which nodes in the Variables scope tree are expanded or collapsed. The current behavior results in nodes collapsing after each step, which is frustrating to a user trying to watch the values of a child member while stepping through a code block. This was surfaced as issue: slackhq/vscode-hack#32

**This does not repro on Nuclide, which uses a path to the variable instead of the variable's internal ID to maintain expanded/collapsed state between steps.**

This diff modifies the back-end to re-use variablesReferences for the same object within the same request, even if the request has stepped. This only applies to complex objects. Simple values (ints, strings, etc) don't have children, and therefore can't be expanded in the UI, so this doesn't matter.  The identitiy of the object is determined by the object's actual in-memory address within HHVM, which assures uniqueness.

A new client preferences option was added to the launch+attach command, disableUniqueVarRef, that disables this new behavior if specified.

Reviewed By: velocityboy

Differential Revision: D10274418

fbshipit-source-id: 0e5d5d0fd2fe2d44c791e85aaa0c9ea33522bbb7

hphp/runtime/ext/vsdebug/debugger.cpp

@@ -329,6 +329,9 @@ void Debugger::cleanupRequestInfo(ThreadInfo* ti, RequestInfo* ri) {
     delete ri->m_breakpointInfo;
   }
 
+  ri->m_scopeIds.clear();
+  ri->m_objectIds.clear();
+
   assertx(ri->m_serverObjects.size() == 0);
   delete ri;
 }

hphp/runtime/ext/vsdebug/debugger.h

@@ -97,6 +97,10 @@ struct RequestInfo {
   const char* m_stepReason {nullptr};
   CommandQueue m_commandQueue;
   RequestBreakpointInfo* m_breakpointInfo {nullptr};
+
+  // Object IDs sent to the debugger client.
+  std::unordered_map<int, unsigned int> m_scopeIds;
+  std::unordered_map<void*, unsigned int> m_objectIds;
   std::unordered_map<unsigned int, ServerObject*> m_serverObjects;
 
   struct {
@@ -177,6 +181,9 @@ struct DebuggerOptions {
 
   // Tell the user if breakpoint calibration moves their bp.
   bool notifyOnBpCalibration;
+
+  // Don't try to unique variable references by address.
+  bool disableUniqueVarRef;
 };
 
 struct ClientInfo {
@@ -469,11 +476,13 @@ struct Debugger final {
       VSDebugLogger::LogLevelInfo,
       "Client options set:\n"
         "showDummyOnAsyncPause: %s\n"
-        "warnOnInterceptedFunctions: %s\n",
-        "notifyOnBpCalibration: %s\n",
+        "warnOnInterceptedFunctions: %s\n"
+        "notifyOnBpCalibration: %s\n"
+        "disableUniqueVarRef: %s\n",
       options.showDummyOnAsyncPause ? "YES" : "NO",
       options.warnOnInterceptedFunctions ? "YES" : "NO",
-      options.notifyOnBpCalibration ? "YES" : "NO"
+      options.notifyOnBpCalibration ? "YES" : "NO",
+      options.disableUniqueVarRef ? "YES" : "NO"
     );
   }
 

hphp/runtime/ext/vsdebug/launch_attach_command.cpp

@@ -56,6 +56,9 @@ bool LaunchAttachCommand::executeImpl(DebuggerSession* /*session*/,
   bool notifyOnBpCalibration =
     tryGetBool(args, "notifyOnBpCalibration", false);
 
+  bool disableUniqueVarRef =
+    tryGetBool(args, "disableUniqueVarRef", false);
+
   const auto& logFilePath =
     tryGetString(args, "logFilePath", emptyString);
 
@@ -94,6 +97,7 @@ bool LaunchAttachCommand::executeImpl(DebuggerSession* /*session*/,
   options.showDummyOnAsyncPause = showDummyOnAsyncPause;
   options.warnOnInterceptedFunctions = warnOnInterceptedFunctions;
   options.notifyOnBpCalibration = notifyOnBpCalibration;
+  options.disableUniqueVarRef = disableUniqueVarRef;
   m_debugger->setDebuggerOptions(options);
 
   // Send the InitializedEvent to indicate to the front-end that we are up

hphp/runtime/ext/vsdebug/session.cpp

@@ -304,9 +304,20 @@ unsigned int DebuggerSession::generateScopeId(
   int depth,
   ScopeType scopeType
 ) {
-  const unsigned int objectId = ++s_nextObjectId;
-  ScopeObject* scope = new ScopeObject(objectId, requestId, depth, scopeType);
+  unsigned int objectId;
+  RequestInfo* ri = m_debugger->getRequestInfo();
+  auto& existingScopes = ri->m_scopeIds;
+  auto it = existingScopes.find((int)scopeType);
+  if (it != existingScopes.end()) {
+    // This scope type for this request already has an ID assigned.
+    // Reuse it.
+    objectId = it->second;
+  } else {
+    objectId = ++s_nextObjectId;
+    existingScopes.emplace((int)scopeType, objectId);
+  }
 
+  ScopeObject* scope = new ScopeObject(objectId, requestId, depth, scopeType);
   assertx(requestId == m_debugger->getCurrentThreadId());
   registerRequestObject(objectId, scope);
   return objectId;
@@ -316,9 +327,8 @@ unsigned int DebuggerSession::generateVariableId(
   request_id_t requestId,
   Variant& variable
 ) {
-  const unsigned int objectId = ++s_nextObjectId;
+  const unsigned int objectId = generateOrReuseVariableId(variable);
   VariableObject* varObj = new VariableObject(objectId, requestId, variable);
-
   assertx(requestId == m_debugger->getCurrentThreadId());
   registerRequestObject(objectId, varObj);
   return objectId;
@@ -345,6 +355,47 @@ unsigned int DebuggerSession::generateVariableSubScope(
   return objectId;
 }
 
+unsigned int DebuggerSession::generateOrReuseVariableId(
+  const Variant& variable
+) {
+  // Generate a new object ID if we haven't seen this variant before. Ensure
+  // IDs are stable for variants that contain objects or arrays, based on the
+  // address of the object to which they point.
+  RequestInfo* ri = m_debugger->getRequestInfo();
+  const auto options = m_debugger->getDebuggerOptions();
+
+  void* key = nullptr;
+  auto& existingVariables = ri->m_objectIds;
+  if (!options.disableUniqueVarRef) {
+    if (variable.isArray()) {
+      key = (void*)variable.getArrayDataOrNull();
+    } else if (variable.isObject()) {
+      key = (void*)variable.getObjectDataOrNull();
+    }
+
+    if (key != nullptr) {
+      const auto it = existingVariables.find(key);
+      if (it != existingVariables.end()) {
+        const unsigned int objectId = it->second;
+        return objectId;
+      }
+    }
+  }
+
+  // Allocate a new ID.
+  const unsigned int objectId = ++s_nextObjectId;
+
+  if (key != nullptr) {
+    // Remember the object ID for complex types (Objects, Arrays).
+    // Since simple types have no children, and cannot be expanded
+    // in clients' Variables/Scopes windows, it is not important
+    // that they receive the same object ID across requests.
+    existingVariables.emplace(key, objectId);
+  }
+
+  return objectId;
+}
+
 ScopeObject* DebuggerSession::getScopeObject(unsigned int objectId) {
   auto object = getServerObject(objectId);
   if (object != nullptr) {
@@ -365,7 +416,16 @@ void DebuggerSession::registerRequestObject(
   RequestInfo* ri = m_debugger->getRequestInfo();
 
   // Add this object to the per-request list of objects.
-  std::unordered_map<unsigned int, ServerObject*>& objs = ri->m_serverObjects;
+  auto& objs = ri->m_serverObjects;
+  auto it = objs.find(objectId);
+  if (it != objs.end()) {
+    // Replacing server object by ID. Free the old object.
+    ServerObject* object = it->second;
+    objs.erase(it);
+    onServerObjectDestroyed(objectId);
+    delete object;
+  }
+
   objs.emplace(objectId, obj);
 
   // Add this object to the per-session global list of objects.

hphp/runtime/ext/vsdebug/session.h

@@ -95,6 +95,10 @@ struct DebuggerSession final {
 
 private:
 
+  unsigned int generateOrReuseVariableId(
+    const Variant& variable
+  );
+
   void registerRequestObject(
     unsigned int objectId,
     ServerObject* obj
@@ -120,8 +124,7 @@ struct DebuggerSession final {
 
   // When a request is paused, the backend must maintain state about scopes,
   // frames and variable IDs sent to the front end. The IDs are globally
-  // unique, and the objects to which they refer are valid per-request and
-  // only until that request resumes.
+  // unique, and the objects to which they refer are valid per-request..
   static unsigned int s_nextObjectId;
   std::unordered_map<unsigned int, ServerObject*> m_serverObjects;