Commit f4c3ac6e authored by David Benjamin's avatar David Benjamin Committed by Commit Bot

Defer creating sheets until a dialog is the first in the queue.

This fixes a regression from MacViews. See https://crbug.com/987744#c2.

Bug: 987744
Change-Id: I8b26d6738161cc8677d78b2cd8da3b4070831d73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003301Reviewed-by: default avatarElly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: default avatarMike Wittman <wittman@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735903}
parent de707f33
......@@ -5,9 +5,11 @@
#import <Cocoa/Cocoa.h>
#import <SecurityInterface/SFCertificatePanel.h>
#include "base/bind.h"
#include "base/mac/foundation_util.h"
#include "base/mac/scoped_cftyperef.h"
#import "base/mac/scoped_nsobject.h"
#include "base/memory/weak_ptr.h"
#include "chrome/browser/certificate_viewer.h"
#include "components/constrained_window/constrained_window_views.h"
#include "components/remote_cocoa/browser/window.h"
......@@ -156,22 +158,10 @@ class CertificateAnchorWidgetDelegate : public views::WidgetDelegateView {
: certificate_viewer_([[SSLCertificateViewerMac alloc]
initWithCertificate:cert
forWebContents:web_contents]) {
views::Widget* overlayWindow =
constrained_window::ShowWebModalDialogWithOverlayViews(this,
web_contents);
NSWindow* overlayNSWindow =
overlayWindow->GetNativeWindow().GetNativeNSWindow();
// TODO(https://crbug.com/913303): The certificate viewer's interface to
// Cocoa should be wrapped in a mojo interface in order to allow
// instantiating across processes. As a temporary solution, create a
// transparent in-process window to the front.
if (remote_cocoa::IsWindowRemote(overlayNSWindow)) {
remote_views_clone_window_ =
remote_cocoa::CreateInProcessTransparentClone(overlayNSWindow);
overlayNSWindow = remote_views_clone_window_;
}
[certificate_viewer_ showCertificateSheet:overlayNSWindow];
[certificate_viewer_ setOverlayWindow:overlayWindow];
constrained_window::ShowWebModalDialogWithOverlayViews(
this, web_contents,
base::BindOnce(&CertificateAnchorWidgetDelegate::ShowSheet,
weak_factory_.GetWeakPtr()));
}
~CertificateAnchorWidgetDelegate() override {
......@@ -186,8 +176,25 @@ class CertificateAnchorWidgetDelegate : public views::WidgetDelegateView {
ui::ModalType GetModalType() const override { return ui::MODAL_TYPE_CHILD; }
private:
void ShowSheet(views::Widget* overlay_window) {
NSWindow* overlay_ns_window =
overlay_window->GetNativeWindow().GetNativeNSWindow();
// TODO(https://crbug.com/913303): The certificate viewer's interface to
// Cocoa should be wrapped in a mojo interface in order to allow
// instantiating across processes. As a temporary solution, create a
// transparent in-process window to the front.
if (remote_cocoa::IsWindowRemote(overlay_ns_window)) {
remote_views_clone_window_ =
remote_cocoa::CreateInProcessTransparentClone(overlay_ns_window);
overlay_ns_window = remote_views_clone_window_;
}
[certificate_viewer_ showCertificateSheet:overlay_ns_window];
[certificate_viewer_ setOverlayWindow:overlay_window];
}
base::scoped_nsobject<SSLCertificateViewerMac> certificate_viewer_;
NSWindow* remote_views_clone_window_ = nil;
base::WeakPtrFactory<CertificateAnchorWidgetDelegate> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(CertificateAnchorWidgetDelegate);
};
......
......@@ -138,14 +138,13 @@ class SSLClientCertificateSelectorDelegate
: SSLClientAuthObserver(contents->GetBrowserContext(),
cert_request_info,
std::move(delegate)) {
certificate_selector_.reset([[SSLClientCertificateSelectorMac alloc]
initWithClientCerts:std::move(client_certs)
delegate:weak_factory_.GetWeakPtr()]);
views::Widget* overlay_window =
constrained_window::ShowWebModalDialogWithOverlayViews(this, contents);
[certificate_selector_ showSheetForWindow:overlay_window->GetNativeWindow()
.GetNativeNSWindow()];
StartObserving();
// Note this may call ShowSheet() synchronously or in a separate event loop
// iteration.
constrained_window::ShowWebModalDialogWithOverlayViews(
this, contents,
base::BindOnce(&SSLClientCertificateSelectorDelegate::ShowSheet,
weak_factory_.GetWeakPtr(), std::move(client_certs)));
}
~SSLClientCertificateSelectorDelegate() override {
......@@ -153,6 +152,10 @@ class SSLClientCertificateSelectorDelegate
// (|certificate_selector_|) in its -beginSheetForWindow:... method. Break
// the retain cycle by explicitly canceling the dialog.
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseAbort];
// This matches the StartObserving() call if ShowSheet() was never called
// and the request was canceled.
StopObserving();
}
// WidgetDelegate:
......@@ -160,10 +163,16 @@ class SSLClientCertificateSelectorDelegate
// OkAndCancelableForTesting:
void ClickOkButton() override {
// Tests should not call ClickOkButton() on a sheet that has not yet been
// shown.
DCHECK(certificate_selector_);
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseOK];
}
void ClickCancelButton() override {
// Tests should not call ClickCancelButton() on a sheet that has not yet
// been shown.
DCHECK(certificate_selector_);
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseCancel];
}
......@@ -173,13 +182,8 @@ class SSLClientCertificateSelectorDelegate
}
void SetDeallocClosureForTesting(base::OnceClosure dealloc_closure) {
DeallocClosureCaller* caller = [[DeallocClosureCaller alloc]
initWithDeallocClosure:std::move(dealloc_closure)];
// The use of the caller as the key is deliberate; nothing needs to ever
// look it up, so it's a convenient unique value.
objc_setAssociatedObject(certificate_selector_.get(), caller, caller,
OBJC_ASSOCIATION_RETAIN_NONATOMIC);
[caller release];
dealloc_closure_ = std::move(dealloc_closure);
SetDeallocClosureIfReady();
}
base::OnceClosure GetCancellationCallback() {
......@@ -193,11 +197,53 @@ class SSLClientCertificateSelectorDelegate
private:
void CloseSelector() {
cancelled_ = true;
if (certificate_selector_) {
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseStop];
} else {
CloseWidgetWithReason(views::Widget::ClosedReason::kUnspecified);
}
}
base::scoped_nsobject<SSLClientCertificateSelectorMac> certificate_selector_;
void ShowSheet(net::ClientCertIdentityList client_certs,
views::Widget* overlay_window) {
DCHECK(!certificate_selector_);
if (cancelled_) {
// If CloseSelector() is called before the sheet is shown, it should
// synchronously destroy the dialog, which means ShowSheet() cannot later
// be called, but check for this in case the dialog logic changes.
NOTREACHED();
return;
}
certificate_selector_.reset([[SSLClientCertificateSelectorMac alloc]
initWithClientCerts:std::move(client_certs)
delegate:weak_factory_.GetWeakPtr()]);
SetDeallocClosureIfReady();
[certificate_selector_ showSheetForWindow:overlay_window->GetNativeWindow()
.GetNativeNSWindow()];
}
// Attaches |dealloc_closure_| to |certificate_selector_| if both are created.
// |certificate_selector_| is not created until ShowSheet(), so this method
// allows SetDeallocClosureForTesting() to take effect when ShowSheet() is
// deferred.
void SetDeallocClosureIfReady() {
if (!certificate_selector_ || dealloc_closure_.is_null())
return;
base::scoped_nsobject<DeallocClosureCaller> caller(
[[DeallocClosureCaller alloc]
initWithDeallocClosure:std::move(dealloc_closure_)]);
// The use of the caller as the key is deliberate; nothing needs to ever
// look it up, so it's a convenient unique value.
objc_setAssociatedObject(certificate_selector_.get(), caller, caller,
OBJC_ASSOCIATION_RETAIN_NONATOMIC);
}
base::scoped_nsobject<SSLClientCertificateSelectorMac> certificate_selector_;
bool cancelled_ = false;
base::OnceClosure dealloc_closure_;
base::WeakPtrFactory<SSLClientCertificateSelectorDelegate> weak_factory_{
this};
......
......@@ -22,8 +22,10 @@
#include "content/public/browser/client_certificate_delegate.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/test_utils.h"
#include "net/base/host_port_pair.h"
#include "net/cert/x509_certificate.h"
#include "net/ssl/client_cert_identity_mac.h"
#include "net/ssl/ssl_cert_request_info.h"
#include "net/ssl/ssl_private_key_test_util.h"
#include "net/test/cert_test_util.h"
#include "net/test/keychain_test_util_mac.h"
......@@ -97,6 +99,15 @@ class TestClientCertificateDelegate
DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate);
};
size_t CountAttachedSheets() {
size_t count = 0;
for (NSWindow* child in [NSApp windows]) {
if ([child attachedSheet])
count++;
}
return count;
}
} // namespace
class SSLClientCertificateSelectorMacTest
......@@ -334,6 +345,192 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, HideShow) {
EXPECT_FALSE(results.continue_with_certificate_called());
}
// Test that a dialog shown in a background tab does not become a sheet until
// switching to it.
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
BackgroundActivate) {
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
WebContentsModalDialogManager* web_contents_modal_dialog_manager =
WebContentsModalDialogManager::FromWebContents(web_contents);
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
AddBlankTabAndShow(browser());
TestClientCertificateDelegateResults results;
base::OnceClosure cancellation_callback =
chrome::ShowSSLClientCertificateSelector(
web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results));
base::RunLoop().RunUntilIdle();
// Although the dialog is active, the sheet is not created.
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
EXPECT_EQ(0u, CountAttachedSheets());
// Activate the tab. Now the sheet is created.
chrome::SelectNumberedTab(browser(), 0);
base::RunLoop().RunUntilIdle();
EXPECT_EQ(1u, CountAttachedSheets());
// Close the tab. The delegate should be destroyed without continuing.
chrome::CloseTab(browser());
results.WaitForDelegateDestroyed();
EXPECT_FALSE(results.continue_with_certificate_called());
}
// Test that a dialog shown in a background tab can be canceled by the caller
// (e.g. if the network request is canceled).
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, BackgroundCancel) {
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
WebContentsModalDialogManager* web_contents_modal_dialog_manager =
WebContentsModalDialogManager::FromWebContents(web_contents);
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
AddBlankTabAndShow(browser());
TestClientCertificateDelegateResults results;
base::OnceClosure cancellation_callback =
chrome::ShowSSLClientCertificateSelector(
web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results));
base::RunLoop().RunUntilIdle();
// Although the dialog is active, the sheet is not created.
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
EXPECT_EQ(0u, CountAttachedSheets());
// Cancel the dialog without selecting a certificate. The delegate should be
// destroyed without continuing.
std::move(cancellation_callback).Run();
results.WaitForDelegateDestroyed();
EXPECT_FALSE(results.continue_with_certificate_called());
}
// Test for race conditions if a dialog is triggered on a background tab,
// canceled externally, and then the tab is immediately activate.
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
BackgroundCancelActivate) {
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
WebContentsModalDialogManager* web_contents_modal_dialog_manager =
WebContentsModalDialogManager::FromWebContents(web_contents);
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
AddBlankTabAndShow(browser());
TestClientCertificateDelegateResults results;
base::OnceClosure cancellation_callback =
chrome::ShowSSLClientCertificateSelector(
web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results));
base::RunLoop().RunUntilIdle();
// Although the dialog is active, the sheet is not created.
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
EXPECT_EQ(0u, CountAttachedSheets());
// Cancel the dialog without selecting a certificate and then immediately
// activate the tab.
std::move(cancellation_callback).Run();
chrome::SelectNumberedTab(browser(), 0);
// The sheet should never be created.
EXPECT_EQ(0u, CountAttachedSheets());
base::RunLoop().RunUntilIdle();
EXPECT_EQ(0u, CountAttachedSheets());
// The delegate should be destroyed without continuing.
results.WaitForDelegateDestroyed();
EXPECT_FALSE(results.continue_with_certificate_called());
}
// Test that a dialog shown in a background tab is cleanly canceled if the tab
// is closed.
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, BackgroundClose) {
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
WebContentsModalDialogManager* web_contents_modal_dialog_manager =
WebContentsModalDialogManager::FromWebContents(web_contents);
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
AddBlankTabAndShow(browser());
TestClientCertificateDelegateResults results;
base::OnceClosure cancellation_callback =
chrome::ShowSSLClientCertificateSelector(
web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results));
base::RunLoop().RunUntilIdle();
// Although the dialog is active, the sheet is not created.
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
EXPECT_EQ(0u, CountAttachedSheets());
// Close the background tab without activating it. The delegate should be
// destroyed without continuing.
web_contents->Close();
results.WaitForDelegateDestroyed();
EXPECT_FALSE(results.continue_with_certificate_called());
}
// Test that multiple dialogs within a dialog are queued.
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, DialogQueue) {
auto request1 = base::MakeRefCounted<net::SSLCertRequestInfo>();
request1->host_and_port = net::HostPortPair("foo.test", 443);
auto request2 = base::MakeRefCounted<net::SSLCertRequestInfo>();
request2->host_and_port = net::HostPortPair("bar.test", 443);
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
WebContentsModalDialogManager* web_contents_modal_dialog_manager =
WebContentsModalDialogManager::FromWebContents(web_contents);
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
// Make three requests. The first two use |request1| and the third uses
// |request2|.
TestClientCertificateDelegateResults results1, results2, results3;
chrome::OkAndCancelableForTesting* ok_and_cancelable =
chrome::ShowSSLClientCertificateSelectorMacForTesting(
web_contents, request1.get(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results1),
base::DoNothing());
chrome::ShowSSLClientCertificateSelector(
web_contents, request1.get(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results2));
chrome::ShowSSLClientCertificateSelector(
web_contents, request2.get(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results3));
base::RunLoop().RunUntilIdle();
// Only one of the dialogs is active.
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
EXPECT_EQ(1u, CountAttachedSheets());
// Select a certificate in the first dialog.
ok_and_cancelable->ClickOkButton();
// This should dismiss the first two dialogs, but not the third.
results1.WaitForDelegateDestroyed();
EXPECT_TRUE(results1.continue_with_certificate_called());
EXPECT_EQ(client_cert1_, results1.cert());
results2.WaitForDelegateDestroyed();
EXPECT_TRUE(results2.continue_with_certificate_called());
EXPECT_EQ(client_cert1_, results2.cert());
EXPECT_FALSE(results3.destroyed());
// The third dialog should be visible.
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
EXPECT_EQ(1u, CountAttachedSheets());
// Close the tab. The delegate should be destroyed without continuing.
web_contents->Close();
results3.WaitForDelegateDestroyed();
EXPECT_FALSE(results3.continue_with_certificate_called());
}
// Test that we can't trigger the crash from https://crbug.com/653093
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
WorkaroundTableViewCrash) {
......
......@@ -7,6 +7,7 @@
#include <algorithm>
#include <memory>
#include "base/callback.h"
#include "base/macros.h"
#include "base/no_destructor.h"
#include "build/build_config.h"
......@@ -185,7 +186,8 @@ views::Widget* ShowWebModalDialogViews(
#if defined(OS_MACOSX)
views::Widget* ShowWebModalDialogWithOverlayViews(
views::WidgetDelegate* dialog,
content::WebContents* initiator_web_contents) {
content::WebContents* initiator_web_contents,
base::OnceCallback<void(views::Widget*)> show_sheet) {
DCHECK(CurrentClient());
// For embedded WebContents, use the embedder's WebContents for constrained
// window.
......@@ -195,8 +197,8 @@ views::Widget* ShowWebModalDialogWithOverlayViews(
web_modal::WebContentsModalDialogManager* manager =
web_modal::WebContentsModalDialogManager::FromWebContents(web_contents);
std::unique_ptr<web_modal::SingleWebContentsDialogManager> dialog_manager(
new NativeWebContentsModalDialogManagerViewsMac(widget->GetNativeWindow(),
manager));
new NativeWebContentsModalDialogManagerViewsMac(
widget->GetNativeWindow(), manager, std::move(show_sheet)));
manager->ShowDialogWithManager(widget->GetNativeWindow(),
std::move(dialog_manager));
return widget;
......
......@@ -7,6 +7,7 @@
#include <memory>
#include "base/callback_forward.h"
#include "build/build_config.h"
#include "ui/gfx/native_widget_types.h"
......@@ -61,10 +62,14 @@ views::Widget* ShowWebModalDialogViews(
// Like ShowWebModalDialogViews, but used to show a native dialog "sheet" on
// Mac. Sheets are always modal to their parent window. To make them tab-modal,
// this provides an invisible tab-modal overlay window managed by
// WebContentsModalDialogManager, which can host a dialog sheet.
// WebContentsModalDialogManager, which can host a dialog sheet. The caller
// should not create the sheet until |show_sheet| is called, which may be
// synchronous or in a separate event loop iteration. |show_sheet| is passed the
// overlay window the attach the sheet to.
views::Widget* ShowWebModalDialogWithOverlayViews(
views::WidgetDelegate* dialog,
content::WebContents* initiator_web_contents);
content::WebContents* initiator_web_contents,
base::OnceCallback<void(views::Widget*)> show_sheet);
#endif
// Create a widget for |dialog| that is modal to |web_contents|.
......
......@@ -5,6 +5,7 @@
#ifndef COMPONENTS_CONSTRAINED_WINDOW_NATIVE_WEB_CONTENTS_MODAL_DIALOG_MANAGER_VIEWS_MAC_H_
#define COMPONENTS_CONSTRAINED_WINDOW_NATIVE_WEB_CONTENTS_MODAL_DIALOG_MANAGER_VIEWS_MAC_H_
#include "base/callback.h"
#include "components/constrained_window/native_web_contents_modal_dialog_manager_views.h"
namespace web_modal {
......@@ -22,7 +23,9 @@ class NativeWebContentsModalDialogManagerViewsMac
public:
NativeWebContentsModalDialogManagerViewsMac(
gfx::NativeWindow dialog,
web_modal::SingleWebContentsDialogManagerDelegate* native_delegate);
web_modal::SingleWebContentsDialogManagerDelegate* native_delegate,
base::OnceCallback<void(views::Widget*)> show_sheet);
~NativeWebContentsModalDialogManagerViewsMac() override;
// NativeWebContentsModalDialogManagerViews:
void OnPositionRequiresUpdate() override;
......@@ -30,6 +33,7 @@ class NativeWebContentsModalDialogManagerViewsMac
void HideWidget(views::Widget* widget) override;
private:
base::OnceCallback<void(views::Widget*)> show_sheet_;
DISALLOW_COPY_AND_ASSIGN(NativeWebContentsModalDialogManagerViewsMac);
};
......
......@@ -6,6 +6,8 @@
#import <Cocoa/Cocoa.h>
#include <utility>
#include "components/constrained_window/constrained_window_views.h"
#include "components/web_modal/web_contents_modal_dialog_manager.h"
#include "content/public/browser/web_contents.h"
......@@ -38,8 +40,13 @@ namespace constrained_window {
NativeWebContentsModalDialogManagerViewsMac::
NativeWebContentsModalDialogManagerViewsMac(
gfx::NativeWindow dialog,
web_modal::SingleWebContentsDialogManagerDelegate* native_delegate)
: NativeWebContentsModalDialogManagerViews(dialog, native_delegate) {}
web_modal::SingleWebContentsDialogManagerDelegate* native_delegate,
base::OnceCallback<void(views::Widget*)> show_sheet)
: NativeWebContentsModalDialogManagerViews(dialog, native_delegate),
show_sheet_(std::move(show_sheet)) {}
NativeWebContentsModalDialogManagerViewsMac::
~NativeWebContentsModalDialogManagerViewsMac() = default;
// NativeWebContentsModalDialogManagerViews:
void NativeWebContentsModalDialogManagerViewsMac::OnPositionRequiresUpdate() {
......@@ -78,9 +85,14 @@ void NativeWebContentsModalDialogManagerViewsMac::ShowWidget(
NativeWebContentsModalDialogManagerViews::ShowWidget(widget);
// Make sure the dialog is sized correctly for the correct animations.
OnPositionRequiresUpdate();
if (!show_sheet_.is_null())
std::move(show_sheet_).Run(GetWidget(dialog()));
return;
}
// The sheet must already have been shown.
DCHECK(show_sheet_.is_null());
// Account for window resizes that happen while another tab is open.
OnPositionRequiresUpdate();
SetSheetVisible(dialog_window, true);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment