Mac SelectFileDialog TLC

- Resurrects shell_dialogs_unittests on Mac
- Fixes 2 tests that had regressed since the gn transition.
- Tweaks the code to be more robust around complex object lifetimes.
- Adds test SelectFileDialogMacTest.Lifetime

Bug: 788271
Change-Id: Ice43179a1da3b7bbcd44fb9b0efdf6d84e4d2f5b
Reviewed-on: https://chromium-review.googlesource.com/920961Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537419}

ui/shell_dialogs/BUILD.gn

@@ -8,6 +8,9 @@ import("//testing/test.gni")
 if (is_android) {
   import("//build/config/android/config.gni")
 }
+if (is_mac) {
+  import("//build/config/mac/rules.gni")
+}
 
 jumbo_component("shell_dialogs") {
   sources = [
@@ -76,11 +79,31 @@ jumbo_component("shell_dialogs") {
   }
 }
 
+if (is_mac) {
+  mac_xib_bundle_data("shell_dialogs_unittests_xibs") {
+    testonly = true
+    sources = [
+      "//chrome/app/nibs/SaveAccessoryView.xib",
+    ]
+  }
+
+  mac_framework_bundle("shell_dialogs_unittests_bundle") {
+    testonly = true
+    info_plist = "//ui/base/test/framework-Info.plist"
+    deps = [
+      ":shell_dialogs_unittests_xibs",
+      "//ui/resources:ui_test_pak_bundle_data",
+    ]
+    extra_substitutions = [ "CHROMIUM_BUNDLE_ID=$target_name" ]
+    output_name = "shell_dialogs_unittests"
+  }
+}
+
 test("shell_dialogs_unittests") {
+  testonly = true
   sources = [
-    # TODO(karandeepb) : Revisit this once gn gets mac bundle support.
-    # "select_file_dialog_mac_unittest.mm",
     "run_all_unittests.cc",
+    "select_file_dialog_mac_unittest.mm",
     "select_file_dialog_unittest.cc",
     "select_file_dialog_win_unittest.cc",
   ]
@@ -92,4 +115,8 @@ test("shell_dialogs_unittests") {
     "//testing/gtest",
     "//ui/base:base",
   ]
+
+  if (is_mac) {
+    deps += [ ":shell_dialogs_unittests_bundle" ]
+  }
 }

ui/shell_dialogs/run_all_unittests.cc

@@ -44,9 +44,7 @@ void ShellDialogsTestSuite::Initialize() {
   // Set up framework bundle so that tests on Mac can access nib files.
   base::FilePath path;
   PathService::Get(base::DIR_EXE, &path);
-  // The three DirName() calls strip "Contents/MacOS/<binary>" from the path.
-  path = path.DirName().DirName().DirName();
-  path = path.Append(FILE_PATH_LITERAL("shell_dialogs_unittests.app"));
+  path = path.Append(FILE_PATH_LITERAL("shell_dialogs_unittests.framework"));
   base::mac::SetOverrideFrameworkBundlePath(path);
 
   // Setup resource bundle.

ui/shell_dialogs/select_file_dialog_mac.mm

@@ -59,6 +59,7 @@ NSString* GetDescriptionFromExtension(const base::FilePath::StringType& ext) {
 }
 
 - (id)initWithSelectFileDialogImpl:(ui::SelectFileDialogImpl*)s;
+- (void)selectFileDialogImplWillBeDestroyed;
 - (void)endedPanel:(NSSavePanel*)panel
          didCancel:(bool)did_cancel
               type:(ui::SelectFileDialog::Type)type
@@ -209,7 +210,7 @@ void SelectFileDialogImpl::SelectFileImpl(
       [dialog setCanSelectHiddenExtension:YES];
     }
   } else {
-    NSOpenPanel* open_dialog = (NSOpenPanel*)dialog;
+    NSOpenPanel* open_dialog = base::mac::ObjCCastStrict<NSOpenPanel>(dialog);
 
     if (type == SELECT_OPEN_MULTI_FILE)
       [open_dialog setAllowsMultipleSelection:YES];
@@ -235,13 +236,23 @@ void SelectFileDialogImpl::SelectFileImpl(
     [dialog setDirectoryURL:[NSURL fileURLWithPath:default_dir]];
   if (default_filename)
     [dialog setNameFieldStringValue:default_filename];
+
+  // Ensure the bridge (rather than |this|) is retained by the block.
+  SelectFileDialogBridge* bridge = bridge_.get();
   [dialog beginSheetModalForWindow:owning_window
                  completionHandler:^(NSInteger result) {
-    [bridge_.get() endedPanel:dialog
-                    didCancel:result != NSFileHandlingPanelOKButton
-                         type:type
-                 parentWindow:owning_window];
-  }];
+                   [bridge endedPanel:dialog
+                            didCancel:result != NSFileHandlingPanelOKButton
+                                 type:type
+                         parentWindow:owning_window];
+
+                   // Balance the setDelegate above. Note this should usually
+                   // have been done already in FileWasSelected().
+                   [dialog setDelegate:nil];
+
+                   // Balance the retain at the start of SelectFileImpl().
+                   [dialog release];
+                 }];
 }
 
 SelectFileDialogImpl::DialogData::DialogData(
@@ -263,6 +274,12 @@ SelectFileDialogImpl::~SelectFileDialogImpl() {
 
   for (const auto& panel : panels)
     [panel cancel:panel];
+
+  // Running |cancel| on all the panels should have run all the completion
+  // handlers, but retaining references to C++ objects inside an NSObject can
+  // result in subtle problems. Ensure the reference to |this| is cleared.
+  DCHECK(dialog_data_map_.empty());
+  [bridge_ selectFileDialogImplWillBeDestroyed];
 }
 
 // static
@@ -384,10 +401,17 @@ SelectFileDialog* CreateSelectFileDialog(
   return self;
 }
 
+- (void)selectFileDialogImplWillBeDestroyed {
+  selectFileDialogImpl_ = nullptr;
+}
+
 - (void)endedPanel:(NSSavePanel*)panel
          didCancel:(bool)did_cancel
               type:(ui::SelectFileDialog::Type)type
       parentWindow:(NSWindow*)parentWindow {
+  if (!selectFileDialogImpl_)
+    return;
+
   int index = 0;
   std::vector<base::FilePath> paths;
   if (!did_cancel) {
@@ -422,7 +446,6 @@ SelectFileDialog* CreateSelectFileDialog(
                                          isMulti,
                                          paths,
                                          index);
-  [panel release];
 }
 
 - (BOOL)panel:(id)sender shouldEnableURL:(NSURL *)url {

ui/shell_dialogs/select_file_dialog_mac_unittest.mm

@@ -13,6 +13,7 @@
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/shell_dialogs/select_file_policy.h"
 
 #define EXPECT_EQ_BOOL(a, b) \
   EXPECT_EQ(static_cast<bool>(a), static_cast<bool>(b))
@@ -305,9 +306,12 @@ TEST_F(SelectFileDialogMacTest, EmptyDescription) {
       GetExtensionDescriptionList(popup);
   EXPECT_EQ(3lu, extension_descriptions.size());
   // Verify that the correct system description is produced for known file types
-  // like pdf if no extension description is provided by the client.
-  EXPECT_EQ(base::ASCIIToUTF16("Portable Document Format (PDF)"),
-            extension_descriptions[0]);
+  // like pdf if no extension description is provided by the client. Search the
+  // string for "PDF" as the system may display:
+  // - Portable Document Format (PDF)
+  // - PDF document
+  EXPECT_NE(base::string16::npos,
+            extension_descriptions[0].find(base::ASCIIToUTF16("PDF")));
   EXPECT_EQ(base::ASCIIToUTF16("Image"), extension_descriptions[1]);
   // Verify the description for unknown file types if no extension description
   // is provided by the client.
@@ -440,6 +444,8 @@ TEST_F(SelectFileDialogMacTest, DefaultPath) {
   SelectFileWithParams(args);
   NSSavePanel* panel = GetPanel();
 
+  [panel setExtensionHidden:NO];
+
   EXPECT_EQ(args.default_path.DirName(),
             base::mac::NSStringToFilePath([[panel directoryURL] path]));
   EXPECT_EQ(args.default_path.BaseName(),
@@ -466,5 +472,34 @@ TEST_F(SelectFileDialogMacTest, MultipleExtension) {
   EXPECT_FALSE([panel isExtensionHidden]);
 }
 
+// Test to ensure lifetime is sound if a reference to the panel outlives the
+// delegate.
+TEST_F(SelectFileDialogMacTest, Lifetime) {
+  base::scoped_nsobject<NSSavePanel> panel;
+  @autoreleasepool {
+    auto args = GetDefaultArguments();
+    // Set a type (Save dialogs do not have a delegate).
+    args.type = SelectFileDialog::SELECT_OPEN_MULTI_FILE;
+    SelectFileWithParams(args);
+    panel.reset([GetPanel() retain]);
+
+    EXPECT_TRUE([panel isVisible]);
+    EXPECT_NE(nil, [panel delegate]);
+
+    // Newer versions of AppKit may clear out weak delegate pointers when
+    // dealloc is called on the delegate. Put a ref into the autorelease pool to
+    // simulate what happens on older versions.
+    [[[panel delegate] retain] autorelease];
+
+    ResetDialog();
+
+    // The SelectFileDialogImpl destructor invokes [panel cancel]. That should
+    // close the panel, and run the completion handler.
+    EXPECT_EQ(nil, [panel delegate]);
+    EXPECT_FALSE([panel isVisible]);
+  }
+  EXPECT_EQ(nil, [panel delegate]);
+}
+
 }  // namespace test
 }  // namespace ui