MacViews: Be robust against some weird/racy teardown paths involving sheets.

Crashes show a task coming from
-[NSWindow _endWindowBlockingModalSession:returnCode:] (before 10.13) or
NSWindowEndWindowModalSession() (10.13 and later). This attempts to access
the |modalDelegate| argument given to -[NSApp beginSheet:...], which is
not retained, and can be destroyed in some lifetimes.

The task is likely the one posted via an asynchronous Close() on a sheet,
attempting to close the sheet using [NSApp endSheet:]. If a parent window
of a sheet then triggers a more abrupt close, the sheet gets closed via
-[NSWindow close] as well. Normally, the delegate would be retained to
also end the sheet (to unblock the sheet machinery on the parent). However,
this only happens if the sheetParent is non-nil. It appears that under
some racy teardown paths the sheet's sheetParent can be nil at this point.

To fix, pass the NSWindow itself as |modalDelegate| to permit WeakPtr-like
semantics on the ViewsNSWindowDelegate. This allows some confusing
lifetimes in ViewsNSWindowDelegate to be tightened up, as well as cleanly
handle the mixed close codepaths that we have to cater for.

Bug: 851376
Change-Id: I1954dc76f7cff5dc3f54f879aaad51fec12b623f
Reviewed-on: https://chromium-review.googlesource.com/1096593
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567134}

ui/views/cocoa/bridged_native_widget.mm

@@ -1347,9 +1347,13 @@ void BridgedNativeWidget::ShowAsModalSheet() {
   NSWindow* parent_window = parent_->GetNSWindow();
   DCHECK(parent_window);
 
+  // -beginSheet: does not retain |modalDelegate| (and we would not want it to).
+  // Since |this| may destroy [window_ delegate], use |window_| itself as the
+  // delegate, which will forward to ViewsNSWindowDelegate if |this| is still
+  // alive (i.e. it has not set the window delegate to nil).
   [NSApp beginSheet:window_
       modalForWindow:parent_window
-       modalDelegate:[window_ delegate]
+       modalDelegate:window_
       didEndSelector:@selector(sheetDidEnd:returnCode:contextInfo:)
          contextInfo:nullptr];
 }

ui/views/cocoa/native_widget_mac_nswindow.h

@@ -18,6 +18,11 @@ VIEWS_EXPORT
 // Set a CommandDispatcherDelegate, i.e. to implement key event handling.
 - (void)setCommandDispatcherDelegate:(id<CommandDispatcherDelegate>)delegate;
 
+// Selector passed to [NSApp beginSheet:]. Forwards to [self delegate], if set.
+- (void)sheetDidEnd:(NSWindow*)sheet
+         returnCode:(NSInteger)returnCode
+        contextInfo:(void*)contextInfo;
+
 @end
 
 #endif  // UI_VIEWS_COCOA_NATIVE_WIDGET_MAC_NSWINDOW_H_

ui/views/cocoa/native_widget_mac_nswindow.mm

@@ -59,6 +59,17 @@
   [commandDispatcher_ setDelegate:delegate];
 }
 
+- (void)sheetDidEnd:(NSWindow*)sheet
+         returnCode:(NSInteger)returnCode
+        contextInfo:(void*)contextInfo {
+  // Note BridgedNativeWidget may have cleared [self delegate], in which case
+  // this will no-op. This indirection is necessary to handle AppKit invoking
+  // this selector via a posted task. See https://crbug.com/851376.
+  [[self viewsNSWindowDelegate] sheetDidEnd:sheet
+                                 returnCode:returnCode
+                                contextInfo:contextInfo];
+}
+
 // Private methods.
 
 - (ViewsNSWindowDelegate*)viewsNSWindowDelegate {

ui/views/cocoa/views_nswindow_delegate.mm

@@ -48,10 +48,6 @@
 - (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();
 }
@@ -99,11 +95,6 @@
 }
 
 - (void)windowWillClose:(NSNotification*)notification {
-  // 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
@@ -111,22 +102,23 @@
     // 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.
+    // window that is closing and -performSelector: won't retain the argument
+    // (putting |window| on the stack above causes this block to retain it).
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(base::RetainBlock(^{
           [sheetParent endSheet:window];
-          [[self retain]
-              release];  // Force |self| to be retained for the block.
         })));
   }
   DCHECK([window isEqual:[notification object]]);
   parent_->OnWindowWillClose();
-  parent_ = nullptr;
+  // |self| may be deleted here (it's NSObject, so who really knows).
+  // |parent_| _will_ be deleted for sure.
+
+  // Note OnWindowWillClose() will clear the NSWindow delegate. That is, |self|.
+  // That guarantees that the task possibly-posted above will never call into
+  // our -sheetDidEnd:. (The task's purpose is just to unblock the modal session
+  // on the parent window.)
+  DCHECK(![window delegate]);
 }
 
 - (void)windowDidMiniaturize:(NSNotification*)notification {

ui/views/widget/native_widget_mac.mm

@@ -392,7 +392,12 @@ void NativeWidgetMac::Close() {
     // sheet has finished animating, it will call sheetDidEnd: on the parent
     // window's delegate. Note it still needs to be asynchronous, since code
     // calling Widget::Close() doesn't expect things to be deleted upon return.
-    [NSApp performSelector:@selector(endSheet:) withObject:window afterDelay:0];
+    // Ensure |window| is retained by a block. Note in some cases during
+    // teardown, [window sheetParent] may be nil.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(base::RetainBlock(^{
+          [NSApp endSheet:window];
+        })));
     return;
   }
 

ui/views/widget/native_widget_mac_unittest.mm

@@ -1354,6 +1354,7 @@ TEST_F(NativeWidgetMacTest, WindowModalSheet) {
 TEST_F(NativeWidgetMacTest, CloseWithWindowModalSheet) {
   NSWindow* native_parent =
       MakeNativeParentWithStyle(NSClosableWindowMask | NSTitledWindowMask);
+
   {
     Widget* sheet_widget = ShowWindowModalWidget(native_parent);
     EXPECT_TRUE([sheet_widget->GetNativeWindow() isVisible]);
@@ -1384,17 +1385,112 @@ TEST_F(NativeWidgetMacTest, CloseWithWindowModalSheet) {
     base::RunLoop().RunUntilIdle();
   }
 
+  // Similar, but invoke -[NSWindow close] immediately after an asynchronous
+  // Close(). This exercises a scenario where two tasks to end the sheet may be
+  // posted. Experimentally (on 10.13) both tasks run, but the second will never
+  // attempt to invoke -didEndSheet: on the |modalDelegate| arg of -beginSheet:.
+  // (If it did, it would be fine.)
+  {
+    Widget* sheet_widget = ShowWindowModalWidget(native_parent);
+    base::scoped_nsobject<NSWindow> sheet_window(
+        sheet_widget->GetNativeWindow(), base::scoped_policy::RETAIN);
+    EXPECT_TRUE([sheet_window isVisible]);
+
+    WidgetChangeObserver widget_observer(sheet_widget);
+    sheet_widget->Close();  // Asynchronous. Can't be called after -close.
+    EXPECT_FALSE(widget_observer.widget_closed());
+    [sheet_window close];
+    EXPECT_TRUE(widget_observer.widget_closed());
+    base::RunLoop().RunUntilIdle();
+
+    // Pretend both tasks ran fully. Note that |sheet_window| serves as its own
+    // |modalDelegate|.
+    [base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>(sheet_window)
+        sheetDidEnd:sheet_window
+         returnCode:NSModalResponseStop
+        contextInfo:nullptr];
+  }
+
+  // Test another hypothetical: What if -sheetDidEnd: was invoked somehow
+  // without going through [NSApp endSheet:] or -[NSWindow endSheet:].
+  {
+    base::mac::ScopedNSAutoreleasePool pool;
+    Widget* sheet_widget = ShowWindowModalWidget(native_parent);
+    NSWindow* sheet_window = sheet_widget->GetNativeWindow();
+    EXPECT_TRUE([sheet_window isVisible]);
+
+    WidgetChangeObserver widget_observer(sheet_widget);
+    sheet_widget->Close();
+
+    [base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>(sheet_window)
+        sheetDidEnd:sheet_window
+         returnCode:NSModalResponseStop
+        contextInfo:nullptr];
+
+    EXPECT_TRUE(widget_observer.widget_closed());
+    // Here, the ViewsNSWindowDelegate should be dealloc'd.
+  }
+  base::RunLoop().RunUntilIdle();  // Run the task posted in Close().
+
+  // Test -[NSWindow close] on the parent window.
   {
     Widget* sheet_widget = ShowWindowModalWidget(native_parent);
     EXPECT_TRUE([sheet_widget->GetNativeWindow() isVisible]);
     WidgetChangeObserver widget_observer(sheet_widget);
 
-    // Test -[NSWindow close] on the parent window.
     [native_parent close];
     EXPECT_TRUE(widget_observer.widget_closed());
   }
 }
 
+// Exercise a scenario where the task posted in the asynchronous Close() could
+// eventually complete on a destroyed NSWindowDelegate. Regression test for
+// https://crbug.com/851376.
+TEST_F(NativeWidgetMacTest, CloseWindowModalSheetWithoutSheetParent) {
+  NSWindow* native_parent =
+      MakeNativeParentWithStyle(NSClosableWindowMask | NSTitledWindowMask);
+  {
+    base::mac::ScopedNSAutoreleasePool pool;
+    Widget* sheet_widget = ShowWindowModalWidget(native_parent);
+    NSWindow* sheet_window = sheet_widget->GetNativeWindow();
+    EXPECT_TRUE([sheet_window isVisible]);
+
+    sheet_widget->Close();  // Asynchronous. Can't be called after -close.
+
+    // Now there's a task to end the sheet in the message queue. But destroying
+    // the NSWindowDelegate without _also_ posting a task that will _retain_ it
+    // is hard. It _is_ possible for a -performSelector:afterDelay: already in
+    // the queue to happen _after_ a PostTask posted now, but it's a very rare
+    // occurrence. So to simulate it, we pretend the sheet isn't actually a
+    // sheet by hiding its sheetParent. This avoids a task being posted that
+    // would retain the delegate, but also puts |native_parent| into a weird
+    // state.
+    //
+    // In fact, the "real" suspected trigger for this bug requires the PostTask
+    // to still be posted, then run to completion, and to dealloc the delegate
+    // it retains all before the -performSelector:afterDelay runs. That's the
+    // theory anyway.
+    //
+    // In reality, it didn't seem possible for -sheetDidEnd: to be invoked twice
+    // (AppKit would suppress it on subsequent calls to -[NSApp endSheet:] or
+    // -[NSWindow endSheet:]), so if the PostTask were to run to completion, the
+    // waiting -performSelector would always no- op. So this is actually testing
+    // a hypothetical where the sheetParent may be somehow nil during teardown
+    // (perhaps due to the parent window being further along in its teardown).
+    EXPECT_TRUE([sheet_window sheetParent]);
+    [sheet_window setValue:nil forKey:@"sheetParent"];
+    EXPECT_FALSE([sheet_window sheetParent]);
+    [sheet_window close];
+
+    // To repro the crash, we need a dealloc to occur here on |sheet_widget|'s
+    // NSWindowDelegate.
+  }
+  // Now there is still a task to end the sheet in the message queue, which
+  // should not crash.
+  base::RunLoop().RunUntilIdle();
+  [native_parent close];
+}
+
 // Test calls to Widget::ReparentNativeView() that result in a no-op on Mac.
 // Tests with both native and non-native parents.
 TEST_F(NativeWidgetMacTest, NoopReparentNativeView) {