1854601 - Re-enable toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html after bug 543435

Status: RESOLVED DUPLICATE of bug 1949168

(WebExtensions :: General, defect, P3)

Description

[Henri Sivonen (:hsivonen)]

toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html is already questionable, see bug 1777023. The patch for bug 543435 makes it permafail, so I'm disabling the test in bug 543435. This bug is about re-enabling it.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

I haven't confirmed yet if it would be the only change necessary, but from a quick look to the test I'd expect that with the changes landed from Bug 543435 the test would get stuck here on the call to await waitForLoad(testWindow):

testWindow = window.open("about:blank", ...);
await waitForLoad(testWindow);

due to the targetWindow about:blank document to be already readyState "complete" right away and waitForLoad test helper would be instead waiting for a load event that will never be fired.

If that's the case, and it is the case in all builds, we could replace that waitForLoad with an assertion that confirms that the targetWindow about:blank document is readyState complete as expected, e.g. something like:

diff --git a/toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html b/toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html
--- a/toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html
@@ -17,13 +17,18 @@ if (AppConstants.platform === "android")
 
 let windowData, testWindow;
 
-add_task(async function setup() {
+add_setup(async function setup() {
   await SpecialPowers.spawnChrome([], async () => {
     Services.cache2.clear();
   });
 
   testWindow = window.open("about:blank", "_blank", "width=100,height=100");
-  await waitForLoad(testWindow);
+  ok(
+    testWindow.document.readyState === "complete",
+    `Expect about:blank doc to be readyState complete right away: ${
+      testWindow.document.readyState
+    }`
+  );
 
   // Fetch the windowId and tabId we need to filter with WebRequest.
   let extension = ExtensionTestUtils.loadExtension({

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Needinfo-ing myself to come back to this issue and confirm what is described in comment 1 (either on a local build where the changes from Bug 543435 are being applied or on an artifact build if Bug 543435 got already merged on mozilla-central in the meantime).

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:rpl, could you have a look please?

For more information, please visit BugBot documentation.

Comment 4

[Vincent Hilla [:vhilla]]

:luca is right in #c1. waitForLoad can be removed to unblock the test and someone already made this change to the patch in the meantime.

But a new failure occurs in head_webrequest.js

origin is correct - Expected: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html, Actual: http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp&ignorePrefsFile=ignorePrefs.json

Looking at Pernosco https://pernos.co/debug/KuRoZwIabTW5J_m2Qmj4xg/index.html, I traced this incorrect originUrl back to MaybeCreateInitialClientSource, called from CreateAboutBlankDocumentViewer right before SimpleTest START. The iframe created in testing/mochitest/harness.xhtml starts out navigating to about:blank before being navigated to the test document in TestRunner.js. The initial blank document will have this http://mochi.test:8888/tests?autorun=1... principal, which the client source will adopt as mClientInfo.principalInfo.spec. When the iframe navigates away, the client source isn't changed. Much later, the test registers a service worker and ServiceWorkerManager::Register queries the client info for the principal that the web request ends up using. But it's not anymore the principal of the current document.

:asuth maybe this is yet another issue caused by window reuse? Maybe we need to reset more than the navigator?

Comment 5

[Vincent Hilla [:vhilla]]

nsGlobalWindowInner::EnsureClientSource compares the client source principal to the document principal via Equals (link). I'm unsure what definition of equality is used, but I guess only the origin http://mochi.test:8888 is considered. The principals are considered equal eventhough they got different URIs, and the client source is not reset. But the test is more strict and compares originUrl.

Comment 6

[Vincent Hilla [:vhilla]]

As pointed out by Rob, the test is now fixed. I checked, the changes in bug 1949168 is what fixed it.