Defer ending AppSSO sheets til NSWindowDidDeminiaturize or NSApplicationDidUnhide
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Feb 2020 22:49:43 +0000 (22:49 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Feb 2020 22:49:43 +0000 (22:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207173
<rdar://problem/55669065>

Reviewed by Brent Fulgham.

Source/WebKit:

AppKit has a bug that [m_sheetWindow sheetParent] is null if the parent is minimized or the host app is hidden.
SheetParent is used to endSheet. Therefore, WebKit cannot dismiss AppSSO UIs in the above two cases if asked
by extensions.

This patch implements a workaround to detect those two cases and defer the endSheet til NSWindowDidDeminiaturize
or NSApplicationDidUnhide is received.

Covered by API tests.

* UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h:
* UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:
(WebKit::SOAuthorizationSession::dismissViewController):
* UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:
(WebKit::SubFrameSOAuthorizationSession::abortInternal):
Treat abort like falling back to the web path for sub frame case given displaying an empty
iframe after aborting seems bad.

Tools:

* TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:
(TestWebKitAPI::TEST):
* TestWebKitAPI/cocoa/TestWKWebView.mm:
(-[TestWKWebView _setUpTestWindow:]):
Make the TestWKWebViewHostWindow miniaturizable to enable the test.

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h
Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm
Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm
Tools/TestWebKitAPI/cocoa/TestWKWebView.mm

index f31bc11..5879320 100644 (file)
@@ -1,3 +1,28 @@
+2020-02-05  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Defer ending AppSSO sheets til NSWindowDidDeminiaturize or NSApplicationDidUnhide
+        https://bugs.webkit.org/show_bug.cgi?id=207173
+        <rdar://problem/55669065>
+
+        Reviewed by Brent Fulgham.
+
+        AppKit has a bug that [m_sheetWindow sheetParent] is null if the parent is minimized or the host app is hidden.
+        SheetParent is used to endSheet. Therefore, WebKit cannot dismiss AppSSO UIs in the above two cases if asked
+        by extensions.
+
+        This patch implements a workaround to detect those two cases and defer the endSheet til NSWindowDidDeminiaturize
+        or NSApplicationDidUnhide is received.
+
+        Covered by API tests.
+
+        * UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h:
+        * UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:
+        (WebKit::SOAuthorizationSession::dismissViewController):
+        * UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:
+        (WebKit::SubFrameSOAuthorizationSession::abortInternal):
+        Treat abort like falling back to the web path for sub frame case given displaying an empty
+        iframe after aborting seems bad.
+
 2020-02-05  Chris Dumez  <cdumez@apple.com>
 
         [IPC Hardening] Protect against targetId String being invalid in WebPageProxy::createInspectorTarget() / destroyInspectorTarget()
index 23e2707..c6ba7e2 100644 (file)
@@ -117,6 +117,8 @@ private:
 #if PLATFORM(MAC)
     RetainPtr<NSWindow> m_sheetWindow;
     RetainPtr<NSObject> m_sheetWindowWillCloseObserver;
+    RetainPtr<NSObject> m_presentingWindowDidDeminiaturizeObserver;
+    RetainPtr<NSObject> m_applicationDidUnhideObserver;
 #endif
 };
 
index c3734be..273b591 100644 (file)
@@ -296,6 +296,32 @@ void SOAuthorizationSession::dismissViewController()
 #if PLATFORM(MAC)
     ASSERT(m_sheetWindow && m_sheetWindowWillCloseObserver);
 
+    // This is a workaround for an AppKit issue: <rdar://problem/59125329>.
+    // [m_sheetWindow sheetParent] is null if the parent is minimized or the host app is hidden.
+    if (auto *presentingWindow = m_page->platformWindow()) {
+        if (presentingWindow.miniaturized) {
+            if (m_presentingWindowDidDeminiaturizeObserver)
+                return;
+            m_presentingWindowDidDeminiaturizeObserver = [[NSNotificationCenter defaultCenter] addObserverForName:NSWindowDidDeminiaturizeNotification object:presentingWindow queue:nil usingBlock:[protectedThis = makeRefPtr(this), this] (NSNotification *) {
+                dismissViewController();
+                [[NSNotificationCenter defaultCenter] removeObserver:m_presentingWindowDidDeminiaturizeObserver.get()];
+                m_presentingWindowDidDeminiaturizeObserver = nullptr;
+            }];
+            return;
+        }
+    }
+
+    if (NSApp.hidden) {
+        if (m_applicationDidUnhideObserver)
+            return;
+        m_applicationDidUnhideObserver = [[NSNotificationCenter defaultCenter] addObserverForName:NSApplicationDidUnhideNotification object:NSApp queue:nil usingBlock:[protectedThis = makeRefPtr(this), this] (NSNotification *) {
+            dismissViewController();
+            [[NSNotificationCenter defaultCenter] removeObserver:m_applicationDidUnhideObserver.get()];
+            m_applicationDidUnhideObserver = nullptr;
+        }];
+        return;
+    }
+
     [[NSNotificationCenter defaultCenter] removeObserver:m_sheetWindowWillCloseObserver.get()];
     m_sheetWindowWillCloseObserver = nullptr;
 
index 3b5380e..265898c 100644 (file)
@@ -83,6 +83,7 @@ void SubFrameSOAuthorizationSession::fallBackToWebPathInternal()
 
 void SubFrameSOAuthorizationSession::abortInternal()
 {
+    fallBackToWebPathInternal();
 }
 
 void SubFrameSOAuthorizationSession::completeInternal(const WebCore::ResourceResponse& response, NSData *data)
index c136027..c5bbd54 100644 (file)
@@ -1,3 +1,17 @@
+2020-02-05  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Defer ending AppSSO sheets til NSWindowDidDeminiaturize or NSApplicationDidUnhide
+        https://bugs.webkit.org/show_bug.cgi?id=207173
+        <rdar://problem/55669065>
+
+        Reviewed by Brent Fulgham.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:
+        (TestWebKitAPI::TEST):
+        * TestWebKitAPI/cocoa/TestWKWebView.mm:
+        (-[TestWKWebView _setUpTestWindow:]):
+        Make the TestWKWebViewHostWindow miniaturizable to enable the test.
+
 2020-02-05  Jonathan Bedard  <jbedard@apple.com>
 
         results.webkit.org: Apply ttl to archive shards
index 9730ac5..f9b3063 100644 (file)
@@ -1427,6 +1427,184 @@ TEST(SOAuthorizationRedirect, NSNotificationCenter)
     gNotificationCallback(nullptr);
     EXPECT_FALSE(uiShowed);
 }
+
+TEST(SOAuthorizationRedirect, DismissUIDuringMiniaturization)
+{
+    resetState();
+    ClassMethodSwizzler swizzler1(PAL::getSOAuthorizationClass(), @selector(canPerformAuthorizationWithURL:responseCode:), reinterpret_cast<IMP>(overrideCanPerformAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler2(PAL::getSOAuthorizationClass(), @selector(setDelegate:), reinterpret_cast<IMP>(overrideSetDelegate));
+    InstanceMethodSwizzler swizzler3(PAL::getSOAuthorizationClass(), @selector(beginAuthorizationWithURL:httpHeaders:httpBody:), reinterpret_cast<IMP>(overrideBeginAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler4(PAL::getSOAuthorizationClass(), @selector(getAuthorizationHintsWithURL:responseCode:completion:), reinterpret_cast<IMP>(overrideGetAuthorizationHintsWithURL));
+
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
+    auto delegate = adoptNS([[TestSOAuthorizationDelegate alloc] init]);
+    configureSOAuthorizationWebView(webView.get(), delegate.get(), OpenExternalSchemesPolicy::Allow);
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&authorizationPerformed);
+
+    auto viewController = adoptNS([[TestSOAuthorizationViewController alloc] init]);
+    auto view = adoptNS([[NSView alloc] initWithFrame:NSZeroRect]);
+    [viewController setView:view.get()];
+
+    [gDelegate authorization:gAuthorization presentViewController:viewController.get() withCompletion:^(BOOL success, NSError *) {
+        EXPECT_TRUE(success);
+    }];
+    Util::run(&uiShowed);
+
+    bool didEndSheet = false;
+    auto *hostWindow = [webView hostWindow];
+    auto observer = adoptNS([[NSNotificationCenter defaultCenter] addObserverForName:NSWindowDidEndSheetNotification object:hostWindow queue:nil usingBlock:[&didEndSheet] (NSNotification *) {
+        didEndSheet = true;
+    }]);
+    [hostWindow miniaturize:hostWindow];
+
+    // Dimiss the UI
+    [gDelegate authorizationDidCancel:gAuthorization];
+    EXPECT_FALSE(didEndSheet);
+
+    // UI is only dimissed after the hostWindow is deminimized.
+    [hostWindow deminiaturize:hostWindow];
+    EXPECT_TRUE(didEndSheet);
+}
+
+TEST(SOAuthorizationRedirect, DismissUIDuringHiding)
+{
+    resetState();
+    ClassMethodSwizzler swizzler1(PAL::getSOAuthorizationClass(), @selector(canPerformAuthorizationWithURL:responseCode:), reinterpret_cast<IMP>(overrideCanPerformAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler2(PAL::getSOAuthorizationClass(), @selector(setDelegate:), reinterpret_cast<IMP>(overrideSetDelegate));
+    InstanceMethodSwizzler swizzler3(PAL::getSOAuthorizationClass(), @selector(beginAuthorizationWithURL:httpHeaders:httpBody:), reinterpret_cast<IMP>(overrideBeginAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler4(PAL::getSOAuthorizationClass(), @selector(getAuthorizationHintsWithURL:responseCode:completion:), reinterpret_cast<IMP>(overrideGetAuthorizationHintsWithURL));
+
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
+    auto delegate = adoptNS([[TestSOAuthorizationDelegate alloc] init]);
+    configureSOAuthorizationWebView(webView.get(), delegate.get(), OpenExternalSchemesPolicy::Allow);
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&authorizationPerformed);
+
+    auto viewController = adoptNS([[TestSOAuthorizationViewController alloc] init]);
+    auto view = adoptNS([[NSView alloc] initWithFrame:NSZeroRect]);
+    [viewController setView:view.get()];
+
+    [gDelegate authorization:gAuthorization presentViewController:viewController.get() withCompletion:^(BOOL success, NSError *) {
+        EXPECT_TRUE(success);
+    }];
+    Util::run(&uiShowed);
+
+    bool didEndSheet = false;
+    auto observer = adoptNS([[NSNotificationCenter defaultCenter] addObserverForName:NSWindowDidEndSheetNotification object:[webView hostWindow] queue:nil usingBlock:[&didEndSheet] (NSNotification *) {
+        didEndSheet = true;
+    }]);
+
+    [NSApp hide:NSApp];
+
+    // Dimiss the UI
+    [gDelegate authorizationDidCancel:gAuthorization];
+    EXPECT_FALSE(didEndSheet);
+
+    // UI is only dimissed after the hostApp is unhidden.
+    [NSApp unhide:NSApp];
+    EXPECT_TRUE(didEndSheet);
+}
+
+TEST(SOAuthorizationRedirect, DismissUIDuringMiniaturizationThenAnother)
+{
+    resetState();
+    ClassMethodSwizzler swizzler1(PAL::getSOAuthorizationClass(), @selector(canPerformAuthorizationWithURL:responseCode:), reinterpret_cast<IMP>(overrideCanPerformAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler2(PAL::getSOAuthorizationClass(), @selector(setDelegate:), reinterpret_cast<IMP>(overrideSetDelegate));
+    InstanceMethodSwizzler swizzler3(PAL::getSOAuthorizationClass(), @selector(beginAuthorizationWithURL:httpHeaders:httpBody:), reinterpret_cast<IMP>(overrideBeginAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler4(PAL::getSOAuthorizationClass(), @selector(getAuthorizationHintsWithURL:responseCode:completion:), reinterpret_cast<IMP>(overrideGetAuthorizationHintsWithURL));
+
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
+    auto delegate = adoptNS([[TestSOAuthorizationDelegate alloc] init]);
+    configureSOAuthorizationWebView(webView.get(), delegate.get(), OpenExternalSchemesPolicy::Allow);
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&authorizationPerformed);
+
+    auto viewController1 = adoptNS([[TestSOAuthorizationViewController alloc] init]);
+    auto view1 = adoptNS([[NSView alloc] initWithFrame:NSZeroRect]);
+    [viewController1 setView:view1.get()];
+
+    [gDelegate authorization:gAuthorization presentViewController:viewController1.get() withCompletion:^(BOOL success, NSError *) {
+        EXPECT_TRUE(success);
+    }];
+    Util::run(&uiShowed);
+
+    bool didEndSheet = false;
+    auto *hostWindow = [webView hostWindow];
+    auto observer = adoptNS([[NSNotificationCenter defaultCenter] addObserverForName:NSWindowDidEndSheetNotification object:hostWindow queue:nil usingBlock:[&didEndSheet] (NSNotification *) {
+        didEndSheet = true;
+    }]);
+    [hostWindow miniaturize:hostWindow];
+
+    // Dimiss the UI
+    [gDelegate authorizationDidCancel:gAuthorization];
+    EXPECT_FALSE(didEndSheet);
+
+    // Load another AppSSO request.
+    authorizationPerformed = false;
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&authorizationPerformed);
+
+    // UI is only dimissed after the hostApp is unhidden.
+    [hostWindow deminiaturize:hostWindow];
+    EXPECT_TRUE(didEndSheet);
+}
+
+TEST(SOAuthorizationRedirect, DismissUIDuringHidingThenAnother)
+{
+    resetState();
+    ClassMethodSwizzler swizzler1(PAL::getSOAuthorizationClass(), @selector(canPerformAuthorizationWithURL:responseCode:), reinterpret_cast<IMP>(overrideCanPerformAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler2(PAL::getSOAuthorizationClass(), @selector(setDelegate:), reinterpret_cast<IMP>(overrideSetDelegate));
+    InstanceMethodSwizzler swizzler3(PAL::getSOAuthorizationClass(), @selector(beginAuthorizationWithURL:httpHeaders:httpBody:), reinterpret_cast<IMP>(overrideBeginAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler4(PAL::getSOAuthorizationClass(), @selector(getAuthorizationHintsWithURL:responseCode:completion:), reinterpret_cast<IMP>(overrideGetAuthorizationHintsWithURL));
+
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
+    auto delegate = adoptNS([[TestSOAuthorizationDelegate alloc] init]);
+    configureSOAuthorizationWebView(webView.get(), delegate.get(), OpenExternalSchemesPolicy::Allow);
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&authorizationPerformed);
+
+    auto viewController = adoptNS([[TestSOAuthorizationViewController alloc] init]);
+    auto view = adoptNS([[NSView alloc] initWithFrame:NSZeroRect]);
+    [viewController setView:view.get()];
+
+    [gDelegate authorization:gAuthorization presentViewController:viewController.get() withCompletion:^(BOOL success, NSError *) {
+        EXPECT_TRUE(success);
+    }];
+    Util::run(&uiShowed);
+
+    bool didEndSheet = false;
+    auto observer = adoptNS([[NSNotificationCenter defaultCenter] addObserverForName:NSWindowDidEndSheetNotification object:[webView hostWindow] queue:nil usingBlock:[&didEndSheet] (NSNotification *) {
+        didEndSheet = true;
+    }]);
+
+    [NSApp hide:NSApp];
+
+    // Dimiss the UI
+    [gDelegate authorizationDidCancel:gAuthorization];
+    EXPECT_FALSE(didEndSheet);
+
+    // Load another AppSSO request.
+    authorizationPerformed = false;
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&authorizationPerformed);
+
+    // UI is only dimissed after the hostApp is unhidden.
+    [NSApp unhide:NSApp];
+    EXPECT_TRUE(didEndSheet);
+}
 #endif
 
 TEST(SOAuthorizationPopUp, NoInterceptions)
@@ -2092,6 +2270,38 @@ TEST(SOAuthorizationSubFrame, InterceptionError)
     EXPECT_WK_STREQ("", finalURL);
 }
 
+TEST(SOAuthorizationSubFrame, InterceptionCancel)
+{
+    resetState();
+    ClassMethodSwizzler swizzler1(PAL::getSOAuthorizationClass(), @selector(canPerformAuthorizationWithURL:responseCode:), reinterpret_cast<IMP>(overrideCanPerformAuthorizationWithURL));
+    InstanceMethodSwizzler swizzler2(PAL::getSOAuthorizationClass(), @selector(setDelegate:), reinterpret_cast<IMP>(overrideSetDelegate));
+    InstanceMethodSwizzler swizzler3(PAL::getSOAuthorizationClass(), @selector(beginAuthorizationWithURL:httpHeaders:httpBody:), reinterpret_cast<IMP>(overrideBeginAuthorizationWithURL));
+    ClassMethodSwizzler swizzler4([AKAuthorizationController class], @selector(isURLFromAppleOwnedDomain:), reinterpret_cast<IMP>(overrideIsURLFromAppleOwnedDomain));
+    InstanceMethodSwizzler swizzler5(PAL::getSOAuthorizationClass(), @selector(getAuthorizationHintsWithURL:responseCode:completion:), reinterpret_cast<IMP>(overrideGetAuthorizationHintsWithURL));
+
+    RetainPtr<NSURL> baseURL = [[NSBundle mainBundle] URLForResource:@"simple2" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"GetSessionCookie" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+    auto testHtml = generateHtml(parentTemplate, testURL.get().absoluteString);
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
+    auto delegate = adoptNS([[TestSOAuthorizationDelegate alloc] init]);
+    configureSOAuthorizationWebView(webView.get(), delegate.get());
+
+    [webView loadHTMLString:testHtml baseURL:baseURL.get()];
+    [webView waitForMessage:@"null"];
+    [webView waitForMessage:@"SOAuthorizationDidStart"];
+    checkAuthorizationOptions(false, "file://", 2);
+
+    [gDelegate authorizationDidCancel:gAuthorization];
+    [webView waitForMessage:@"null"];
+    [webView waitForMessage:@"SOAuthorizationDidCancel"];
+    [webView waitForMessage:@""];
+    // Trys to wait until the iframe load is finished.
+    Util::sleep(0.5);
+    // Make sure we don't load the request of the iframe to the main frame.
+    EXPECT_WK_STREQ("", finalURL);
+}
+
 TEST(SOAuthorizationSubFrame, InterceptionSuccess)
 {
     resetState();
index ef61adb..439b9cc 100644 (file)
@@ -394,7 +394,7 @@ static UICalloutBar *suppressUICalloutBar()
 - (void)_setUpTestWindow:(NSRect)frame
 {
 #if PLATFORM(MAC)
-    _hostWindow = adoptNS([[TestWKWebViewHostWindow alloc] initWithContentRect:frame styleMask:NSWindowStyleMaskBorderless backing:NSBackingStoreBuffered defer:NO]);
+    _hostWindow = adoptNS([[TestWKWebViewHostWindow alloc] initWithContentRect:frame styleMask:(NSWindowStyleMaskBorderless | NSWindowStyleMaskMiniaturizable) backing:NSBackingStoreBuffered defer:NO]);
     [_hostWindow setFrameOrigin:NSMakePoint(0, 0)];
     [_hostWindow setIsVisible:YES];
     [_hostWindow contentView].wantsLayer = YES;