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

Index: chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
diff --git a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
index f16a443928269edbd24b319ef9babfb8ae166dbd..07440ae2d370b0d45568fabc9cfcff11b62675fa 100644
--- a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
+++ b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
@@ -105,6 +105,43 @@ void ShowSSLClientCertificateSelector(
 
 }  // 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
@@ -121,7 +158,7 @@ void ShowSSLClientCertificateSelector(
   return self;
 }
 
-- (void)sheetDidEnd:(NSWindow*)parent
+- (void)sheetDidEnd:(NSWindow*)sheet
          returnCode:(NSInteger)returnCode
             context:(void*)context {
   net::X509Certificate* cert = NULL;
@@ -135,6 +172,9 @@ void ShowSSLClientCertificateSelector(
       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