Bug 1746952 - [devtools] Avoid notifying about same-process storage values when EFT is enabled. r=nchevobbe

We were notifying about same-process storage values twice.
Once with the top document target and another time via the iframe target.
This was probably invisible from the UI. We were probably only updating things twice without any visible breakage.

But this also highlighted that the test was quite wrong.
It was only asserting about the last resource for each storage type.
Luckily, the last resource to be notified was the one from the top document.
So that it contained all the storage values about all the iframes.

When I fix EFT implementation, now, iframe data is only visible in the iframe's related storage resource.
I tweaked the test to correctly assert data against the many resources for a single storage type.

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

devtools/server/actors/resources/utils/content-process-storage.js

@@ -8,7 +8,6 @@ const { storageTypePool } = require("devtools/server/actors/storage");
 const EventEmitter = require("devtools/shared/event-emitter");
 const { Ci } = require("chrome");
 const Services = require("Services");
-const { isWindowIncluded } = require("devtools/shared/layout/utils");
 
 // ms of delay to throttle updates
 const BATCH_DELAY = 200;
@@ -207,6 +206,9 @@ class StorageActorMock extends EventEmitter {
       // creating any global.
       return null;
     }
+    if (!this.isIncludedInTopLevelWindow(window)) {
+      return null;
+    }
     this.childWindowPool.add(window);
     for (let i = 0; i < docShell.childCount; i++) {
       const child = docShell.getChildAt(i);
@@ -224,7 +226,7 @@ class StorageActorMock extends EventEmitter {
   }
 
   isIncludedInTopLevelWindow(window) {
-    return isWindowIncluded(this.window, window);
+    return this.targetActor.windows.includes(window);
   }
 
   getWindowFromInnerWindowID(innerID) {

devtools/server/tests/browser/browser_storage_dynamic_windows.js

@@ -130,7 +130,12 @@ add_task(async function() {
     parentProcessStorages,
     allStorages,
   });
-  await testRemoveIframe(commands, initialResources, { allStorages });
+
+  await testRemoveIframe(commands, initialResources, {
+    contentProcessStorages,
+    parentProcessStorages,
+    allStorages,
+  });
 
   await clearStorage();
 
@@ -247,20 +252,24 @@ async function testAddIframe(
 ) {
   info("Testing if new iframe addition works properly");
 
-  // If Fission is enabled:
+  // If Fission or EFT is enabled:
   // * we get new resources alongside single-store-update events for content process storages
   // * only single-store-update events for previous resources for parent process storages
   // Otherwise if fission is disables:
   // * we get single-store-update events for all previous resources
   const onResources = waitForNewResourcesAndUpdates(
     commands,
-    isFissionEnabled() ? contentProcessStorages : []
-  );
-  // If fission is enabled, we only get update
-  const onUpdates = waitForResourceUpdates(
-    resources,
-    isFissionEnabled() ? parentProcessStorages : allStorages
+    isFissionEnabled() || isEveryFrameTargetEnabled()
+      ? contentProcessStorages
+      : []
   );
+  // If fission or EFT is enabled, we only get update for parent process storages.
+  // The content process storage resources are notified via brand new resource instances.
+  const storagesWithUpdates =
+    isFissionEnabled() || isEveryFrameTargetEnabled()
+      ? parentProcessStorages
+      : allStorages;
+  const onUpdates = waitForResourceUpdates(resources, storagesWithUpdates);
 
   await SpecialPowers.spawn(
     gBrowser.selectedBrowser,
@@ -280,7 +289,7 @@ async function testAddIframe(
   info("Wait for all updates");
   const previousResourceUpdates = await onUpdates;
 
-  if (isFissionEnabled()) {
+  if (isFissionEnabled() || isEveryFrameTargetEnabled()) {
     for (const resourceType of contentProcessStorages) {
       const resource = newResources[resourceType];
       const expected = afterIframeAdded[resourceType];
@@ -308,9 +317,6 @@ async function testAddIframe(
     }
   }
 
-  const storagesWithUpdates = isFissionEnabled()
-    ? parentProcessStorages
-    : allStorages;
   for (const resourceType of storagesWithUpdates) {
     const expected = afterIframeAdded[resourceType];
     const update = previousResourceUpdates[resourceType];
@@ -325,10 +331,21 @@ async function testAddIframe(
   return newResources;
 }
 
-async function testRemoveIframe(commands, resources, { allStorages }) {
+async function testRemoveIframe(
+  commands,
+  resources,
+  { contentProcessStorages, parentProcessStorages, allStorages }
+) {
   info("Testing if iframe removal works properly");
 
-  const onUpdates = waitForResourceUpdates(resources, allStorages);
+  // If fission or EFT is enabled, we only get update for parent process storages.
+  // The content process storage resources are wiped via their related target destruction.
+  const onUpdates = waitForResourceUpdates(
+    resources,
+    isFissionEnabled() || isEveryFrameTargetEnabled()
+      ? parentProcessStorages
+      : allStorages
+  );
 
   await SpecialPowers.spawn(gBrowser.selectedBrowser, [], () => {
     for (const iframe of content.document.querySelectorAll("iframe")) {
@@ -342,7 +359,11 @@ async function testRemoveIframe(commands, resources, { allStorages }) {
   info("Wait for all updates");
   const previousResourceUpdates = await onUpdates;
 
-  for (const resourceType of allStorages) {
+  const storagesWithUpdates =
+    isFissionEnabled() || isEveryFrameTargetEnabled()
+      ? parentProcessStorages
+      : allStorages;
+  for (const resourceType of storagesWithUpdates) {
     const expected = afterIframeRemoved[resourceType];
     const update = previousResourceUpdates[resourceType];
     const storageKey = resourceTypeToStorageKey(resourceType);
@@ -352,6 +373,24 @@ async function testRemoveIframe(commands, resources, { allStorages }) {
       `We get an update after the resource ${resourceType}, with the host values`
     );
   }
+
+  // With Fission or EFT, the iframe target is destroyed,
+  // which ends up destroying the related resources
+  if (isFissionEnabled() || isEveryFrameTargetEnabled()) {
+    const destroyedResourceTypes = [];
+    for (const storageType in resources) {
+      for (const resource of resources[storageType]) {
+        if (resource.isDestroyed()) {
+          destroyedResourceTypes.push(resource.resourceType);
+        }
+      }
+    }
+    Assert.deepEqual(
+      destroyedResourceTypes.sort(),
+      contentProcessStorages.sort(),
+      "Content process storage resources have been destroyed [local and session storages]"
+    );
+  }
 }
 
 /**