From 458ed0080a8ae62b362bd1f509225a465adf6a6e Mon Sep 17 00:00:00 2001 From: Brian Hackett Date: Mon, 16 Sep 2019 22:43:39 +0000 Subject: [PATCH] Bug 1581665 - Tidy up handling of pause and paint data, r=jlast. Differential Revision: https://phabricator.services.mozilla.com/D46087 --HG-- extra : moz-landing-system : lando --- devtools/server/actors/replay/control.js | 54 +++++++++-------------- devtools/server/actors/replay/debugger.js | 8 ++-- devtools/server/actors/replay/replay.js | 27 +++++------- 3 files changed, 34 insertions(+), 55 deletions(-) diff --git a/devtools/server/actors/replay/control.js b/devtools/server/actors/replay/control.js index 2dabc46376f0..b1c7f9f2350f 100644 --- a/devtools/server/actors/replay/control.js +++ b/devtools/server/actors/replay/control.js @@ -238,7 +238,7 @@ ChildProcess.prototype = { return; } RecordReplayControl.waitUntilPaused(this.id, maybeCreateCheckpoint); - assert(this.paused); + assert(this.paused || this.crashed); }, // Add a checkpoint for this child to save. @@ -323,7 +323,6 @@ function lookupChild(id) { if (id == gMainChild.id) { return gMainChild; } - assert(gReplayingChildren[id]); return gReplayingChildren[id]; } @@ -1263,6 +1262,7 @@ function resume(forward) { maybeResumeRecording(); return; } + ensureFlushed(); } if ( gPausePoint.checkpoint == FirstCheckpointId && @@ -1827,7 +1827,12 @@ function Initialize(recordingChildId) { 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`); } @@ -2005,7 +2010,7 @@ const gControl = { 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) { @@ -2014,11 +2019,21 @@ const gControl = { // 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) { @@ -2028,35 +2043,6 @@ const gControl = { } }, - 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) { diff --git a/devtools/server/actors/replay/debugger.js b/devtools/server/actors/replay/debugger.js index a6132efc7275..9b098fd88c27 100644 --- a/devtools/server/actors/replay/debugger.js +++ b/devtools/server/actors/replay/debugger.js @@ -476,9 +476,6 @@ ReplayDebugger.prototype = { // 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; @@ -537,13 +534,14 @@ ReplayDebugger.prototype = { }, // 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; } diff --git a/devtools/server/actors/replay/replay.js b/devtools/server/actors/replay/replay.js index efaa89c707db..609e6dc06cd4 100644 --- a/devtools/server/actors/replay/replay.js +++ b/devtools/server/actors/replay/replay.js @@ -366,8 +366,6 @@ Services.obs.addObserver( contents.arguments.forEach(v => contents.argumentsData.addValue(v, PropertyLevels.FULL) ); - - ClearPausedState(); } newConsoleMessage(contents); @@ -911,12 +909,6 @@ function getDebuggeeValue(value) { return value; } -// eslint-disable-next-line no-unused-vars -function ClearPausedState() { - gPausedObjects = new IdMap(); - gDereferencedObjects = new Map(); -} - /////////////////////////////////////////////////////////////////////////////// // Manifest Management /////////////////////////////////////////////////////////////////////////////// @@ -1015,7 +1007,6 @@ const gManifestStartHandlers = { getPauseData() { divergeFromRecording(); const data = getPauseData(); - data.paintData = RecordReplayControl.repaint(); RecordReplayControl.manifestFinished(data); }, @@ -1034,12 +1025,7 @@ const gManifestStartHandlers = { 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) { @@ -1189,6 +1175,12 @@ 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) { @@ -1778,13 +1770,16 @@ PreviewedObjects.prototype = { // 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 = [];