Skip to content

Commit

Permalink
Bug 1611817 - Fix race condition in DevTools shutdown and stopProfile…
Browse files Browse the repository at this point in the history
…rAndDiscardProfile; r=julienw

This race condition is really only exposed in automated testing, but it
was making the DevTools performance-new mochitests intermittent.

The race looks like this:

* stopProfilerAndDiscardProfiler() request
* perf actor process the request
* The gecko profiler sends an event notifying that the profiler stopped
* DevTools updates the UI upon receiving the event
* The test suite sees the UI update, and triggers a close of DevTools
* Error: Connection closed, pending request
* stopProfilerAndDiscardProfiler still hasn't sent the response yet

This patch fixes it by not throwing an error if the panel is already destroyed.

Differential Revision: https://phabricator.services.mozilla.com/D65150

--HG--
extra : moz-landing-system : lando
  • Loading branch information
gregtatum committed Mar 9, 2020
1 parent 806c8be commit 68c5d4c
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 2 deletions.
3 changes: 2 additions & 1 deletion devtools/client/performance-new/@types/perf.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ export interface PanelWindow {
gToolbox?: any;
gInit(perfFront: PerfFront, preferenceFront: PageContext): void;
gDestroy(): void;
gReportReady?(): void
gReportReady?(): void;
gIsPanelDestroyed?: boolean;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions devtools/client/performance-new/panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class PerformancePanel {
*/
async _doOpen() {
this.panelWin.gToolbox = this.toolbox;
this.panelWin.gIsPanelDestroyed = false;

const perfFront = await this.target.client.mainRoot.getFront("perf");

Expand All @@ -82,6 +83,7 @@ class PerformancePanel {
this.panelWin.gDestroy();
this.emit("destroyed");
this._destroyed = true;
this.panelWin.gIsPanelDestroyed = true;
}
}

Expand Down
19 changes: 18 additions & 1 deletion devtools/client/performance-new/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const {
* @typedef {import("../@types/perf").RecordingState} RecordingState
* @typedef {import("../@types/perf").InitializeStoreValues} InitializeStoreValues
* @typedef {import("../@types/perf").Presets} Presets
* @typedef {import("../@types/perf").PanelWindow} PanelWindow
*/

/**
Expand Down Expand Up @@ -230,6 +231,22 @@ exports.stopProfilerAndDiscardProfile = () => {
return async (dispatch, getState) => {
const perfFront = selectors.getPerfFront(getState());
dispatch(changeRecordingState("request-to-stop-profiler"));
perfFront.stopProfilerAndDiscardProfile();

try {
await perfFront.stopProfilerAndDiscardProfile();
} catch (error) {
/** @type {any} */
const anyWindow = window;
/** @type {PanelWindow} - Coerce the window into the PanelWindow. */
const { gIsPanelDestroyed } = anyWindow;

if (gIsPanelDestroyed) {
// This error is most likely "Connection closed, pending request" as the
// command can race with closing the panel. Do not report an error. It's
// most likely fine.
} else {
throw error;
}
}
};
};

0 comments on commit 68c5d4c

Please sign in to comment.