Fix leak in debug mirror cache.

src/debug-debugger.js

 // Copyright 2012 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.
 
 // Default number of frames to include in the response to backtrace request.
 var kDefaultBacktraceLength = 10;
 
 var Debug = {};
 
 // Regular expression to skip "crud" at the beginning of a source line which is
@@ skipping 468 matching lines @@
   var result = [];
   for (var i = 0; i < script_break_points.length; i++) {
     if (script_break_points[i].matchesScript(script)) {
       result.push(script_break_points[i]);
     }
   }
   return result;
 }
 
 
+function GetLoadedScripts() {
+  ClearMirrorCache();  // The mirror cache may be holding onto scripts.
+  return %DebugGetLoadedScripts();

[yurys, 2014/06/03 08:07:21]

This change looks wrong to me. By design of V8 debugging protocol mirror cache
should be kept while we are staying on a breakpoint so that the client could
access corresponding object by its mirror id. After this change, however, the
cache will be cleared on any call to GetLoadedScripts() and the remote id will
become invalid.

In case of blink the problem is that we create mirror objects not only when we
stay on a breakpoint and I'm not sure we clear them properly. Also we don't use
id->mirror map in blink and I believe a right way to fix this would be to
disable mirror cache in blink completely. WDYT?

[Yang, 2014/06/03 08:12:46]

Not sure I understand. Which code path does blink use to get loaded scripts?

+}
+
+
 Debug.setListener = function(listener, opt_data) {
   if (!IS_FUNCTION(listener) && !IS_UNDEFINED(listener) && !IS_NULL(listener)) {
     throw new Error('Parameters have wrong types.');
   }
   %SetDebugEventListener(listener, opt_data);
 };
 
 
 Debug.breakExecution = function(f) {
   %Break();
@@ skipping 409 matching lines @@
   pos = source.length;
   result += source.substring(prev_pos, pos);
   return result;
 };
 
 
 // Get all the scripts currently loaded. Locating all the scripts is based on
 // scanning the heap.
 Debug.scripts = function() {
   // Collect all scripts in the heap.
-  return %DebugGetLoadedScripts();
+  return GetLoadedScripts();
 };
 
 
 Debug.debuggerFlags = function() {
   return debugger_flags;
 };
 
 Debug.MakeMirror = MakeMirror;
 
 function MakeExecutionState(break_id) {
@@ skipping 1308 matching lines @@
     if (!IS_UNDEFINED(request.arguments.filter)) {
       var num = %ToNumber(request.arguments.filter);
       if (!isNaN(num)) {
         filterNum = num;
       }
       filterStr = request.arguments.filter;
     }
   }
 
   // Collect all scripts in the heap.
-  var scripts = %DebugGetLoadedScripts();
+  var scripts = GetLoadedScripts();
 
   response.body = [];
 
   for (var i = 0; i < scripts.length; i++) {
     if (idsToInclude && !idsToInclude[scripts[i].id]) {
       continue;
     }
     if (filterStr || filterNum) {
       var script = scripts[i];
       var found = false;
@@ skipping 51 matching lines @@
 
 
 DebugCommandProcessor.prototype.changeLiveRequest_ = function(
     request, response) {
   if (!request.arguments) {
     return response.failed('Missing arguments');
   }
   var script_id = request.arguments.script_id;
   var preview_only = !!request.arguments.preview_only;
 
-  var scripts = %DebugGetLoadedScripts();
+  var scripts = GetLoadedScripts();
 
   var the_script = null;
   for (var i = 0; i < scripts.length; i++) {
     if (scripts[i].id == script_id) {
       the_script = scripts[i];
     }
   }
   if (!the_script) {
     response.failed('Script not found');
     return;
@@ skipping 232 matching lines @@
 
     default:
       json = null;
   }
   return json;
 }
 
 Debug.TestApi = {
   CommandProcessorResolveValue: DebugCommandProcessor.resolveValue_
 };