MacViews: Support -[NSWindow close] on sheets.

ui/views/cocoa/views_nswindow_delegate.mm

Index: ui/views/cocoa/views_nswindow_delegate.mm
diff --git a/ui/views/cocoa/views_nswindow_delegate.mm b/ui/views/cocoa/views_nswindow_delegate.mm
index 7a70d44b1e9ecc55a46a08bad7167b99d38ff05e..8fee708a4a6a7a0ffc740dd1db2741018466212f 100644
--- a/ui/views/cocoa/views_nswindow_delegate.mm
+++ b/ui/views/cocoa/views_nswindow_delegate.mm
@@ -5,6 +5,8 @@
 #import "ui/views/cocoa/views_nswindow_delegate.h"
 
 #include "base/logging.h"
+#import "base/mac/bind_objc_block.h"
+#include "base/threading/thread_task_runner_handle.h"
 #import "ui/views/cocoa/bridged_content_view.h"
 #import "ui/views/cocoa/bridged_native_widget.h"
 #include "ui/views/widget/native_widget_mac.h"
@@ -46,6 +48,10 @@
 - (void)sheetDidEnd:(NSWindow*)sheet
          returnCode:(NSInteger)returnCode
         contextInfo:(void*)contextInfo {
+  // |parent_| will be null when triggered from the block in -windowWillClose:.
+  if (!parent_)
+    return;
+
   [sheet orderOut:nil];
   parent_->OnWindowWillClose();
 }
@@ -86,9 +92,36 @@
   parent_->OnWindowKeyStatusChangedTo(false);
 }
 
+// Does nothing: Used to force |self| to be retained in a block.

[Robert Sesek, 2017/01/13 20:02:47]

Could you instead just put another keepAlive in the block?

[tapted, 2017/01/13 20:52:51]

That would take (and release) an additional reference :). Currently there's just
the reference being held by the ObjC block machinery. i.e. It would work, but it
would effectively be replacing

[self doNothing];

with

[self retain];
[self release];

So I don't know if that's better. I do think `doNothing` is weird, but I didn't
want to use additional retain/release since it might suggest that the block
doesn't already have a ref to begin with that exists for the lifetime of the
block (or that we need to account for some other hidden weirdness..)

But I'm not otherwise attached to `doNothing` - e.g. I thought about calling
some random NSObject property, but thought "doNothing" gave slightly more
meaning.

[Robert Sesek, 2017/01/13 21:46:00]

Hm, I see your point. Maybe rather than doNothing, you end the block with [[self
retain] release]; ?

[tapted, 2017/01/13 22:32:45]

Done!

+- (void)doNothing {}
+
 - (void)windowWillClose:(NSNotification*)notification {
-  DCHECK([parent_->ns_window() isEqual:[notification object]]);
+  // Retain |self|. |parent_| should be cleared. OnWindowWillClose() may delete
+  // |parent_|, but it may also dealloc |self| before returning. However, the
+  // observers it notifies before that need a valid |parent_| on the delegate,
+  // so it can only be cleared after OnWindowWillClose() returns.
+  base::scoped_nsobject<NSObject> keepAlive(self, base::scoped_policy::RETAIN);
+  NSWindow* window = parent_->ns_window();
+  if (NSWindow* sheetParent = [window sheetParent]) {
+    // On no! Something called -[NSWindow close] on a sheet rather than calling
+    // -[NSWindow endSheet:] on its parent. If the modal session is not ended
+    // then the parent will never be able to show another sheet. But calling
+    // -endSheet: here will block the thread with an animation, so post a task.
+    // Use a block: The argument to -endSheet: must be retained, since it's the
+    // window that is closing and -performSelector: won't retain the argument.
+    // The NSWindowDelegate (i.e. |self|) must also be explicitly retained. Even
+    // though the call to OnWindowWillClose() below will remove |self| as the
+    // NSWindow delegate, the call to -[NSApp beginSheet:] also took a weak
+    // reference to the delegate, which will be destroyed when the sheet's
+    // BridgedNativeWidget is destroyed.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, base::BindBlock(^{
+      [sheetParent endSheet:window];
+      [self doNothing];
+    }));
+  }
+  DCHECK([window isEqual:[notification object]]);
   parent_->OnWindowWillClose();
+  parent_ = nullptr;
 }
 
 - (void)windowDidMiniaturize:(NSNotification*)notification {