Bug 1581665 - Tidy up handling of pause and paint data, r=jlast.
authorBrian Hackett <bhackett1024@gmail.com>
Mon, 16 Sep 2019 22:43:39 +0000
changeset 493650 e4ceef48e52d0b621c93eefa563ce731606f0ad3
parent 493649 19c786ed3d977565a649758242f5519000f9501d
child 493651 f3cf877afac25af50b7d30d81f8efefed15fd158
push id36585
push usermalexandru@mozilla.com
push dateWed, 18 Sep 2019 09:56:40 +0000
treeherdermozilla-central@e71921b729c6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlast
bugs1581665
milestone71.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1581665 - Tidy up handling of pause and paint data, r=jlast. Differential Revision: https://phabricator.services.mozilla.com/D46087
devtools/server/actors/replay/control.js
devtools/server/actors/replay/debugger.js
devtools/server/actors/replay/replay.js
--- a/devtools/server/actors/replay/control.js
+++ b/devtools/server/actors/replay/control.js
@@ -233,17 +233,17 @@ ChildProcess.prototype = {
   // Block until this child is paused. If maybeCreateCheckpoint is specified
   // then a checkpoint is created if this child is recording, so that it pauses
   // quickly (otherwise it could sit indefinitely if there is no page activity).
   waitUntilPaused(maybeCreateCheckpoint) {
     if (this.paused) {
       return;
     }
     RecordReplayControl.waitUntilPaused(this.id, maybeCreateCheckpoint);
-    assert(this.paused);
+    assert(this.paused || this.crashed);
   },
 
   // Add a checkpoint for this child to save.
   addSavedCheckpoint(checkpoint) {
     dumpv(`AddSavedCheckpoint #${this.id} ${checkpoint}`);
     this.savedCheckpoints.add(checkpoint);
   },
 
@@ -318,17 +318,16 @@ let gMainChild;
 // Replaying children available for exploring the interior of the recording,
 // indexed by their ID.
 const gReplayingChildren = [];
 
 function lookupChild(id) {
   if (id == gMainChild.id) {
     return gMainChild;
   }
-  assert(gReplayingChildren[id]);
   return gReplayingChildren[id];
 }
 
 function closestChild(point) {
   let minChild = null,
     minTime = Infinity;
   for (const child of gReplayingChildren) {
     if (child) {
@@ -1258,16 +1257,17 @@ async function finishResume() {
 // breakpoint hit.
 function resume(forward) {
   gDebuggerRequests.length = 0;
   if (gActiveChild.recording) {
     if (forward) {
       maybeResumeRecording();
       return;
     }
+    ensureFlushed();
   }
   if (
     gPausePoint.checkpoint == FirstCheckpointId &&
     !gPausePoint.position &&
     !forward
   ) {
     gDebugger._onPause();
     return;
@@ -1822,17 +1822,22 @@ function Initialize(recordingChildId) {
     dump(`ERROR: Initialize threw exception: ${e}\n`);
   }
 }
 
 // eslint-disable-next-line no-unused-vars
 function ManifestFinished(id, response) {
   try {
     dumpv(`ManifestFinished #${id} ${stringify(response)}`);
-    lookupChild(id).manifestFinished(response);
+    const child = lookupChild(id);
+    if (child) {
+      child.manifestFinished(response);
+    } else {
+      // Ignore messages from child processes that we have marked as crashed.
+    }
   } catch (e) {
     dump(`ERROR: ManifestFinished threw exception: ${e} ${e.stack}\n`);
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Debugger Operations
 ////////////////////////////////////////////////////////////////////////////////
@@ -2000,68 +2005,49 @@ const gControl = {
 
   unscannedRegions,
   cachedPoints,
 
   debuggerRequests() {
     return gDebuggerRequests;
   },
 
-  getPauseData() {
+  getPauseDataAndRepaint() {
     // If the child has not arrived at the pause point yet, see if there is
     // cached pause data for this point already which we can immediately return.
     if (gPauseMode == PauseModes.ARRIVING && !gDebuggerRequests.length) {
       const data = maybeGetPauseData(gPausePoint);
       if (data) {
         // After the child pauses, it will need to generate the pause data so
         // that any referenced objects will be instantiated.
         addDebuggerRequest({ type: "pauseData" });
+        RecordReplayControl.hadRepaint(data.paintData);
         return data;
       }
     }
     gControl.maybeSwitchToReplayingChild();
-    return gControl.sendRequest({ type: "pauseData" });
+    const data = gControl.sendRequest({ type: "pauseData" });
+    if (data.unhandledDivergence) {
+      RecordReplayControl.clearGraphics();
+    } else {
+      addPauseData(gPausePoint, data, /* trackCached */ true);
+      if (data.paintData) {
+        RecordReplayControl.hadRepaint(data.paintData);
+      }
+    }
+    return data;
   },
 
   paint(point) {
     const data = maybeGetPauseData(point);
     if (data && data.paintData) {
       RecordReplayControl.hadRepaint(data.paintData);
     }
   },
 
-  repaint() {
-    if (!gPausePoint) {
-      return;
-    }
-    if (
-      gMainChild.paused &&
-      pointEquals(gPausePoint, gMainChild.pausePoint())
-    ) {
-      // Flush the recording if we are repainting because we interrupted things
-      // and will now rewind.
-      if (gMainChild.recording) {
-        ensureFlushed();
-      }
-      return;
-    }
-    const data = maybeGetPauseData(gPausePoint);
-    if (data && data.paintData) {
-      RecordReplayControl.hadRepaint(data.paintData);
-    } else {
-      gControl.maybeSwitchToReplayingChild();
-      const rv = gControl.sendRequest({ type: "repaint" });
-      if (rv && rv.length) {
-        RecordReplayControl.hadRepaint(rv);
-      } else {
-        RecordReplayControl.clearGraphics();
-      }
-    }
-  },
-
   isPausedAtDebuggerStatement() {
     const point = gControl.pausePoint();
     if (point) {
       const checkpoint = getSavedCheckpoint(point.checkpoint);
       const { debuggerStatements } = getCheckpointInfo(checkpoint);
       return pointArrayIncludes(debuggerStatements, point);
     }
     return false;
--- a/devtools/server/actors/replay/debugger.js
+++ b/devtools/server/actors/replay/debugger.js
@@ -471,19 +471,16 @@ ReplayDebugger.prototype = {
     // The thread has paused so that the user can interact with it. The child
     // will stay paused until this thread-wide pause has been popped.
     this._ensurePaused();
     assert(!this._resumeCallback);
     if (++this._threadPauseCount == 1) {
       // There is no preferred direction of travel after an explicit pause.
       this._direction = Direction.NONE;
 
-      // Update graphics according to the current state of the child.
-      this._control.repaint();
-
       // If breakpoint handlers for the pause haven't been called yet, don't
       // call them at all.
       this._cancelPerformPause = true;
     }
     const point = this.replayCurrentExecutionPoint();
     dumpv("PushPause " + JSON.stringify(point));
   },
 
@@ -532,23 +529,24 @@ ReplayDebugger.prototype = {
   },
 
   // Clear out all data that becomes invalid when the child unpauses.
   _invalidateAfterUnpause() {
     this._pool = new ReplayPool(this);
   },
 
   // Fill in the debugger with (hopefully) all data the client/server need to
-  // pause at the current location.
+  // pause at the current location. This also updates graphics to match the
+  // current location.
   _capturePauseData() {
     if (this._pool.frames.length) {
       return;
     }
 
-    const pauseData = this._control.getPauseData();
+    const pauseData = this._control.getPauseDataAndRepaint();
     if (!pauseData.frames) {
       return;
     }
 
     for (const data of Object.values(pauseData.scripts)) {
       this._addScript(data);
     }
 
--- a/devtools/server/actors/replay/replay.js
+++ b/devtools/server/actors/replay/replay.js
@@ -361,18 +361,16 @@ Services.obs.addObserver(
         contents.arguments = apiMessage.arguments.map(v => {
           return makeConvertedDebuggeeValue(v);
         });
 
         contents.argumentsData = new PreviewedObjects();
         contents.arguments.forEach(v =>
           contents.argumentsData.addValue(v, PropertyLevels.FULL)
         );
-
-        ClearPausedState();
       }
 
       newConsoleMessage(contents);
     },
   },
   "console-api-log-event"
 );
 
@@ -906,22 +904,16 @@ function makeConvertedDebuggeeValue(valu
 function getDebuggeeValue(value) {
   if (value && typeof value == "object") {
     assert(value instanceof Debugger.Object);
     return value.unsafeDereference();
   }
   return value;
 }
 
-// eslint-disable-next-line no-unused-vars
-function ClearPausedState() {
-  gPausedObjects = new IdMap();
-  gDereferencedObjects = new Map();
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // Manifest Management
 ///////////////////////////////////////////////////////////////////////////////
 
 // The manifest that is currently being processed.
 let gManifest = { kind: "primordial" };
 
 // When processing certain manifests this tracks the execution time when the
@@ -1010,17 +1002,16 @@ const gManifestStartHandlers = {
     RecordReplayControl.manifestFinished({
       divergedFromRecording: gDivergedFromRecording,
     });
   },
 
   getPauseData() {
     divergeFromRecording();
     const data = getPauseData();
-    data.paintData = RecordReplayControl.repaint();
     RecordReplayControl.manifestFinished(data);
   },
 
   hitLogpoint({ text, condition, skipPauseData }) {
     divergeFromRecording();
 
     const frame = scriptFrameForIndex(countScriptFrames() - 1);
     if (condition) {
@@ -1029,22 +1020,17 @@ const gManifestStartHandlers = {
         RecordReplayControl.manifestFinished({ result: null });
         return;
       }
     }
 
     const displayName = formatDisplayName(frame);
     const rv = frame.evalWithBindings(`[${text}]`, { displayName });
 
-    let pauseData;
-    if (!skipPauseData) {
-      pauseData = getPauseData();
-      pauseData.paintData = RecordReplayControl.repaint();
-      ClearPausedState();
-    }
+    const pauseData = skipPauseData ? undefined : getPauseData();
 
     let result;
     if (rv.return) {
       result = getDebuggeeValue(rv.return);
     } else {
       result = [getDebuggeeValue(rv.throw)];
     }
     result = result.map(v => makeConvertedDebuggeeValue(v));
@@ -1184,16 +1170,22 @@ function processManifestAfterCheckpoint(
   }
 }
 
 // eslint-disable-next-line no-unused-vars
 function HitCheckpoint(id) {
   gLastCheckpoint = id;
   const point = currentExecutionPoint();
 
+  // Reset paused state at each checkpoint. In order to reach the checkpoint we
+  // must have unpaused, and resetting the state allows these objects to be
+  // collected by the GC.
+  gPausedObjects = new IdMap();
+  gDereferencedObjects = new Map();
+
   try {
     processManifestAfterCheckpoint(point);
   } catch (e) {
     printError("AfterCheckpoint", e);
   }
 }
 
 // Handlers that run after reaching a position watched by ensurePositionHandler.
@@ -1773,23 +1765,26 @@ PreviewedObjects.prototype = {
 // debugger running in the server can request a pause data packet which includes
 // everything the server will need.
 //
 // This should avoid overapproximation, so that we can quickly send pause data
 // across a network connection, and especially should not underapproximate
 // as the server will end up needing to make more requests before the client can
 // finish pausing.
 function getPauseData() {
+  const paintData = RecordReplayControl.repaint();
+
   const numFrames = countScriptFrames();
   if (!numFrames) {
-    return {};
+    return { paintData };
   }
 
   const rv = new PreviewedObjects();
 
+  rv.paintData = paintData;
   rv.frames = [];
   rv.scripts = {};
   rv.offsetMetadata = [];
 
   // eslint-disable-next-line no-shadow
   function addScript(id) {
     if (!rv.scripts[id]) {
       rv.scripts[id] = getScriptData(id);