Title: [272258] branches/safari-611-branch
- Revision
- 272258
- Author
- [email protected]
- Date
- 2021-02-02 17:39:53 -0800 (Tue, 02 Feb 2021)
Log Message
Cherry-pick r271770. rdar://problem/73890671
REGRESSION(r266148) Cancelling a navigation in decidePolicyForNavigationAction should not suspend the previous document's font loading timer
https://bugs.webkit.org/show_bug.cgi?id=220882
<rdar://problem/71634501>
Patch by Alex Christensen <[email protected]> on 2021-01-22
Reviewed by Brady Eidson.
Source/WebCore:
Things like the load event and didFinishNavigation wait until loading of things like fonts and iframes has completed.
If we navigate to a document that immediately tries to navigate to a different document after starting the loads of fonts,
then we momentarily have two DocumentLoaders with m_frame pointing to the same main frame.
If we cancel that second navigation using WKNavigationActionPolicyCancel, then it calls DocumentLoader::stopLoading on the second DocumentLoader.
This is fine. This is the way things have worked for a very long time.
r266148 introduced a call to the document's font loader's suspendFontLoadingTimer inside of DocumentLoader::stopLoading which is also fine.
What is not fine is the way we get that document. Using m_frame->document() in this case gets us the first document, which may still be loading fonts that we do still want.
Using this->document() only returns non-null if this DocumentLoader is the DocumentLoader that was used to load the Frame's Document,
and in this case the DocumentLoader should only stop the font loading timer if that is true.
I added an API test that reproduces the issue before but not after this fix.
For further verification, you can replace server.request() in my API test with a request to the URL in the radar ending in "Authentication.htm"
and verify it times out before the fix but not after.
This is close to the largest amount of time spent per character changed I've ever written to change "m_frame" to "this".
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::stopLoading):
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm:
(TEST):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271770 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-611-branch/Source/WebCore/ChangeLog (272257 => 272258)
--- branches/safari-611-branch/Source/WebCore/ChangeLog 2021-02-03 01:39:50 UTC (rev 272257)
+++ branches/safari-611-branch/Source/WebCore/ChangeLog 2021-02-03 01:39:53 UTC (rev 272258)
@@ -1,5 +1,77 @@
2021-02-02 Alan Coon <[email protected]>
+ Cherry-pick r271770. rdar://problem/73890671
+
+ REGRESSION(r266148) Cancelling a navigation in decidePolicyForNavigationAction should not suspend the previous document's font loading timer
+ https://bugs.webkit.org/show_bug.cgi?id=220882
+ <rdar://problem/71634501>
+
+ Patch by Alex Christensen <[email protected]> on 2021-01-22
+ Reviewed by Brady Eidson.
+
+ Source/WebCore:
+
+ Things like the load event and didFinishNavigation wait until loading of things like fonts and iframes has completed.
+
+ If we navigate to a document that immediately tries to navigate to a different document after starting the loads of fonts,
+ then we momentarily have two DocumentLoaders with m_frame pointing to the same main frame.
+
+ If we cancel that second navigation using WKNavigationActionPolicyCancel, then it calls DocumentLoader::stopLoading on the second DocumentLoader.
+ This is fine. This is the way things have worked for a very long time.
+
+ r266148 introduced a call to the document's font loader's suspendFontLoadingTimer inside of DocumentLoader::stopLoading which is also fine.
+ What is not fine is the way we get that document. Using m_frame->document() in this case gets us the first document, which may still be loading fonts that we do still want.
+ Using this->document() only returns non-null if this DocumentLoader is the DocumentLoader that was used to load the Frame's Document,
+ and in this case the DocumentLoader should only stop the font loading timer if that is true.
+
+ I added an API test that reproduces the issue before but not after this fix.
+ For further verification, you can replace server.request() in my API test with a request to the URL in the radar ending in "Authentication.htm"
+ and verify it times out before the fix but not after.
+
+ This is close to the largest amount of time spent per character changed I've ever written to change "m_frame" to "this".
+
+ * loader/DocumentLoader.cpp:
+ (WebCore::DocumentLoader::stopLoading):
+
+ Tools:
+
+ * TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm:
+ (TEST):
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271770 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2021-01-22 Alex Christensen <[email protected]>
+
+ REGRESSION(r266148) Cancelling a navigation in decidePolicyForNavigationAction should not suspend the previous document's font loading timer
+ https://bugs.webkit.org/show_bug.cgi?id=220882
+ <rdar://problem/71634501>
+
+ Reviewed by Brady Eidson.
+
+ Things like the load event and didFinishNavigation wait until loading of things like fonts and iframes has completed.
+
+ If we navigate to a document that immediately tries to navigate to a different document after starting the loads of fonts,
+ then we momentarily have two DocumentLoaders with m_frame pointing to the same main frame.
+
+ If we cancel that second navigation using WKNavigationActionPolicyCancel, then it calls DocumentLoader::stopLoading on the second DocumentLoader.
+ This is fine. This is the way things have worked for a very long time.
+
+ r266148 introduced a call to the document's font loader's suspendFontLoadingTimer inside of DocumentLoader::stopLoading which is also fine.
+ What is not fine is the way we get that document. Using m_frame->document() in this case gets us the first document, which may still be loading fonts that we do still want.
+ Using this->document() only returns non-null if this DocumentLoader is the DocumentLoader that was used to load the Frame's Document,
+ and in this case the DocumentLoader should only stop the font loading timer if that is true.
+
+ I added an API test that reproduces the issue before but not after this fix.
+ For further verification, you can replace server.request() in my API test with a request to the URL in the radar ending in "Authentication.htm"
+ and verify it times out before the fix but not after.
+
+ This is close to the largest amount of time spent per character changed I've ever written to change "m_frame" to "this".
+
+ * loader/DocumentLoader.cpp:
+ (WebCore::DocumentLoader::stopLoading):
+
+2021-02-02 Alan Coon <[email protected]>
+
Cherry-pick r271761. rdar://problem/73890346
Crash under FFTFrame::fftSetupForSize()
Modified: branches/safari-611-branch/Source/WebCore/loader/DocumentLoader.cpp (272257 => 272258)
--- branches/safari-611-branch/Source/WebCore/loader/DocumentLoader.cpp 2021-02-03 01:39:50 UTC (rev 272257)
+++ branches/safari-611-branch/Source/WebCore/loader/DocumentLoader.cpp 2021-02-03 01:39:53 UTC (rev 272258)
@@ -337,7 +337,7 @@
// Always cancel multipart loaders
cancelAll(m_multipartSubresourceLoaders);
- if (auto* document = m_frame->document())
+ if (auto* document = this->document())
document->fontSelector().suspendFontLoadingTimer();
// Appcache uses ResourceHandle directly, DocumentLoader doesn't count these loads.
Modified: branches/safari-611-branch/Tools/ChangeLog (272257 => 272258)
--- branches/safari-611-branch/Tools/ChangeLog 2021-02-03 01:39:50 UTC (rev 272257)
+++ branches/safari-611-branch/Tools/ChangeLog 2021-02-03 01:39:53 UTC (rev 272258)
@@ -1,3 +1,56 @@
+2021-02-02 Alan Coon <[email protected]>
+
+ Cherry-pick r271770. rdar://problem/73890671
+
+ REGRESSION(r266148) Cancelling a navigation in decidePolicyForNavigationAction should not suspend the previous document's font loading timer
+ https://bugs.webkit.org/show_bug.cgi?id=220882
+ <rdar://problem/71634501>
+
+ Patch by Alex Christensen <[email protected]> on 2021-01-22
+ Reviewed by Brady Eidson.
+
+ Source/WebCore:
+
+ Things like the load event and didFinishNavigation wait until loading of things like fonts and iframes has completed.
+
+ If we navigate to a document that immediately tries to navigate to a different document after starting the loads of fonts,
+ then we momentarily have two DocumentLoaders with m_frame pointing to the same main frame.
+
+ If we cancel that second navigation using WKNavigationActionPolicyCancel, then it calls DocumentLoader::stopLoading on the second DocumentLoader.
+ This is fine. This is the way things have worked for a very long time.
+
+ r266148 introduced a call to the document's font loader's suspendFontLoadingTimer inside of DocumentLoader::stopLoading which is also fine.
+ What is not fine is the way we get that document. Using m_frame->document() in this case gets us the first document, which may still be loading fonts that we do still want.
+ Using this->document() only returns non-null if this DocumentLoader is the DocumentLoader that was used to load the Frame's Document,
+ and in this case the DocumentLoader should only stop the font loading timer if that is true.
+
+ I added an API test that reproduces the issue before but not after this fix.
+ For further verification, you can replace server.request() in my API test with a request to the URL in the radar ending in "Authentication.htm"
+ and verify it times out before the fix but not after.
+
+ This is close to the largest amount of time spent per character changed I've ever written to change "m_frame" to "this".
+
+ * loader/DocumentLoader.cpp:
+ (WebCore::DocumentLoader::stopLoading):
+
+ Tools:
+
+ * TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm:
+ (TEST):
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271770 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2021-01-22 Alex Christensen <[email protected]>
+
+ REGRESSION(r266148) Cancelling a navigation in decidePolicyForNavigationAction should not suspend the previous document's font loading timer
+ https://bugs.webkit.org/show_bug.cgi?id=220882
+ <rdar://problem/71634501>
+
+ Reviewed by Brady Eidson.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm:
+ (TEST):
+
2021-01-29 Alan Coon <[email protected]>
Cherry-pick r271507. rdar://problem/73473467
Modified: branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm (272257 => 272258)
--- branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm 2021-02-03 01:39:50 UTC (rev 272257)
+++ branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm 2021-02-03 01:39:53 UTC (rev 272258)
@@ -772,3 +772,54 @@
[webView removeObserver:observer forKeyPath:@"loading"];
}
+
+TEST(WKNavigation, SimultaneousNavigationWithFontsFinishes)
+{
+ const char* mainHTML =
+ "<!DOCTYPE html>"
+ "<html>"
+ "<head>"
+ "<style>"
+ "@font-face {"
+ " font-family: 'WebFont';"
+ " src: url('Ahem.svg') format('svg');"
+ "}"
+ "</style>"
+ "<script src=''></script>"
+ "</head>"
+ "<body>"
+ "<span style=\"font: 100px 'WebFont';\">text</span>"
+ "<iframe src=''></iframe>"
+ "<script>window.location='refresh-nav:///'</script>"
+ "</body>"
+ "</html>";
+
+ NSString *svg = [NSString stringWithContentsOfURL:[[NSBundle mainBundle] URLForResource:@"AllAhem" withExtension:@"svg" subdirectory:@"TestWebKitAPI.resources"] encoding:NSUTF8StringEncoding error:nil];
+
+ using namespace TestWebKitAPI;
+ HTTPServer server({
+ { "/", { mainHTML } },
+ { "Ahem.svg", { svg } },
+ { "/scriptsrc.js", { "/* js content */" } },
+ { "/iframesrc.html", { "frame content" } },
+ });
+
+ auto webView = [[WKWebView new] autorelease];
+ auto delegate = [[TestNavigationDelegate new] autorelease];
+ webView.navigationDelegate = delegate;
+
+ delegate.decidePolicyForNavigationAction = ^(WKNavigationAction *action, void (^completionHandler)(WKNavigationActionPolicy)) {
+ if ([action.request.URL.scheme isEqualToString:@"refresh-nav"])
+ completionHandler(WKNavigationActionPolicyCancel);
+ else
+ completionHandler(WKNavigationActionPolicyAllow);
+ };
+
+ __block bool finishedNavigation = false;
+ delegate.didFinishNavigation = ^(WKWebView *, WKNavigation *) {
+ finishedNavigation = true;
+ };
+
+ [webView loadRequest:server.request()];
+ Util::run(&finishedNavigation);
+}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
