Attachment 340887 Details for Bug 185832 – Patch

[patch] Patch

bug-185832-20180521141355.patch (text/plain), 13.25 KB, created by Chris Dumez on 2018-05-21 14:13:56 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-05-21 14:13:56 PDT

Size: 13.25 KB

patch

obsolete

>Subversion Revision: 232025
>diff --git a/Source/WebKitLegacy/mac/ChangeLog b/Source/WebKitLegacy/mac/ChangeLog
>index c3d63f3b5808f6d02be3be48019cfc76df7f7650..8bdfbac27fe5fb7e3e299610a679ddecd10c16b4 100644
>--- a/Source/WebKitLegacy/mac/ChangeLog
>+++ b/Source/WebKitLegacy/mac/ChangeLog
>@@ -1,3 +1,32 @@
>+2018-05-21  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(AsyncPolicyDelegates): Box.app login Window is blank
>+        https://bugs.webkit.org/show_bug.cgi?id=185832
>+        <rdar://problem/40307871>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We used to have a bug where where we would fail to wait for the policy decision for
>+        the navigation response from the client and the load would keep going, racing with
>+        the client's policy decision. If the client did not respond in time, the behavior
>+        would be the same as "Use" policy action.
>+
>+        Box.app fails to make any policy decision in its decidePolicyForMIMEType delegate
>+        but the load happened to proceed anyway due to our bug. Now that we've fixed the 
>+        WebKit bug, however, the load would hang because the completion handler for the
>+        decidePolicyForNavigationResponse would never get called.
>+
>+        To work around the issue, I made the policy listener weak on the WebFrameLoaderClient
>+        instead of retaining it. If the policy listener object gets destroyed because getting
>+        resolved, we now use "Use" policy action in its dealloc function to maintain previous
>+        behavior.
>+
>+        * WebCoreSupport/WebFrameLoaderClient.h:
>+        * WebCoreSupport/WebFrameLoaderClient.mm:
>+        (WebFrameLoaderClient::cancelPolicyCheck):
>+        (WebFrameLoaderClient::setUpPolicyListener):
>+        (-[WebFramePolicyListener dealloc]):
>+
> 2018-05-21  Jer Noble  <jer.noble@apple.com>
> 
>         Complete fix for enabling modern EME by default
>diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>index a2c7932f68269976c2933694c3eedc6029652dd1..799b0e6ab950b03bdeadeeff2fdd3c67be55f4fc 100644
>--- a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>+++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>@@ -260,5 +260,5 @@ private:
> 
>     RetainPtr<WebFrame> m_webFrame;
> 
>-    RetainPtr<WebFramePolicyListener> m_policyListener;
>+    __weak WebFramePolicyListener *m_policyListener;
> };
>diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>index 1a411348a8eb6c77d87edafcdc49ed1859b5f616..5e465d7af5326a6c2460fdf89051d29608ff7d7f 100644
>--- a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>+++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>@@ -914,8 +914,11 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
> 
> void WebFrameLoaderClient::cancelPolicyCheck()
> {
>+    if (!m_policyListener)
>+        return;
>+
>     [m_policyListener invalidate];
>-    m_policyListener = nullptr;
>+    m_policyListener = nil;
> }
> 
> void WebFrameLoaderClient::dispatchUnableToImplementPolicy(const ResourceError& error)
>@@ -1524,14 +1527,17 @@ RetainPtr<WebFramePolicyListener> WebFrameLoaderClient::setUpPolicyListener(Fram
>     // FIXME: <rdar://5634381> We need to support multiple active policy listeners.
>     [m_policyListener invalidate];
> 
>+    RetainPtr<WebFramePolicyListener> policyListener;
> #if HAVE(APP_LINKS)
>     if (appLinkURL)
>-        m_policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function) appLinkURL:appLinkURL]);
>+        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function) appLinkURL:appLinkURL]);
>     else
> #endif
>-        m_policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function)]);
>+        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function)]);
>+
>+    m_policyListener = policyListener.get();
> 
>-    return m_policyListener;
>+    return policyListener;
> }
> 
> String WebFrameLoaderClient::userAgent(const URL& url)
>@@ -2426,6 +2432,14 @@ - (void)dealloc
>     if (WebCoreObjCScheduleDeallocateOnMainThread([WebFramePolicyListener class], self))
>         return;
> 
>+    // If the app did not respond before the listener is destroyed, then we let the load
>+    // proceed with policy "Use".
>+    _frame = nullptr;
>+    if (auto policyFunction = std::exchange(_policyFunction, nullptr)) {
>+        RELEASE_LOG_ERROR(Loading, "Client application failed to make a policy decision via WebPolicyDecisionListener, letting the load proceed");
>+        policyFunction(PolicyAction::Use);
>+    }
>+
>     [super dealloc];
> }
> 
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index db55e3c883da710b03e8cd9a1f22b97987cc671c..29cefe9fe95fea9afcab540a7a2a93e8a2ddfebe 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-05-21  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(AsyncPolicyDelegates): Box.app login Window is blank
>+        https://bugs.webkit.org/show_bug.cgi?id=185832
>+        <rdar://problem/40307871>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
>+        * TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm: Added.
>+        (-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForNavigationAction:request:frame:decisionListener:]):
>+        (-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForMIMEType:request:frame:decisionListener:]):
>+        (-[NoPolicyDelegateDecisionDelegate webView:didFinishLoadForFrame:]):
>+        (TestWebKitAPI::TEST):
>+
> 2018-05-21  Jer Noble  <jer.noble@apple.com>
> 
>         Complete fix for enabling modern EME by default
>diff --git a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>index 0372673c68009200fbb179812d8dcc90a870df38..8f32e4c5830f5746e2dac910888093e4943936bf 100644
>--- a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>+++ b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>@@ -565,6 +565,7 @@
> 		83BAEE8D1EF4625500DDE894 /* PluginLoadClientPolicies.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83BAEE8C1EF4625500DDE894 /* PluginLoadClientPolicies.mm */; };
> 		83DB79691EF63B3C00BFA5E5 /* Function.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83DB79671EF63B3C00BFA5E5 /* Function.cpp */; };
> 		83DE134D1EF1C50800C1B355 /* ResponsivenessTimer.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83DE134C1EF1C4FE00C1B355 /* ResponsivenessTimer.cpp */; };
>+		83F22C6420B355F80034277E /* NoPolicyDelegateResponse.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83F22C6320B355EB0034277E /* NoPolicyDelegateResponse.mm */; };
> 		8C10AF98206467920018FD90 /* localstorage-empty-string-value.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 8C10AF97206467830018FD90 /* localstorage-empty-string-value.html */; };
> 		8C10AF99206467A90018FD90 /* LocalStoragePersistence.mm in Sources */ = {isa = PBXBuildFile; fileRef = 8C10AF96206467770018FD90 /* LocalStoragePersistence.mm */; };
> 		8E4A85371E1D1AB200F53B0F /* GridPosition.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8E4A85361E1D1AA100F53B0F /* GridPosition.cpp */; };
>@@ -1604,6 +1605,7 @@
> 		83BAEE8C1EF4625500DDE894 /* PluginLoadClientPolicies.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = PluginLoadClientPolicies.mm; sourceTree = "<group>"; };
> 		83DB79671EF63B3C00BFA5E5 /* Function.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Function.cpp; sourceTree = "<group>"; };
> 		83DE134C1EF1C4FE00C1B355 /* ResponsivenessTimer.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ResponsivenessTimer.cpp; sourceTree = "<group>"; };
>+		83F22C6320B355EB0034277E /* NoPolicyDelegateResponse.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = NoPolicyDelegateResponse.mm; sourceTree = "<group>"; };
> 		86BD19971A2DB05B006DCF0A /* RefCounter.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = RefCounter.cpp; sourceTree = "<group>"; };
> 		8A2C750D16CED9550024F352 /* ResizeWindowAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ResizeWindowAfterCrash.cpp; sourceTree = "<group>"; };
> 		8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ReloadPageAfterCrash.cpp; sourceTree = "<group>"; };
>@@ -3041,6 +3043,7 @@
> 				5C0BF88C1DD5957400B00328 /* MemoryPressureHandler.mm */,
> 				7A99D9931AD4A29D00373141 /* MenuTypesForMouseEvents.mm */,
> 				E19DB9781B32137C00DB38D4 /* NavigatorLanguage.mm */,
>+				83F22C6320B355EB0034277E /* NoPolicyDelegateResponse.mm */,
> 				A57A34EF16AF677200C2501F /* PageVisibilityStateWithWindowChanges.mm */,
> 				00BC16851680FE810065F1E5 /* PublicSuffix.mm */,
> 				37C784DE197C8F2E0010A496 /* RenderedImageFromDOMNode.mm */,
>@@ -3830,6 +3833,7 @@
> 				37B47E301D64E7CA005F4EFF /* WKObject.mm in Sources */,
> 				7C89D2AC1A69B80D003A5FDE /* WKPageConfiguration.cpp in Sources */,
> 				52D673EE1AFB127300FA19FE /* WKPageCopySessionStateWithFiltering.cpp in Sources */,
>+				83F22C6420B355F80034277E /* NoPolicyDelegateResponse.mm in Sources */,
> 				7CCE7F1F1A411AE600447C4C /* WKPageGetScaleFactorNotZero.cpp in Sources */,
> 				7CCE7F201A411AE600447C4C /* WKPageIsPlayingAudio.cpp in Sources */,
> 				A14AAB631E78D7DE00C1ADC2 /* WKPDFView.mm in Sources */,
>diff --git a/Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm b/Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm
>new file mode 100644
>index 0000000000000000000000000000000000000000..cbc3ac6339b81fc88a3144b51a2b91135d991190
>--- /dev/null
>+++ b/Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm
>@@ -0,0 +1,77 @@
>+/*
>+ * Copyright (C) 2018 Apple Inc. All rights reserved.
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *    notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *    notice, this list of conditions and the following disclaimer in the
>+ *    documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
>+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
>+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
>+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
>+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
>+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
>+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
>+ * THE POSSIBILITY OF SUCH DAMAGE.
>+ */
>+
>+#import "config.h"
>+#import "PlatformUtilities.h"
>+#import <WebKit/WebViewPrivate.h>
>+#import <wtf/RetainPtr.h>
>+
>+@interface NoPolicyDelegateDecisionDelegate : NSObject <WebPolicyDelegate, WebFrameLoadDelegate> {
>+}
>+@end
>+
>+static bool didFinishLoad;
>+static bool didNavigationActionCheck;
>+static bool didNavigationResponseCheck;
>+
>+@implementation NoPolicyDelegateDecisionDelegate
>+
>+- (void)webView:(WebView *)webView decidePolicyForNavigationAction:(NSDictionary *)actionInformation request:(NSURLRequest *)request frame:(WebFrame *)frame decisionListener:(id<WebPolicyDecisionListener>)listener
>+{
>+    // Implements decidePolicyForNavigationAction but fails to call the decision listener.
>+    didNavigationActionCheck = YES;
>+}
>+
>+- (void)webView:(WebView *)webView decidePolicyForMIMEType:(NSString *)type request:(NSURLRequest *)request frame:(WebFrame *)frame decisionListener:(id<WebPolicyDecisionListener>)listener
>+{
>+    // Implements decidePolicyForMIMEType but fails to call the decision listener.
>+    didNavigationResponseCheck = YES;
>+}
>+
>+- (void)webView:(WebView *)sender didFinishLoadForFrame:(WebFrame *)frame
>+{
>+    didFinishLoad = true;
>+}
>+
>+@end
>+
>+namespace TestWebKitAPI {
>+
>+TEST(WebKitLegacy, NoPolicyDelegateDecision)
>+{
>+    auto webView = adoptNS([[WebView alloc] initWithFrame:NSZeroRect frameName:nil groupName:nil]);
>+    auto delegate = adoptNS([NoPolicyDelegateDecisionDelegate new]);
>+
>+    webView.get().frameLoadDelegate = delegate.get();
>+    webView.get().policyDelegate = delegate.get();
>+    [[webView.get() mainFrame] loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"verboseMarkup" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
>+
>+    Util::run(&didFinishLoad);
>+
>+    EXPECT_TRUE(didNavigationActionCheck);
>+    EXPECT_TRUE(didNavigationResponseCheck);
>+}
>+
>+} // namespace TestWebKitAPI

Actions: View | Formatted Diff | Diff

Attachments on bug 185832: 340874 | 340876 | 340885 | 340887 | 340888 | 340901 | 340909 | 340916 | 340920 | 340921