WKWebExtensionAPITabs.ExecuteScript test has a race condition and fai…

…ls sometimes. https://webkit.org/b/264754 rdar://problem/118344217 Reviewed by Brian Weinstein. Use the onUpdated or onCompleted events to know when the tab navigates so we can execute script. Also properly fire the onUpdated event by using web view navigation delegate methods. * Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPITabs.mm: (TestWebKitAPI::TEST): * Tools/TestWebKitAPI/cocoa/WebExtensionUtilities.mm: (-[TestWebExtensionTab initWithWindow:extensionController:]): Set navigationDelegate. (-[TestWebExtensionTab changeWebViewIfNeededForURL:forExtensionContext:]): Ditto. (-[TestWebExtensionTab webView:didStartProvisionalNavigation:]): Added. (-[TestWebExtensionTab webView:didReceiveServerRedirectForProvisionalNavigation:]): Added. (-[TestWebExtensionTab webView:didFailNavigation:withError:]): Added. (-[TestWebExtensionTab webView:didCommitNavigation:]): Added. (-[TestWebExtensionTab webView:didFinishNavigation:]): Added. (-[TestWebExtensionTab setParentTab:forWebExtensionContext:completionHandler:]): Added cast to fix error. Canonical link: https://commits.webkit.org/270658@main
WebKit · Nov 13, 2023 · 204a096 · 204a096
1 parent abd1c3e
commit 204a096
Show file tree

Hide file tree

Showing 3 changed files with 121 additions and 58 deletions.
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIScripting.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIScripting.mm
@@ -38,7 +38,7 @@
     @"description": @"Scripting Test",
     @"version": @"1",
 
-    @"permissions": @[ @"scripting" ],
+    @"permissions": @[ @"scripting", @"webNavigation" ],
     @"host_permissions": @[ @"*://localhost/*" ],
 
     @"background": @{
@@ -99,35 +99,38 @@
     }, TestWebKitAPI::HTTPServer::Protocol::Http);
 
     auto *backgroundScript = Util::constructScript(@[
-        @"const blueValue = 'rgb(0, 0, 255)'",
+        @"browser.webNavigation.onCompleted.addListener(async (details) => {",
+        @"  const tabId = details.tabId",
 
-        @"const tabs = await browser.tabs.query({ active: true, currentWindow: true })",
-        @"const tabId = tabs[0].id",
+        @"  const blueValue = 'rgb(0, 0, 255)'",
 
-        @"const expectedResultWithFileExecution = { 'result': 'pink', 'frameId': 0 }",
-        @"const expectedResultWithFunctionExecution = { 'result': null, 'frameId': 0 }",
+        @"  const expectedResultWithFileExecution = { 'result': 'pink', 'frameId': 0 }",
+        @"  const expectedResultWithFunctionExecution = { 'result': null, 'frameId': 0 }",
 
-        @"function changeBackgroundColor(color) { document.body.style.background = color }",
-        @"function getBackgroundColor() { return window.getComputedStyle(document.body).getPropertyValue('background-color') }",
+        @"  function changeBackgroundColor(color) { document.body.style.background = color }",
+        @"  function getBackgroundColor() { return window.getComputedStyle(document.body).getPropertyValue('background-color') }",
 
-        @"let results = await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: false }, files: [ 'backgroundColor.js' ] })",
-        @"browser.test.assertDeepEq(results[0], expectedResultWithFileExecution)",
+        @"  let results = await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: false }, files: [ 'backgroundColor.js' ] })",
+        @"  browser.test.assertDeepEq(results[0], expectedResultWithFileExecution)",
 
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ 0 ] }, files: [ 'backgroundColor.js' ] })",
-        @"browser.test.assertDeepEq(results[0], expectedResultWithFileExecution)",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ 0 ] }, files: [ 'backgroundColor.js' ] })",
+        @"  browser.test.assertDeepEq(results[0], expectedResultWithFileExecution)",
 
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId }, files: [ 'backgroundColor.js'] })",
-        @"browser.test.assertDeepEq(results[0], expectedResultWithFileExecution)",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId }, files: [ 'backgroundColor.js'] })",
+        @"  browser.test.assertDeepEq(results[0], expectedResultWithFileExecution)",
 
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ 0 ] }, func: changeBackgroundColor, args: ['pink'] })",
-        @"browser.test.assertDeepEq(results[0], expectedResultWithFunctionExecution)",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ 0 ] }, func: changeBackgroundColor, args: ['pink'] })",
+        @"  browser.test.assertDeepEq(results[0], expectedResultWithFunctionExecution)",
 
-        @"await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: true }, func: changeBackgroundColor, args: ['blue'] })",
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: true }, func: getBackgroundColor })",
-        @"browser.test.assertEq(results[0].result, blueValue)",
-        @"browser.test.assertEq(results[0].result, blueValue)",
+        @"  await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: true }, func: changeBackgroundColor, args: ['blue'] })",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: true }, func: getBackgroundColor })",
+        @"  browser.test.assertEq(results[0].result, blueValue)",
+        @"  browser.test.assertEq(results[0].result, blueValue)",
 
-        @"browser.test.notifyPass()"
+        @"  browser.test.notifyPass()",
+        @"})",
+
+        @"browser.test.yield('Load Tab')",
     ]);
 
     static auto *backgroundColor = @"document.body.style.background = 'pink'";
@@ -141,10 +144,16 @@
     auto *url = urlRequest.URL;
 
     auto *matchPattern = [_WKWebExtensionMatchPattern matchPatternWithScheme:url.scheme host:url.host path:@"/*"];
+    [manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forPermission:_WKWebExtensionPermissionWebNavigation];
     [manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forMatchPattern:matchPattern];
-    [manager.get().defaultTab.mainWebView loadRequest:urlRequest];
 
     [manager loadAndRun];
+
+    EXPECT_NS_EQUAL(manager.get().yieldMessage, @"Load Tab");
+
+    [manager.get().defaultTab.mainWebView loadRequest:urlRequest];
+
+    [manager run];
 }
 
 TEST(WKWebExtensionAPIScripting, ExecuteScriptWithFrameIds)
@@ -155,38 +164,44 @@
     }, TestWebKitAPI::HTTPServer::Protocol::Http);
 
     auto *backgroundScript = Util::constructScript(@[
-        @"const pinkValue = 'rgb(255, 192, 203)'",
-        @"const blueValue = 'rgb(0, 0, 255)'",
+        @"browser.webNavigation.onCompleted.addListener(async (details) => {",
+        @"  if (details.frameId !== 0)",
+        @"    return",
 
-        @"const tabs = await browser.tabs.query({ active: true, currentWindow: true })",
-        @"const tabId = tabs[0].id",
+        @"  const tabId = details.tabId",
 
-        @"function changeBackgroundColor(color) { document.body.style.background = color }",
-        @"function getBackgroundColor() { return window.getComputedStyle(document.body).getPropertyValue('background-color') }",
-        @"function getFontSize() { return window.getComputedStyle(document.body).getPropertyValue('font-size') }",
+        @"  const pinkValue = 'rgb(255, 192, 203)'",
+        @"  const blueValue = 'rgb(0, 0, 255)'",
 
-        @"function logMessage() { console.log('Logging message') }",
+        @"  function changeBackgroundColor(color) { document.body.style.background = color }",
+        @"  function getBackgroundColor() { return window.getComputedStyle(document.body).getPropertyValue('background-color') }",
+        @"  function getFontSize() { return window.getComputedStyle(document.body).getPropertyValue('font-size') }",
+
+        @"  function logMessage() { console.log('Logging message') }",
 
         // FIXME: <https://webkit.org/b/260160> A better way to get the frame Ids would be from browser.webNavigation.getAllFrames().
-        @"let results = await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: true }, func: logMessage })",
-        @"browser.test.assertEq(results.length, 2)",
+        @"  let results = await browser.scripting.executeScript( { target: { tabId: tabId, allFrames: true }, func: logMessage })",
+        @"  browser.test.assertEq(results.length, 2)",
 
-        @"let subframe = results[1].frameId",
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ 0, subframe ] }, files: [ 'backgroundColor.js' ]})",
-        @"browser.test.assertEq(results[0].result, 'pink')",
-        @"browser.test.assertEq(results[1].result, 'pink')",
+        @"  let subframe = results[1].frameId",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ 0, subframe ] }, files: [ 'backgroundColor.js' ]})",
+        @"  browser.test.assertEq(results[0].result, 'pink')",
+        @"  browser.test.assertEq(results[1].result, 'pink')",
 
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: changeBackgroundColor, args: [ 'blue' ] })",
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: getBackgroundColor })",
-        @"browser.test.assertEq(results[0].result, blueValue)",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: changeBackgroundColor, args: [ 'blue' ] })",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: getBackgroundColor })",
+        @"  browser.test.assertEq(results[0].result, blueValue)",
 
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, files: [ 'backgroundColor.js', 'fontSize.js' ] })",
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: getBackgroundColor })",
-        @"browser.test.assertEq(results[0].result, pinkValue)",
-        @"results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: getFontSize })",
-        @"browser.test.assertEq(results[0].result, '104px')",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, files: [ 'backgroundColor.js', 'fontSize.js' ] })",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: getBackgroundColor })",
+        @"  browser.test.assertEq(results[0].result, pinkValue)",
+        @"  results = await browser.scripting.executeScript( { target: { tabId: tabId, frameIds: [ subframe ] }, func: getFontSize })",
+        @"  browser.test.assertEq(results[0].result, '104px')",
 
-        @"browser.test.notifyPass()"
+        @"  browser.test.notifyPass()",
+        @"})",
+
+        @"browser.test.yield('Load Tab')",
     ]);
 
     static auto *backgroundColor = @"document.body.style.background = 'pink'";
@@ -205,10 +220,16 @@
     auto *url = urlRequest.URL;
 
     auto *matchPattern = [_WKWebExtensionMatchPattern matchPatternWithScheme:url.scheme host:url.host path:@"/*"];
+    [manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forPermission:_WKWebExtensionPermissionWebNavigation];
     [manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forMatchPattern:matchPattern];
-    [manager.get().defaultTab.mainWebView loadRequest:urlRequest];
 
     [manager loadAndRun];
+
+    EXPECT_NS_EQUAL(manager.get().yieldMessage, @"Load Tab");
+
+    [manager.get().defaultTab.mainWebView loadRequest:urlRequest];
+
+    [manager run];
 }
 
 TEST(WKWebExtensionAPIScripting, InsertAndRemoveCSS)

diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPITabs.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPITabs.mm
@@ -1497,20 +1497,26 @@
         @"const tabs = await browser.tabs.query({ active: true, currentWindow: true })",
         @"const tabId = tabs[0].id",
 
-        @"let results = await browser.tabs.executeScript(tabId, { allFrames: false, file: 'executeScript.js' })",
-        @"browser.test.assertEq(results[0], 'pink')",
+        @"browser.tabs.onUpdated.addListener(async (tabId, changeInfo, tab) => {",
+        @"  if (changeInfo.url) {",
+        @"    let results = await browser.tabs.executeScript(tabId, { allFrames: false, file: 'executeScript.js' })",
+        @"    browser.test.assertEq(results[0], 'pink')",
 
-        @"results = await browser.tabs.executeScript(tabId, { frameId: 0, file: 'executeScript.js' })",
-        @"browser.test.assertEq(results[0], 'pink')",
+        @"    results = await browser.tabs.executeScript(tabId, { frameId: 0, file: 'executeScript.js' })",
+        @"    browser.test.assertEq(results[0], 'pink')",
 
-        @"results = await browser.tabs.executeScript(tabId, { allFrames: true, file: 'executeScript.js' })",
-        @"browser.test.assertEq(results[0], 'pink')",
-        @"browser.test.assertEq(results[1], 'pink')",
+        @"    results = await browser.tabs.executeScript(tabId, { allFrames: true, file: 'executeScript.js' })",
+        @"    browser.test.assertEq(results[0], 'pink')",
+        @"    browser.test.assertEq(results[1], 'pink')",
 
-        @"results = await browser.tabs.executeScript(tabId, { allFrames: false, code: \"document.body.style.background = 'pink'\" })",
-        @"browser.test.assertEq(results[0], 'pink')",
+        @"    results = await browser.tabs.executeScript(tabId, { allFrames: false, code: \"document.body.style.background = 'pink'\" })",
+        @"    browser.test.assertEq(results[0], 'pink')",
 
-        @"browser.test.notifyPass()"
+        @"    browser.test.notifyPass()",
+        @"  }",
+        @"})",
+
+        @"browser.test.yield('Load Tab')",
     ]);
 
     auto extension = adoptNS([[_WKWebExtension alloc] _initWithManifestDictionary:tabsManifestV2 resources:@{ @"background.js": backgroundScript, @"executeScript.js": javaScript }]);
@@ -1520,9 +1526,14 @@
     auto *url = urlRequest.URL;
     auto *matchPattern = [_WKWebExtensionMatchPattern matchPatternWithScheme:url.scheme host:url.host path:@"/*"];
     [manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forMatchPattern:matchPattern];
-    [manager.get().defaultTab.mainWebView loadRequest:urlRequest];
 
     [manager loadAndRun];
+
+    EXPECT_NS_EQUAL(manager.get().yieldMessage, @"Load Tab");
+
+    [manager.get().defaultTab.mainWebView loadRequest:urlRequest];
+
+    [manager run];
 }
 
 TEST(WKWebExtensionAPITabs, InsertAndRemoveCSSInMainFrame)

diff --git a/Tools/TestWebKitAPI/cocoa/WebExtensionUtilities.mm b/Tools/TestWebKitAPI/cocoa/WebExtensionUtilities.mm
@@ -296,6 +296,9 @@ - (void)_webExtensionController:(_WKWebExtensionController *)controller recordTe
     return usingPrivateBrowsing ? privateController : normalController;
 }
 
+@interface TestWebExtensionTab () <WKNavigationDelegate>
+@end
+
 @implementation TestWebExtensionTab {
     __weak _WKWebExtensionController *_extensionController;
 }
@@ -321,6 +324,8 @@ - (instancetype)initWithWindow:(TestWebExtensionWindow *)window extensionControl
         configuration.userContentController = userContentController(usingPrivateBrowsing);
 
         _mainWebView = [[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration];
+        _mainWebView.navigationDelegate = self;
+
         _extensionController = extensionController;
     }
 
@@ -345,6 +350,32 @@ - (void)changeWebViewIfNeededForURL:(NSURL *)url forExtensionContext:(_WKWebExte
     configuration.userContentController = userContentController(usingPrivateBrowsing);
 
     _mainWebView = [[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration];
+    _mainWebView.navigationDelegate = self;
+}
+
+- (void)webView:(WKWebView *)webView didStartProvisionalNavigation:(WKNavigation *)navigation
+{
+    [_extensionController didChangeTabProperties:_WKWebExtensionTabChangedPropertiesURL | _WKWebExtensionTabChangedPropertiesLoading forTab:self];
+}
+
+- (void)webView:(WKWebView *)webView didReceiveServerRedirectForProvisionalNavigation:(WKNavigation *)navigation
+{
+    [_extensionController didChangeTabProperties:_WKWebExtensionTabChangedPropertiesURL forTab:self];
+}
+
+- (void)webView:(WKWebView *)webView didFailNavigation:(WKNavigation *)navigation withError:(NSError *)error
+{
+    [_extensionController didChangeTabProperties:_WKWebExtensionTabChangedPropertiesURL | _WKWebExtensionTabChangedPropertiesLoading forTab:self];
+}
+
+- (void)webView:(WKWebView *)webView didCommitNavigation:(WKNavigation *)navigation
+{
+    [_extensionController didChangeTabProperties:_WKWebExtensionTabChangedPropertiesURL forTab:self];
+}
+
+- (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation
+{
+    [_extensionController didChangeTabProperties:_WKWebExtensionTabChangedPropertiesLoading forTab:self];
 }
 
 - (WKWebView *)mainWebViewForWebExtensionContext:(_WKWebExtensionContext *)context
@@ -424,7 +455,7 @@ - (void)goForwardForWebExtensionContext:(_WKWebExtensionContext *)context comple
 
 - (void)setParentTab:(id<_WKWebExtensionTab>)parentTab forWebExtensionContext:(_WKWebExtensionContext *)context completionHandler:(void (^)(NSError *))completionHandler
 {
-    _parentTab = parentTab;
+    _parentTab = dynamic_objc_cast<TestWebExtensionTab>(parentTab);
 
     completionHandler(nil);
 }