Attachment 338589 Details for Bug 184848 – Patch

[patch] Patch

bug-184848-20180423085801.patch (text/plain), 11.74 KB, created by Chris Dumez on 2018-04-23 08:58:01 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-04-23 08:58:01 PDT

Size: 11.74 KB

patch

obsolete

>Subversion Revision: 230915
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 182e635413790c4e3bfe31aac2ad084cded18d9b..547129f9a3f32430df431a941078c7bf28d9076d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-04-23  Chris Dumez  <cdumez@apple.com>
>+
>+        HTML String load cannot be prevented by responding 'Cancel' asynchronously in decidePolicyForNavigationAction
>+        https://bugs.webkit.org/show_bug.cgi?id=184848
>+        <rdar://problem/39145306>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When calling loadHTMLString on a WebView, we end up doing a load for 'about:blank'
>+        with substitute data. In such case, we want to do a regular asynchronous policy
>+        delegate check, there is no reason we need it to be synchronous. Update our check
>+        to make sure we only do a synchronous policy check for initial 'about:blank' loads
>+        that do not have substitute data.
>+
>+        * loader/PolicyChecker.cpp:
>+        (WebCore::PolicyChecker::checkNavigationPolicy):
>+
> 2018-04-23  Zalan Bujtas  <zalan@apple.com>
> 
>         [Simple line layout] Generate inline boxtree using simple line layout runs.
>diff --git a/Source/WebCore/loader/PolicyChecker.cpp b/Source/WebCore/loader/PolicyChecker.cpp
>index fdeda3f7e9a5f464ef14045fd15c7c8dc0597894..fc2419498384d22bdf6d7a7d926ec53c6b04ad58 100644
>--- a/Source/WebCore/loader/PolicyChecker.cpp
>+++ b/Source/WebCore/loader/PolicyChecker.cpp
>@@ -123,7 +123,8 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
> 
>     loader->setLastCheckedRequest(ResourceRequest(request));
> 
>-    if (request.url().isBlankURL())
>+    // Initial 'about:blank' load needs to happen synchronously so the policy check needs to be synchronous in this case.
>+    if (!m_frame.loader().stateMachine().committedFirstRealDocumentLoad() && request.url().isBlankURL() && !substituteData.isValid())
>         policyDecisionMode = PolicyDecisionMode::Synchronous;
> 
> #if USE(QUICK_LOOK)
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 586ce6623439415f16cbfbfdcf4202a7c8df5fa8..387fe38b6996650ac886234e82b9654066f17d71 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-04-23  Chris Dumez  <cdumez@apple.com>
>+
>+        HTML String load cannot be prevented by responding 'Cancel' asynchronously in decidePolicyForNavigationAction
>+        https://bugs.webkit.org/show_bug.cgi?id=184848
>+        <rdar://problem/39145306>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:
>+        (-[DecidePolicyForNavigationActionController webView:decidePolicyForNavigationAction:decisionHandler:]):
>+        (TEST):
>+
> 2018-04-23  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [GTK][WPE] TestSSL fails due to additional TLS errors returned
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm
>index 31116aac213b579f7b6ffbab218708d60563547d..8a111278c84d003750bb93686c291de3bb76970f 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm
>@@ -35,6 +35,7 @@
> 
> #if WK_API_ENABLED
> 
>+static bool shouldCancelNavigation;
> static bool createdWebView;
> static bool decidedPolicy;
> static bool finishedNavigation;
>@@ -51,6 +52,16 @@ static NSString *secondURL = @"data:text/html,Second";
> 
> - (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))decisionHandler
> {
>+    if (shouldCancelNavigation) {
>+        int64_t deferredWaitTime = 100 * NSEC_PER_MSEC;
>+        dispatch_time_t when = dispatch_time(DISPATCH_TIME_NOW, deferredWaitTime);
>+        dispatch_after(when, dispatch_get_main_queue(), ^{
>+            decisionHandler(WKNavigationActionPolicyCancel);
>+            decidedPolicy = true;
>+        });
>+        return;
>+    }
>+
>     decisionHandler(webView == newWebView.get() ? WKNavigationActionPolicyCancel : WKNavigationActionPolicyAllow);
> 
>     action = navigationAction;
>@@ -285,7 +296,7 @@ TEST(WebKit, DecidePolicyForNavigationActionForTargetedHyperlink)
>     action = nullptr;
> }
> 
>-TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLString)
>+TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLStringAllow)
> {
>     auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
> 
>@@ -296,9 +307,31 @@ TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLString)
>     [webView setNavigationDelegate:controller.get()];
>     [webView setUIDelegate:controller.get()];
> 
>+    finishedNavigation = false;
>     decidedPolicy = false;
>     [webView loadHTMLString:@"TEST" baseURL:[NSURL URLWithString:@"about:blank"]];
>-    TestWebKitAPI::Util::run(&decidedPolicy);
>+    TestWebKitAPI::Util::run(&finishedNavigation);
>+    EXPECT_TRUE(decidedPolicy);
>+}
>+
>+TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLStringDeny)
>+{
>+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
>+
>+    auto window = adoptNS([[NSWindow alloc] initWithContentRect:[webView frame] styleMask:NSWindowStyleMaskBorderless backing:NSBackingStoreBuffered defer:YES]);
>+    [[window contentView] addSubview:webView.get()];
>+
>+    auto controller = adoptNS([[DecidePolicyForNavigationActionController alloc] init]);
>+    [webView setNavigationDelegate:controller.get()];
>+    [webView setUIDelegate:controller.get()];
>+
>+    shouldCancelNavigation = true;
>+    finishedNavigation = false;
>+    decidedPolicy = false;
>+    [webView loadHTMLString:@"TEST" baseURL:[NSURL URLWithString:@"about:blank"]];
>+    TestWebKitAPI::Util::sleep(0.5);
>+    EXPECT_FALSE(finishedNavigation);
>+    shouldCancelNavigation = false;
> }
> 
> TEST(WebKit, DecidePolicyForNavigationActionForTargetedWindowOpen)
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 06a5799c29614b517274d74d045cb8e6b9ea5d7a..4260b93f98d623c595c8ae1a81b5dd641d068892 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-04-23  Chris Dumez  <cdumez@apple.com>
>+
>+        HTML String load cannot be prevented by responding 'Cancel' asynchronously in decidePolicyForNavigationAction
>+        https://bugs.webkit.org/show_bug.cgi?id=184848
>+        <rdar://problem/39145306>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Update layout tests that wrongly expected 'about:blank' to load synchronously even
>+        when it is not the initial empty document of an iframe. I have checked that our
>+        behavior is now consistent with Chrome.
>+
>+        * fast/events/beforeunload-alert-user-interaction2.html:
>+        * http/tests/security/cross-origin-reified-window-location-setting-expected.txt:
>+        * http/tests/security/cross-origin-reified-window-location-setting.html:
>+        * webarchive/loading/javascript-url-iframe-crash-expected.txt:
>+
> 2018-04-22  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Add a layout test for r230785
>diff --git a/LayoutTests/fast/events/beforeunload-alert-user-interaction2.html b/LayoutTests/fast/events/beforeunload-alert-user-interaction2.html
>index 9969a12f7a2ff00f8e3987133ef4e378a4fac270..184d8b5b6159f04e7ce99278902bc5fd3fa775df 100644
>--- a/LayoutTests/fast/events/beforeunload-alert-user-interaction2.html
>+++ b/LayoutTests/fast/events/beforeunload-alert-user-interaction2.html
>@@ -16,8 +16,8 @@ onload = function() {
>     const testInput = document.getElementById("testInput");
>     UIHelper.activateAt(testInput.offsetLeft + 5, testInput.offsetTop + 5).then(function() {
>         setTimeout(function() {
>+            testFrame.onload = finishJSTest;
>             testFrame.src = "about:blank";
>-            setTimeout(finishJSTest, 0);
>         }, 0);
>     });
> };
>diff --git a/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt b/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt
>index b6ba68ca6210075c83308ad690129da33a3165d9..18792da2c21512b9e83e630cddf2830a696a158d 100644
>--- a/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt
>+++ b/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt
>@@ -3,7 +3,9 @@ Tests that window.location can be set cross-origin even if the window object is
> On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> 
>+PASS crossOriginWindow.location.href threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
> PASS crossOriginWindow.location = 'about:blank' did not throw exception.
>+PASS crossOriginWindow.location.href threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
> PASS crossOriginWindow.location.href is "about:blank"
> PASS successfullyParsed is true
> 
>diff --git a/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html b/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html
>index db280f997dfb9e8e8ac3174d1e0a669b498f4869..d62003bd8014919f1b68b69b32bfc7f8b6baa874 100644
>--- a/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html
>+++ b/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html
>@@ -1,7 +1,7 @@
> <!DOCTYPE html>
> <html>
> <head>
>-<script src="../../resources/js-test-pre.js"></script>
>+<script src="/js-test-resources/js-test.js"></script>
> </head>
> <body onload="runTest()">
> <iframe id="crossOriginFrame" src="http://localhost:8000/security/resources/reify-window.html"></iframe>
>@@ -11,15 +11,19 @@ jsTestIsAsync = true;
> 
> function runTest()
> {
>-    crossOriginWindow = crossOriginFrame.window;
>+    crossOriginWindow = document.getElementById("crossOriginFrame").contentWindow;
>+    shouldThrow("crossOriginWindow.location.href");
>     shouldNotThrow("crossOriginWindow.location = 'about:blank'");
>-
>-    setTimeout(function() {
>-        shouldBeEqualToString("crossOriginWindow.location.href", "about:blank");
>-        finishJSTest();
>-    }, 0);
>+    shouldThrow("crossOriginWindow.location.href");
>+    handle = setInterval(function() {
>+        try {
>+            crossOriginWindow.location.href;
>+            shouldBeEqualToString("crossOriginWindow.location.href", "about:blank");
>+            clearInterval(handle);
>+            finishJSTest();
>+        } catch(e) { }
>+    }, 5);
> }
> </script>
> </body>
>-<script src="../../resources/js-test-post.js"></script>
> </html>
>diff --git a/LayoutTests/platform/wk2/webarchive/loading/javascript-url-iframe-crash-expected.txt b/LayoutTests/platform/wk2/webarchive/loading/javascript-url-iframe-crash-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..aafec5d39574aece29928eb2a987478a2216f30f
>--- /dev/null
>+++ b/LayoutTests/platform/wk2/webarchive/loading/javascript-url-iframe-crash-expected.txt
>@@ -0,0 +1,15 @@
>+main frame - didStartProvisionalLoadForFrame
>+main frame - didCommitLoadForFrame
>+main frame - willPerformClientRedirectToURL: resources/javascript-url-iframe-crash.webarchive 
>+main frame - didFinishDocumentLoadForFrame
>+main frame - didFinishLoadForFrame
>+main frame - didStartProvisionalLoadForFrame
>+main frame - didCancelClientRedirectForFrame
>+main frame - didCommitLoadForFrame
>+frame "-->" - didFinishDocumentLoadForFrame
>+frame "-->" - didHandleOnloadEventsForFrame
>+main frame - didFinishDocumentLoadForFrame
>+main frame - didHandleOnloadEventsForFrame
>+main frame - didFinishLoadForFrame
>+Loading this webarchive with a "non-empty javascript URL iframe" should not crash.
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 184848: 338483 | 338493 | 338508 | 338518 | 338521 | 338523 | 338589