Regression(r232082): Websites get loaded inside of Messages App chat transcript
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Jun 2018 04:53:35 +0000 (04:53 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Jun 2018 04:53:35 +0000 (04:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186331
<rdar://problem/40735446>

Reviewed by Darin Adler.

Source/WebKitLegacy/mac:

r232082 made it so that if the client implements decidePolicyForMIMEType / decidePolicyForNavigationAction
but does not call use / ignore on the listener, then we would do "use" by default.
The intention was to restore pre-AsyncPolicyDelegates behavior and unbreak Box.app. However,
the pre-AsyncPolicyDelegates behavior was only to "use" by default for decidePolicyForMIMEType,
not decidePolicyForNavigationAction. Doing "use" by default for decidePolicyForNavigationAction
is new behavior and it breaks Messages.app. This patch updates r232082 so that we now do call
"use" by default on the listener for decidePolicyForMIMEType and "ignore" by default for other
policy decisions. This should restore pre-AsyncPolicyDelegates behavior. This fixes Messages.app
and Box.app is still working properly.

* WebCoreSupport/WebFrameLoaderClient.h:
* WebCoreSupport/WebFrameLoaderClient.mm:
(WebFrameLoaderClient::dispatchDecidePolicyForResponse):
(WebFrameLoaderClient::dispatchDecidePolicyForNewWindowAction):
(WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
(WebFrameLoaderClient::dispatchWillSubmitForm):
(WebFrameLoaderClient::setUpPolicyListener):
(-[WebFramePolicyListener initWithFrame:policyFunction:defaultPolicy:]):
(-[WebFramePolicyListener initWithFrame:policyFunction:defaultPolicy:appLinkURL:]):
(-[WebFramePolicyListener dealloc]):
(-[WebFramePolicyListener initWithFrame:policyFunction:]): Deleted.
(-[WebFramePolicyListener initWithFrame:policyFunction:appLinkURL:]): Deleted.

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm:
(-[NoDecidePolicyForNavigationActionDecisionDelegate webView:decidePolicyForNavigationAction:request:frame:decisionListener:]):
(-[NoDecidePolicyForNavigationActionDecisionDelegate webView:didStartProvisionalLoadForFrame:]):
(TestWebKitAPI::TEST):
(-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForNavigationAction:request:frame:decisionListener:]): Deleted.
(-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForMIMEType:request:frame:decisionListener:]): Deleted.
(-[NoPolicyDelegateDecisionDelegate webView:didFinishLoadForFrame:]): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232536 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm

index 144f43c..f8e309a 100644 (file)
@@ -1,3 +1,34 @@
+2018-06-05  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r232082): Websites get loaded inside of Messages App chat transcript
+        https://bugs.webkit.org/show_bug.cgi?id=186331
+        <rdar://problem/40735446>
+
+        Reviewed by Darin Adler.
+
+        r232082 made it so that if the client implements decidePolicyForMIMEType / decidePolicyForNavigationAction
+        but does not call use / ignore on the listener, then we would do "use" by default.
+        The intention was to restore pre-AsyncPolicyDelegates behavior and unbreak Box.app. However,
+        the pre-AsyncPolicyDelegates behavior was only to "use" by default for decidePolicyForMIMEType,
+        not decidePolicyForNavigationAction. Doing "use" by default for decidePolicyForNavigationAction
+        is new behavior and it breaks Messages.app. This patch updates r232082 so that we now do call
+        "use" by default on the listener for decidePolicyForMIMEType and "ignore" by default for other
+        policy decisions. This should restore pre-AsyncPolicyDelegates behavior. This fixes Messages.app
+        and Box.app is still working properly.
+
+        * WebCoreSupport/WebFrameLoaderClient.h:
+        * WebCoreSupport/WebFrameLoaderClient.mm:
+        (WebFrameLoaderClient::dispatchDecidePolicyForResponse):
+        (WebFrameLoaderClient::dispatchDecidePolicyForNewWindowAction):
+        (WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+        (WebFrameLoaderClient::dispatchWillSubmitForm):
+        (WebFrameLoaderClient::setUpPolicyListener):
+        (-[WebFramePolicyListener initWithFrame:policyFunction:defaultPolicy:]):
+        (-[WebFramePolicyListener initWithFrame:policyFunction:defaultPolicy:appLinkURL:]):
+        (-[WebFramePolicyListener dealloc]):
+        (-[WebFramePolicyListener initWithFrame:policyFunction:]): Deleted.
+        (-[WebFramePolicyListener initWithFrame:policyFunction:appLinkURL:]): Deleted.
+
 2018-06-05  Brent Fulgham  <bfulgham@apple.com>
 
         Adjust compile and runtime flags to match shippable state of features
index 4c9d41f..0d6aaf8 100644 (file)
@@ -234,7 +234,7 @@ private:
 
     RemoteAXObjectRef accessibilityRemoteObject() final { return 0; }
     
-    RetainPtr<WebFramePolicyListener> setUpPolicyListener(WebCore::FramePolicyFunction&&, NSURL *appLinkURL = nil);
+    RetainPtr<WebFramePolicyListener> setUpPolicyListener(WebCore::FramePolicyFunction&&, WebCore::PolicyAction defaultPolicy, NSURL *appLinkURL = nil);
 
     NSDictionary *actionDictionary(const WebCore::NavigationAction&, WebCore::FormState*) const;
     
index bdb8839..e59c189 100644 (file)
@@ -180,11 +180,12 @@ NSString *WebPluginContainerKey = @"WebPluginContainer";
 #if HAVE(APP_LINKS)
     RetainPtr<NSURL> _appLinkURL;
 #endif
+    PolicyAction _defaultPolicy;
 }
 
-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction;
+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy;
 #if HAVE(APP_LINKS)
-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction appLinkURL:(NSURL *)url;
+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy appLinkURL:(NSURL *)url;
 #endif
 
 - (void)invalidate;
@@ -865,7 +866,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForResponse(const ResourceRespons
                         decidePolicyForMIMEType:response.mimeType()
                                         request:request.nsURLRequest(UpdateHTTPBody)
                                           frame:m_webFrame.get()
-                               decisionListener:setUpPolicyListener(WTFMove(function)).get()];
+                               decisionListener:setUpPolicyListener(WTFMove(function), PolicyAction::Use).get()];
 }
 
 
@@ -897,7 +898,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNewWindowAction(const Navigati
             decidePolicyForNewWindowAction:actionDictionary(action, formState)
                                    request:request.nsURLRequest(UpdateHTTPBody)
                               newFrameName:frameName
-                          decisionListener:setUpPolicyListener(WTFMove(function), tryAppLink ? (NSURL *)request.url() : nil).get()];
+                          decisionListener:setUpPolicyListener(WTFMove(function), PolicyAction::Ignore, tryAppLink ? (NSURL *)request.url() : nil).get()];
 }
 
 void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& action, const ResourceRequest& request, bool, FormState* formState, PolicyDecisionMode, FramePolicyFunction&& function)
@@ -909,7 +910,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
                 decidePolicyForNavigationAction:actionDictionary(action, formState)
                                         request:request.nsURLRequest(UpdateHTTPBody)
                                           frame:m_webFrame.get()
-                               decisionListener:setUpPolicyListener(WTFMove(function), tryAppLink ? (NSURL *)request.url() : nil).get()];
+                               decisionListener:setUpPolicyListener(WTFMove(function), PolicyAction::Ignore, tryAppLink ? (NSURL *)request.url() : nil).get()];
 }
 
 void WebFrameLoaderClient::cancelPolicyCheck()
@@ -957,7 +958,7 @@ void WebFrameLoaderClient::dispatchWillSubmitForm(FormState& formState, WTF::Fun
     }
 
     NSDictionary *values = makeFormFieldValuesDictionary(formState);
-    CallFormDelegate(getWebView(m_webFrame.get()), @selector(frame:sourceFrame:willSubmitForm:withValues:submissionListener:), m_webFrame.get(), kit(formState.sourceDocument().frame()), kit(&formState.form()), values, setUpPolicyListener([function = WTFMove(function)](PolicyAction) { function(); }).get());
+    CallFormDelegate(getWebView(m_webFrame.get()), @selector(frame:sourceFrame:willSubmitForm:withValues:submissionListener:), m_webFrame.get(), kit(formState.sourceDocument().frame()), kit(&formState.form()), values, setUpPolicyListener([function = WTFMove(function)](PolicyAction) { function(); }, PolicyAction::Ignore).get());
 }
 
 void WebFrameLoaderClient::revertToProvisionalState(DocumentLoader* loader)
@@ -1522,7 +1523,7 @@ void WebFrameLoaderClient::dispatchDidBecomeFrameset(bool)
 {
 }
 
-RetainPtr<WebFramePolicyListener> WebFrameLoaderClient::setUpPolicyListener(FramePolicyFunction&& function, NSURL *appLinkURL)
+RetainPtr<WebFramePolicyListener> WebFrameLoaderClient::setUpPolicyListener(FramePolicyFunction&& function, PolicyAction defaultPolicy, NSURL *appLinkURL)
 {
     // FIXME: <rdar://5634381> We need to support multiple active policy listeners.
     [m_policyListener invalidate];
@@ -1530,10 +1531,10 @@ RetainPtr<WebFramePolicyListener> WebFrameLoaderClient::setUpPolicyListener(Fram
     RetainPtr<WebFramePolicyListener> policyListener;
 #if HAVE(APP_LINKS)
     if (appLinkURL)
-        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) defaultPolicy:defaultPolicy appLinkURL:appLinkURL]);
     else
 #endif
-        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function)]);
+        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function) defaultPolicy:defaultPolicy]);
 
     m_policyListener = policyListener.get();
 
@@ -2386,7 +2387,7 @@ void WebFrameLoaderClient::finishedLoadingIcon(uint64_t callbackID, SharedBuffer
 #endif
 }
 
-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction
+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy
 {
     self = [self init];
     if (!self)
@@ -2394,14 +2395,15 @@ void WebFrameLoaderClient::finishedLoadingIcon(uint64_t callbackID, SharedBuffer
 
     _frame = frame;
     _policyFunction = WTFMove(policyFunction);
+    _defaultPolicy = defaultPolicy;
 
     return self;
 }
 
 #if HAVE(APP_LINKS)
-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction appLinkURL:(NSURL *)appLinkURL
+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy appLinkURL:(NSURL *)appLinkURL
 {
-    self = [self initWithFrame:frame policyFunction:WTFMove(policyFunction)];
+    self = [self initWithFrame:frame policyFunction:WTFMove(policyFunction) defaultPolicy:defaultPolicy];
     if (!self)
         return nil;
 
@@ -2423,12 +2425,12 @@ void WebFrameLoaderClient::finishedLoadingIcon(uint64_t callbackID, SharedBuffer
     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".
+    // If the app did not respond before the listener is destroyed, then we use the default policy ("Use" for navigation
+    // response policy decision, "Ignore" for other policy decisions).
     _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);
+        RELEASE_LOG_ERROR(Loading, "Client application failed to make a policy decision via WebPolicyDecisionListener, using defaultPolicy %u", _defaultPolicy);
+        policyFunction(_defaultPolicy);
     }
 
     [super dealloc];
index 12c5454..89b532a 100644 (file)
@@ -1,3 +1,21 @@
+2018-06-05  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r232082): Websites get loaded inside of Messages App chat transcript
+        https://bugs.webkit.org/show_bug.cgi?id=186331
+        <rdar://problem/40735446>
+
+        Reviewed by Darin Adler.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm:
+        (-[NoDecidePolicyForNavigationActionDecisionDelegate webView:decidePolicyForNavigationAction:request:frame:decisionListener:]):
+        (-[NoDecidePolicyForNavigationActionDecisionDelegate webView:didStartProvisionalLoadForFrame:]):
+        (TestWebKitAPI::TEST):
+        (-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForNavigationAction:request:frame:decisionListener:]): Deleted.
+        (-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForMIMEType:request:frame:decisionListener:]): Deleted.
+        (-[NoPolicyDelegateDecisionDelegate webView:didFinishLoadForFrame:]): Deleted.
+
 2018-06-05  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         DataInteractionTests ContentEditableToTextarea and ContentEditableToContentEditable are failing on recent iOS 12
index cbc3ac6..a61aea8 100644 (file)
 #import <WebKit/WebViewPrivate.h>
 #import <wtf/RetainPtr.h>
 
-@interface NoPolicyDelegateDecisionDelegate : NSObject <WebPolicyDelegate, WebFrameLoadDelegate> {
-}
-@end
-
 static bool didFinishLoad;
 static bool didNavigationActionCheck;
 static bool didNavigationResponseCheck;
+static bool didStartProvisionalLoad;
 
-@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;
+@interface NoDecidePolicyForMIMETypeDecisionDelegate : NSObject <WebPolicyDelegate, WebFrameLoadDelegate> {
 }
+@end
+
+@implementation NoDecidePolicyForMIMETypeDecisionDelegate
 
 - (void)webView:(WebView *)webView decidePolicyForMIMEType:(NSString *)type request:(NSURLRequest *)request frame:(WebFrame *)frame decisionListener:(id<WebPolicyDecisionListener>)listener
 {
@@ -57,12 +52,31 @@ static bool didNavigationResponseCheck;
 
 @end
 
+@interface NoDecidePolicyForNavigationActionDecisionDelegate : NSObject <WebPolicyDelegate, WebFrameLoadDelegate> {
+}
+@end
+
+@implementation NoDecidePolicyForNavigationActionDecisionDelegate
+
+- (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 *)sender didStartProvisionalLoadForFrame:(WebFrame *)frame
+{
+    didStartProvisionalLoad = true;
+}
+
+@end
+
 namespace TestWebKitAPI {
 
-TEST(WebKitLegacy, NoPolicyDelegateDecision)
+TEST(WebKitLegacy, NoDecidePolicyForMIMETypeDecision)
 {
     auto webView = adoptNS([[WebView alloc] initWithFrame:NSZeroRect frameName:nil groupName:nil]);
-    auto delegate = adoptNS([NoPolicyDelegateDecisionDelegate new]);
+    auto delegate = adoptNS([NoDecidePolicyForMIMETypeDecisionDelegate new]);
 
     webView.get().frameLoadDelegate = delegate.get();
     webView.get().policyDelegate = delegate.get();
@@ -70,8 +84,21 @@ TEST(WebKitLegacy, NoPolicyDelegateDecision)
 
     Util::run(&didFinishLoad);
 
-    EXPECT_TRUE(didNavigationActionCheck);
     EXPECT_TRUE(didNavigationResponseCheck);
 }
 
+TEST(WebKitLegacy, NoDecidePolicyForNavigationActionDecision)
+{
+    auto webView = adoptNS([[WebView alloc] initWithFrame:NSZeroRect frameName:nil groupName:nil]);
+    auto delegate = adoptNS([NoDecidePolicyForNavigationActionDecisionDelegate 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(&didNavigationActionCheck);
+
+    EXPECT_FALSE(didStartProvisionalLoad);
+}
+
 } // namespace TestWebKitAPI