Skip to content

Commit

Permalink
Bug 1651522 - [devtools] Remove the now useless `destroyServiceWorker…
Browse files Browse the repository at this point in the history 
…sOnNavigation` flag on TargetCommand. r=devtools-reviewers,nchevobbe

Given that this flag was only used from Debugger's firefox.js and only for local tab debugging,
we don't have to care about backward compatibility and can remove it right away.

Differential Revision: https://phabricator.services.mozilla.com/D191469
  • Loading branch information
@ochameau
ochameau committed Nov 20, 2023
1 parent 9d39da7 commit 0d2c86d
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 179 deletions.
14 changes: 0 additions & 14 deletions devtools/shared/commands/target/legacy-target-watchers/legacy-serviceworkers-watcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,6 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
return super._onProcessAvailable({ targetFront });
}

_shouldDestroyTargetsOnNavigation() {
return !!this.targetCommand.destroyServiceWorkersOnNavigation;
}

_onProcessDestroyed({ targetFront }) {
this._processTargets.delete(targetFront);
return super._onProcessDestroyed({ targetFront });
Expand Down Expand Up @@ -183,18 +179,8 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
// need to be destroyed.
if (resource.name === "dom-loading") {
const allServiceWorkerTargets = this._getAllServiceWorkerTargets();
const shouldDestroy = this._shouldDestroyTargetsOnNavigation();

for (const target of allServiceWorkerTargets) {
const isRegisteredBefore =
this.targetCommand.isTargetRegistered(target);
if (shouldDestroy && isRegisteredBefore) {
// Instruct the target command to notify about the worker target destruction
// but do not destroy the front as we want to keep using it.
// We will notify about it again via onTargetAvailable.
this.onTargetDestroyed(target, { shouldDestroyTargetFront: false });
}

// Note: we call isTargetRegistered again because calls to
// onTargetDestroyed might have modified the list of registered targets.
const isRegisteredAfter =
Expand Down
10 changes: 3 additions & 7 deletions devtools/shared/commands/target/legacy-target-watchers/legacy-workers-watcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,9 @@ class LegacyWorkersWatcher {
const listener = this.targetsListeners.get(targetFront);
targetFront.off("workerListChanged", listener);

// When unlisten is called from a target switch and service workers targets are not
// destroyed on navigation, we don't want to remove the targets from targetsByProcess
if (
!isTargetSwitching ||
!this._isServiceWorkerWatcher ||
this.targetCommand.destroyServiceWorkersOnNavigation
) {
// When unlisten is called from a target switch or when we observe service workers targets
// we don't want to remove the targets from targetsByProcess
if (!isTargetSwitching || !this._isServiceWorkerWatcher) {
this.targetsByProcess.delete(targetFront);
}
this.targetsListeners.delete(targetFront);
Expand Down
12 changes: 3 additions & 9 deletions devtools/shared/commands/target/target-command.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ class TargetCommand extends EventEmitter {
this.rootFront.traits.workerConsoleApiMessagesDispatchedToMainThread ===
false;
this.listenForServiceWorkers = false;
this.destroyServiceWorkersOnNavigation = false;

// Tells us if we received the first top level target.
// If target switching is done on:
Expand Down Expand Up @@ -271,12 +270,8 @@ class TargetCommand extends EventEmitter {

// Never destroy the popup targets when the top level target is destroyed
// as the popup follow a different lifecycle.
// Also avoid destroying service worker targets for similar reason,
// unless this.destroyServiceWorkersOnNavigation is true.
if (
!isPopup &&
(!isServiceWorker || this.destroyServiceWorkersOnNavigation)
) {
// Also avoid destroying service worker targets for similar reason.
if (!isPopup && !isServiceWorker) {
this._onTargetDestroyed(target, {
isTargetSwitching: isDestroyedTargetSwitching,
// Do not destroy service worker front as we may want to keep using it.
Expand All @@ -291,8 +286,7 @@ class TargetCommand extends EventEmitter {
this.stopListening({ isTargetSwitching: true });

// Remove destroyed target from the cached target list. We don't simply clear the
// Map as SW targets might not have been destroyed (i.e. when destroyServiceWorkersOnNavigation
// is set to false).
// Map as SW targets might not have been destroyed.
for (const target of destroyedTargets) {
this._targets.delete(target);
}
Expand Down
168 changes: 19 additions & 149 deletions devtools/shared/commands/target/tests/browser_target_command_service_workers_navigation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,37 +20,26 @@ const ORG_WORKER_URL = URL_ROOT_ORG_SSL + "test_sw_page_worker.js";
* The steps will be:
* - navigate to .com page
* - create target list
* -> onAvailable should be called for the .com worker
* - navigate to .org page
* -> onAvailable should be called for the .org worker
* - reload .org page
* -> nothing should happen
* - unregister .org worker
* -> onDestroyed should be called for the .org worker
* - navigate back to .com page
* -> nothing should happen
* - unregister .com worker
*
* First we test this with destroyServiceWorkersOnNavigation = false.
* In this case we expect the following calls:
* - navigate to .com page
* - create target list
* - onAvailable should be called for the .com worker
* - navigate to .org page
* - onAvailable should be called for the .org worker
* - reload .org page
* - nothing should happen
* - unregister .org worker
* - onDestroyed should be called for the .org worker
* - navigate back to .com page
* - nothing should happen
* - unregister .com worker
* - onDestroyed should be called for the .com worker
* -> onDestroyed should be called for the .com worker
*/
add_task(async function test_NavigationBetweenTwoDomains_NoDestroy() {
await setupServiceWorkerNavigationTest();

const tab = await addTab(COM_PAGE_URL);

const { hooks, commands, targetCommand } = await watchServiceWorkerTargets({
tab,
destroyServiceWorkersOnNavigation: false,
});
const { hooks, commands, targetCommand } = await watchServiceWorkerTargets(
tab
);

// We expect onAvailable to have been called one time, for the only service
// worker target available in the test page.
Expand Down Expand Up @@ -114,90 +103,6 @@ add_task(async function test_NavigationBetweenTwoDomains_NoDestroy() {
await removeTab(tab);
});

/**
* Same scenario as test_NavigationBetweenTwoDomains_NoDestroy, but this time
* with destroyServiceWorkersOnNavigation set to true.
*
* In this case we expect the following calls:
* - navigate to .com page
* - create target list
* - onAvailable should be called for the .com worker
* - navigate to .org page
* - onDestroyed should be called for the .com worker
* - onAvailable should be called for the .org worker
* - reload .org page
* - onDestroyed & onAvailable should be called for the .org worker
* - unregister .org worker
* - onDestroyed should be called for the .org worker
* - navigate back to .com page
* - onAvailable should be called for the .com worker
* - unregister .com worker
* - onDestroyed should be called for the .com worker
*/
add_task(async function test_NavigationBetweenTwoDomains_WithDestroy() {
await setupServiceWorkerNavigationTest();

const tab = await addTab(COM_PAGE_URL);

const { hooks, commands, targetCommand } = await watchServiceWorkerTargets({
tab,
destroyServiceWorkersOnNavigation: true,
});

// We expect onAvailable to have been called one time, for the only service
// worker target available in the test page.
await checkHooks(hooks, {
available: 1,
destroyed: 0,
targets: [COM_WORKER_URL],
});

info("Go to .org page, wait for onAvailable to be called");
BrowserTestUtils.startLoadingURIString(
gBrowser.selectedBrowser,
ORG_PAGE_URL
);
await checkHooks(hooks, {
available: 2,
destroyed: 1,
targets: [ORG_WORKER_URL],
});

info("Reload .org page, onAvailable and onDestroyed should be called");
gBrowser.reloadTab(gBrowser.selectedTab);
await checkHooks(hooks, {
available: 3,
destroyed: 2,
targets: [ORG_WORKER_URL],
});

info("Unregister .org service worker and wait until onDestroyed is called.");
await unregisterServiceWorker(tab, ORG_PAGE_URL);
await checkHooks(hooks, { available: 3, destroyed: 3, targets: [] });

info("Go back to page 1, wait for onDestroyed and onAvailable to be called");
BrowserTestUtils.startLoadingURIString(
gBrowser.selectedBrowser,
COM_PAGE_URL
);
await checkHooks(hooks, {
available: 4,
destroyed: 3,
targets: [COM_WORKER_URL],
});

info("Unregister .com service worker and wait until onDestroyed is called.");
await unregisterServiceWorker(tab, COM_PAGE_URL);
await checkHooks(hooks, { available: 4, destroyed: 4, targets: [] });

// Stop listening to avoid worker related requests
targetCommand.destroy();

await commands.waitForRequestsToSettle();
await commands.destroy();
await removeTab(tab);
});

/**
* In this test we load a service worker in a page prior to starting the
* TargetCommand. We start the target list on another page, and then we go back to
Expand All @@ -208,40 +113,15 @@ add_task(async function test_NavigationBetweenTwoDomains_WithDestroy() {
* - navigate to .com page
* - navigate to .org page
* - create target list
* -> onAvailable is called for the .org worker
* - unregister .org worker
* -> onDestroyed is called for the .org worker
* - navigate back to .com page
* -> onAvailable is called for the .com worker
* - unregister .com worker
*
* The expected calls are the same whether destroyServiceWorkersOnNavigation is
* true or false.
*
* Expected calls:
* - navigate to .com page
* - navigate to .org page
* - create target list
* - onAvailable is called for the .org worker
* - unregister .org worker
* - onDestroyed is called for the .org worker
* - navigate back to .com page
* - onAvailable is called for the .com worker
* - unregister .com worker
* - onDestroyed is called for the .com worker
* -> onDestroyed is called for the .com worker
*/
add_task(async function test_NavigationToPageWithExistingWorker_NoDestroy() {
await testNavigationToPageWithExistingWorker({
destroyServiceWorkersOnNavigation: false,
});
});

add_task(async function test_NavigationToPageWithExistingWorker_WithDestroy() {
await testNavigationToPageWithExistingWorker({
destroyServiceWorkersOnNavigation: true,
});
});

async function testNavigationToPageWithExistingWorker({
destroyServiceWorkersOnNavigation,
}) {
add_task(async function test_NavigationToPageWithExistingWorker() {
await setupServiceWorkerNavigationTest();

const tab = await addTab(COM_PAGE_URL);
Expand All @@ -266,10 +146,9 @@ async function testNavigationToPageWithExistingWorker({
await onBrowserLoaded;
await waitForRegistrationReady(tab, ORG_PAGE_URL);

const { hooks, commands, targetCommand } = await watchServiceWorkerTargets({
tab,
destroyServiceWorkersOnNavigation,
});
const { hooks, commands, targetCommand } = await watchServiceWorkerTargets(
tab
);

// We expect onAvailable to have been called one time, for the only service
// worker target available in the test page.
Expand Down Expand Up @@ -307,7 +186,7 @@ async function testNavigationToPageWithExistingWorker({
await commands.waitForRequestsToSettle();
await commands.destroy();
await removeTab(tab);
}
});

async function setupServiceWorkerNavigationTest() {
// Disable the preloaded process as it creates processes intermittently
Expand All @@ -320,22 +199,13 @@ async function setupServiceWorkerNavigationTest() {
await pushPref("dom.serviceWorkers.idle_timeout", 3000);
}

async function watchServiceWorkerTargets({
destroyServiceWorkersOnNavigation,
tab,
}) {
async function watchServiceWorkerTargets(tab) {
info("Create a target list for a tab target");
const commands = await CommandsFactory.forTab(tab);
const targetCommand = commands.targetCommand;

// Enable Service Worker listening.
targetCommand.listenForServiceWorkers = true;
info(
"Set targetCommand.destroyServiceWorkersOnNavigation to " +
destroyServiceWorkersOnNavigation
);
targetCommand.destroyServiceWorkersOnNavigation =
destroyServiceWorkersOnNavigation;
await targetCommand.startListening();

// Setup onAvailable & onDestroyed callbacks so that we can check how many
Expand Down

0 comments on commit 0d2c86d

Please sign in to comment.