Work around an AppKit bug that can crash Chrome after using the client certificate prompt.

chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h"
 
 #import <SecurityInterface/SFChooseIdentityPanel.h>
 #include <stddef.h>
 
 #include <utility>
@@ skipping 87 matching lines @@
   SSLClientCertificateSelectorCocoa* selector =
       [[SSLClientCertificateSelectorCocoa alloc]
           initWithBrowserContext:contents->GetBrowserContext()
                  certRequestInfo:cert_request_info
                         delegate:std::move(delegate)];
   [selector displayForWebContents:contents];
 }
 
 }  // namespace chrome
 
+namespace {
+
+// These ClearTableViewDataSources... functions help work around a bug in macOS
+// 10.12 where SFChooseIdentityPanel leaks a window and some views, including
+// an NSTableView. Future events may make cause the table view to query its
+// dataSource, which will have been deallocated.
+//
+// NSTableView.dataSource becomes a zeroing weak reference starting in 10.11,

[Robert Sesek, 2016/11/29 21:31:36]

Use the availability macros to only do this work if our SDK is <10.11 maybe?

[Sidney San Martín, 2016/11/29 22:55:07]

Done.

+// so this workaround can be removed once we're on the 10.11 SDK.
+//
+// rdar://29409207
+// BUG=653093

[Robert Sesek, 2016/11/29 21:31:35]

Make this a https://crbug.com/ URL please. (Could collapse with the rdar line
like, "See https://crbug.com/x and rdar://y for more information").

[Sidney San Martín, 2016/11/29 22:55:07]

Done.

+
+void ClearTableViewDataSources(NSView* view){
+  if (auto tableView = base::mac::ObjCCast<NSTableView>(view)) {

[Robert Sesek, 2016/11/29 21:31:35]

naming: table_view

[Sidney San Martín, 2016/11/29 22:55:07]

Done.

+    tableView.dataSource = nil;
+  } else {
+    for (NSView* subview in view.subviews) {
+      ClearTableViewDataSources(subview);
+    }
+  }
+}
+
+void ClearTableViewDataSourcesIfWindowStillAround(
+    __unsafe_unretained NSWindow* leakedWindow) {

[Robert Sesek, 2016/11/29 21:31:35]

naming: leaked_window

[Robert Sesek, 2016/11/29 21:31:35]

Is the __unsafe_unretained so the block doesn't grab a ref? Comment.

[Sidney San Martín, 2016/11/29 22:55:07]

Done.

[Sidney San Martín, 2016/11/29 22:55:07]

That's the idea. Without ARC it's a noop, but I think it serves as good
documentation/future proofing if we do switch on ARC.

[Robert Sesek, 2016/11/30 00:23:32]

I don't think this is a practice we currently follow (and I don't think a Mac
ARC switch is really on the horizon), but I don't feel strongly against it.

[Sidney San Martín, 2016/11/30 04:19:50]

That's fair — and base::Unretained also gets that point across. Removed.

+  dispatch_async(dispatch_get_main_queue(), ^{

[Robert Sesek, 2016/11/29 21:31:35]

Why does this need to be done in dispatch_async()? Leave a comment if it's
necessary. Though I'd probably have used PostTask from within -sheetDidEnd:.

[Sidney San Martín, 2016/11/29 22:55:07]

At the call site, the window is still in an autorelease pool. The
dispatch_async() lets us skip the rest of the workaround if/when Apple fixes the
leak.

Could you tell me more about PostTask? I haven't used it yet.

[Robert Sesek, 2016/11/30 00:23:32]

It's Chrome's task posting system. You can use it to post a closure to the
current thread like so:

  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
      base::Bind(ClearTableViewDataSourcesIfWindowStillAround,
                 base::Unretained(sheet)));

I am a little hesitant about using dispatch_async() for something like this,
since blocks get processed by the runloop in several places, and when the
autorelease pool drains is not defined. We have a better sense of when
MessageLoop/PostTask work runs, especially with respect to the autorelease pool.

[Sidney San Martín, 2016/11/30 04:19:50]

Done (mostly). If I get what you're saying, then moving the PostTask() into
-sheetDidEnd: would require a second SDK check. Is that what you meant?

[Robert Sesek, 2016/11/30 16:24:48]

It is, but what you did is fine as well.

+    for (NSWindow* window : [NSApp windows]) {
+      if (window == leakedWindow) {
+        ClearTableViewDataSources(window.contentView);
+        break;
+      }
+    }
+  });
+}
+
+}  // namespace
+
 @implementation SSLClientCertificateSelectorCocoa
 
 - (id)initWithBrowserContext:(const content::BrowserContext*)browserContext
              certRequestInfo:(net::SSLCertRequestInfo*)certRequestInfo
                     delegate:
                         (std::unique_ptr<content::ClientCertificateDelegate>)
                             delegate {
   DCHECK(browserContext);
   DCHECK(certRequestInfo);
   if ((self = [super init])) {
     observer_.reset(new SSLClientAuthObserverCocoaBridge(
         browserContext, certRequestInfo, std::move(delegate), self));
   }
   return self;
 }
 
-- (void)sheetDidEnd:(NSWindow*)parent
+- (void)sheetDidEnd:(NSWindow*)sheet
          returnCode:(NSInteger)returnCode
             context:(void*)context {
   net::X509Certificate* cert = NULL;
   if (returnCode == NSFileHandlingPanelOKButton) {
     CFRange range = CFRangeMake(0, CFArrayGetCount(identities_));
     CFIndex index =
         CFArrayGetFirstIndexOfValue(identities_, range, [panel_ identity]);
     if (index != -1)
       cert = certificates_[index].get();
     else
       NOTREACHED();
   }
 
+  // See comment at definition; this works around a 10.12 bug.
+  ClearTableViewDataSourcesIfWindowStillAround(sheet);
+
   if (!closePending_) {
     // If |closePending_| is already set, |closeSheetWithAnimation:| was called
     // already to cancel the selection rather than continue with no
     // certificate. Otherwise, tell the backend which identity (or none) the
     // user selected.
     userResponded_ = YES;
     observer_->CertificateSelected(cert);
 
     constrainedWindow_->CloseWebContentsModalDialog();
   }
@@ skipping 122 matching lines @@
 }
 
 - (void)onConstrainedWindowClosed {
   observer_->StopObserving();
   panel_.reset();
   constrainedWindow_.reset();
   [self release];
 }
 
 @end