зеркало из https://github.com/mozilla/gecko-dev.git
Bug 1652016 - Destroy target created by WebExtension when destroying the toolbox. r=rpl,daisuke,jdescottes.
The problem has 2 folds. The first one is that the shutdown function on DevToolsExtensionPageContextParent wasn't called, which means the target was never destroyed. This is fixed by overriding unload in DevToolsExtensionPageContextParent instead of shutdown, so the function gets called and the target destroyed. The second issue was that a single webextension could create 2 targets, and since we only keep track of a single target, we would miss one. This is fixed by putting the call to watchTargets in getCurrentDevTools in a promise, so subsequent calls that might occur before the resulting promise isn't resolved don't end up calling watchTargets a second time. Differential Revision: https://phabricator.services.mozilla.com/D83935
This commit is contained in:
Nicolas Chevobbe
Родитель
f75d9d861b
Коммит
ae24ed2cde
|
|
@ -24,6 +24,25 @@ function getAdditionalPanelId(toolbox, label) {
|
|||
return panelDef.id;
|
||||
}
|
||||
|
||||
/**
|
||||
* Helper that returns the number of existing target actors for the content browserId
|
||||
* @param {Tab} tab
|
||||
* @returns {Integer} the number of targets
|
||||
*/
|
||||
function getTargetActorsCount(tab) {
|
||||
return SpecialPowers.spawn(tab.linkedBrowser, [], () => {
|
||||
const { TargetActorRegistry } = ChromeUtils.import(
|
||||
"resource://devtools/server/actors/targets/target-actor-registry.jsm"
|
||||
);
|
||||
|
||||
// Retrieve the target actor instances
|
||||
const targets = TargetActorRegistry.getTargetActors(
|
||||
content.browsingContext.browserId
|
||||
);
|
||||
return targets.length;
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* this test file ensures that:
|
||||
*
|
||||
|
|
@ -436,3 +455,89 @@ add_task(async function test_devtools_inspectedWindow_eval_in_page_and_panel() {
|
|||
await extension.unload();
|
||||
BrowserTestUtils.removeTab(tab);
|
||||
});
|
||||
|
||||
/**
|
||||
* This test asserts that there's only one target created by the extension, and that
|
||||
* closing the DevTools toolbox destroys it.
|
||||
* See Bug 1652016
|
||||
*/
|
||||
add_task(async function test_devtools_inspectedWindow_eval_target_lifecycle() {
|
||||
const TEST_TARGET_URL = "http://mochi.test:8888/";
|
||||
let tab = await BrowserTestUtils.openNewForegroundTab(
|
||||
gBrowser,
|
||||
TEST_TARGET_URL
|
||||
);
|
||||
|
||||
function devtools_page() {
|
||||
browser.test.onMessage.addListener(async (msg, ...args) => {
|
||||
if (msg !== "inspectedWindow-eval-requests") {
|
||||
browser.test.fail(`Unexpected test message received: ${msg}`);
|
||||
return;
|
||||
}
|
||||
|
||||
const promises = [];
|
||||
for (let i = 0; i < 10; i++) {
|
||||
promises.push(browser.devtools.inspectedWindow.eval(`${i * 2}`));
|
||||
}
|
||||
|
||||
await Promise.all(promises);
|
||||
browser.test.sendMessage("inspectedWindow-eval-requests-done");
|
||||
});
|
||||
}
|
||||
|
||||
let extension = ExtensionTestUtils.loadExtension({
|
||||
manifest: {
|
||||
devtools_page: "devtools_page.html",
|
||||
},
|
||||
files: {
|
||||
"devtools_page.html": `<!DOCTYPE html>
|
||||
<html>
|
||||
<head>
|
||||
<meta charset="utf-8">
|
||||
<script text="text/javascript" src="devtools_page.js"></script>
|
||||
</head>
|
||||
<body>
|
||||
</body>
|
||||
</html>`,
|
||||
"devtools_page.js": devtools_page,
|
||||
},
|
||||
});
|
||||
|
||||
await extension.startup();
|
||||
|
||||
await openToolboxForTab(tab);
|
||||
|
||||
let targetsCount = await getTargetActorsCount(tab);
|
||||
is(
|
||||
targetsCount,
|
||||
1,
|
||||
"There's only one target for the content page, the one for DevTools Toolbox"
|
||||
);
|
||||
|
||||
info("Check that evaluating multiple times doesn't create multiple targets");
|
||||
const onEvalRequestsDone = extension.awaitMessage(
|
||||
`inspectedWindow-eval-requests-done`
|
||||
);
|
||||
extension.sendMessage(`inspectedWindow-eval-requests`);
|
||||
|
||||
info("Wait for response from the panel");
|
||||
await onEvalRequestsDone;
|
||||
|
||||
targetsCount = await getTargetActorsCount(tab);
|
||||
is(
|
||||
targetsCount,
|
||||
2,
|
||||
"Only 1 additional target was created when calling inspectedWindow.eval"
|
||||
);
|
||||
|
||||
info(
|
||||
"Close the toolbox and make sure the extension gets unloaded, and the target destroyed"
|
||||
);
|
||||
await closeToolboxForTab(tab);
|
||||
|
||||
targetsCount = await getTargetActorsCount(tab);
|
||||
is(targetsCount, 0, "All targets were removed as toolbox was closed");
|
||||
|
||||
await extension.unload();
|
||||
BrowserTestUtils.removeTab(tab);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -24,22 +24,37 @@ var TargetActorRegistry = {
|
|||
},
|
||||
|
||||
/**
|
||||
* Return the target actor matching the passed browser element id. Returns null if
|
||||
* Return the first target actor matching the passed browser element id. Returns null if
|
||||
* no matching target actors could be found.
|
||||
*
|
||||
* @param {Integer} browserId
|
||||
* @param {Integer} browserId: The browserId to retrieve targets for. Pass null to
|
||||
* retrieve the parent process targets.
|
||||
* @returns {TargetActor|null}
|
||||
*/
|
||||
getTargetActor(browserId) {
|
||||
return this.getTargetActors(browserId)[0] || null;
|
||||
},
|
||||
|
||||
/**
|
||||
* Return the target actors matching the passed browser element id.
|
||||
* In some scenarios, the registstry can have multiple target actors for a given
|
||||
* browserId (e.g. the regular DevTools content toolbox + DevTools WebExtensions targets).
|
||||
*
|
||||
* @param {Integer} browserId: The browserId to retrieve targets for. Pass null to
|
||||
* retrieve the parent process targets.
|
||||
* @returns {Array<TargetActor>}
|
||||
*/
|
||||
getTargetActors(browserId) {
|
||||
const actors = [];
|
||||
for (const actor of browsingContextTargetActors) {
|
||||
if (
|
||||
actor.browserId == browserId ||
|
||||
(browserId === null && actor.typeName === "parentProcessTarget")
|
||||
) {
|
||||
return actor;
|
||||
actors.push(actor);
|
||||
}
|
||||
}
|
||||
return null;
|
||||
return actors;
|
||||
},
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -675,16 +675,30 @@ class DevToolsExtensionPageContextParent extends ExtensionPageContextParent {
|
|||
*/
|
||||
async getCurrentDevToolsTarget() {
|
||||
if (!this._currentDevToolsTarget) {
|
||||
await this.devToolsToolbox.targetList.watchTargets(
|
||||
[this.devToolsToolbox.targetList.TYPES.FRAME],
|
||||
this._onTargetAvailable
|
||||
);
|
||||
if (!this._pendingWatchTargetsPromise) {
|
||||
// When _onTargetAvailable is called, it will create a new target,
|
||||
// via DevToolsShim.createTargetForTab. If this function is called multiple times
|
||||
// before this._currentDevToolsTarget is populated, we don't want to create X
|
||||
// new, duplicated targets, so we store the Promise returned by watchTargets, in
|
||||
// order to properly wait on subsequent calls.
|
||||
this._pendingWatchTargetsPromise = this.devToolsToolbox.targetList.watchTargets(
|
||||
[this.devToolsToolbox.targetList.TYPES.FRAME],
|
||||
this._onTargetAvailable
|
||||
);
|
||||
}
|
||||
await this._pendingWatchTargetsPromise;
|
||||
this._pendingWatchTargetsPromise = null;
|
||||
}
|
||||
|
||||
return this._currentDevToolsTarget;
|
||||
}
|
||||
|
||||
shutdown() {
|
||||
unload() {
|
||||
// Bail if the toolbox reference was already cleared.
|
||||
if (!this.devToolsToolbox) {
|
||||
return;
|
||||
}
|
||||
|
||||
this.devToolsToolbox.targetList.unwatchTargets(
|
||||
[this.devToolsToolbox.targetList.TYPES.FRAME],
|
||||
this._onTargetAvailable
|
||||
|
|
@ -709,7 +723,7 @@ class DevToolsExtensionPageContextParent extends ExtensionPageContextParent {
|
|||
|
||||
this._devToolsToolbox = null;
|
||||
|
||||
super.shutdown();
|
||||
super.unload();
|
||||
}
|
||||
|
||||
async _onTargetAvailable({ targetFront }) {
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче