[Mac] Fix NSColorPanel crasher on macOS 10.13.

The ColorPanelCocoa sets itself as the target and delegate of the
NSColorPanel, and on dealloc it clears the NSColorPanel's target and
delegate settings if it sees that it is the delegate. On 10.13 it
appears that the NSColorPanel's delegate can get set to nil while
leaving the target intact, preventing -dealloc from clearing the
ColorPanelCocoa target and leading to a Zombie crash.

This cl uses a private method to check the NSColorPanel's target in
-dealloc and clear it if it's still the ColorPanelCocoa.

R=rsesek@chromium.org,avi@chromium.org

Bug: 767598
Change-Id: I86557f3dadc65b8e0d455156457d960d8f45044e
Reviewed-on: https://chromium-review.googlesource.com/685394
Commit-Queue: Jayson Adams <shrike@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517645}

chrome/browser/ui/BUILD.gn

@@ -1704,6 +1704,7 @@ split_static_library("ui") {
       "cocoa/chrome_command_dispatcher_delegate.mm",
       "cocoa/chrome_style.cc",
       "cocoa/chrome_style.h",
+      "cocoa/color_chooser_mac.h",
       "cocoa/color_chooser_mac.mm",
       "cocoa/confirm_quit.h",
       "cocoa/confirm_quit_panel_controller.h",

chrome/browser/ui/cocoa/color_chooser_mac.h

+// Copyright 2017 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.
+
+#ifndef CHROME_BROWSER_UI_COCOA_COLOR_CHOOSER_MAC_H_
+#define CHROME_BROWSER_UI_COCOA_COLOR_CHOOSER_MAC_H_
+
+#import <Cocoa/Cocoa.h>
+
+#import "base/mac/scoped_nsobject.h"
+#include "content/public/browser/color_chooser.h"
+#include "content/public/browser/web_contents.h"
+
+class ColorChooserMac;
+
+// A Listener class to act as a event target for NSColorPanel and send
+// the results to the C++ class, ColorChooserMac.
+@interface ColorPanelCocoa : NSObject<NSWindowDelegate> {
+ @protected
+  // We don't call DidChooseColor if the change wasn't caused by the user
+  // interacting with the panel.
+  BOOL nonUserChange_;
+ @private
+  ColorChooserMac* chooser_;  // weak, owns this
+}
+
+- (id)initWithChooser:(ColorChooserMac*)chooser;
+
+// Called from NSColorPanel.
+- (void)didChooseColor:(NSColorPanel*)panel;
+
+// Sets color to the NSColorPanel as a non user change.
+- (void)setColor:(NSColor*)color;
+
+@end
+
+class ColorChooserMac : public content::ColorChooser {
+ public:
+  // Returns a ColorChooserMac instance owned by the ColorChooserMac class -
+  // call End() when done to free it. Each call to Open() returns a new
+  // instance after freeing the previous one (i.e. it does not reuse the
+  // previous instance even if it still exists).
+  static ColorChooserMac* Open(content::WebContents* web_contents,
+                               SkColor initial_color);
+
+  // Called from ColorPanelCocoa.
+  void DidChooseColorInColorPanel(SkColor color);
+  void DidCloseColorPabel();
+
+  // Set the color programmatically.
+  void SetSelectedColor(SkColor color) override;
+
+  // Call when done with the ColorChooserMac.
+  void End() override;
+
+ private:
+  ColorChooserMac(content::WebContents* tab, SkColor initial_color);
+
+  ~ColorChooserMac() override;
+
+  static ColorChooserMac* current_color_chooser_;
+
+  // The web contents invoking the color chooser.  No ownership because it will
+  // outlive this class.
+  content::WebContents* web_contents_;
+  base::scoped_nsobject<ColorPanelCocoa> panel_;
+
+  DISALLOW_COPY_AND_ASSIGN(ColorChooserMac);
+};
+
+#endif  // CHROME_BROWSER_UI_COCOA_COLOR_CHOOSER_MAC_H_

chrome/browser/ui/cocoa/color_chooser_mac.mm

@@ -2,61 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#import <Cocoa/Cocoa.h>
+#import "chrome/browser/ui/cocoa/color_chooser_mac.h"
 
 #include "base/logging.h"
-#import "base/mac/scoped_nsobject.h"
 #include "chrome/browser/ui/browser_dialogs.h"
-#include "content/public/browser/color_chooser.h"
-#include "content/public/browser/web_contents.h"
 #include "skia/ext/skia_utils_mac.h"
 
-class ColorChooserMac;
-
-// A Listener class to act as a event target for NSColorPanel and send
-// the results to the C++ class, ColorChooserMac.
-@interface ColorPanelCocoa : NSObject<NSWindowDelegate> {
- @private
-  // We don't call DidChooseColor if the change wasn't caused by the user
-  // interacting with the panel.
-  BOOL nonUserChange_;
-  ColorChooserMac* chooser_;  // weak, owns this
-}
-
-- (id)initWithChooser:(ColorChooserMac*)chooser;
-
-// Called from NSColorPanel.
-- (void)didChooseColor:(NSColorPanel*)panel;
-
-// Sets color to the NSColorPanel as a non user change.
-- (void)setColor:(NSColor*)color;
-
-@end
-
-class ColorChooserMac : public content::ColorChooser {
- public:
-  static ColorChooserMac* Open(content::WebContents* web_contents,
-                               SkColor initial_color);
-
-  ColorChooserMac(content::WebContents* tab, SkColor initial_color);
-  ~ColorChooserMac() override;
-
-  // Called from ColorPanelCocoa.
-  void DidChooseColorInColorPanel(SkColor color);
-  void DidCloseColorPabel();
-
-  void End() override;
-  void SetSelectedColor(SkColor color) override;
-
- private:
-  static ColorChooserMac* current_color_chooser_;
-
-  // The web contents invoking the color chooser.  No ownership because it will
-  // outlive this class.
-  content::WebContents* web_contents_;
-  base::scoped_nsobject<ColorPanelCocoa> panel_;
-};
-
 ColorChooserMac* ColorChooserMac::current_color_chooser_ = NULL;
 
 // static
@@ -104,6 +55,11 @@ void ColorChooserMac::SetSelectedColor(SkColor color) {
   [panel_ setColor:skia::SkColorToDeviceNSColor(color)];
 }
 
+@interface NSColorPanel (Private)
+// Private method returning the NSColorPanel's target.
+- (id)__target;
+@end
+
 @implementation ColorPanelCocoa
 
 - (id)initWithChooser:(ColorChooserMac*)chooser {
@@ -120,10 +76,18 @@ void ColorChooserMac::SetSelectedColor(SkColor color) {
 
 - (void)dealloc {
   NSColorPanel* panel = [NSColorPanel sharedColorPanel];
-  if ([panel delegate] == self) {
+
+  // On macOS 10.13 the NSColorPanel delegate can apparently get reset to nil
+  // with the target left unchanged. Use the private __target method to see if
+  // the ColorPanelCocoa is still the target.
+  BOOL respondsToPrivateTargetMethod =
+      [panel respondsToSelector:@selector(__target)];
+
+  if ([panel delegate] == self ||
+      (respondsToPrivateTargetMethod && [panel __target] == self)) {
     [panel setDelegate:nil];
     [panel setTarget:nil];
-    [panel setAction:nil];
+    [panel setAction:nullptr];
   }
 
   [super dealloc];

chrome/browser/ui/cocoa/color_panel_cocoa_unittest.mm

+// Copyright 2017 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/color_chooser_mac.h"
+
+#import "chrome/browser/ui/cocoa/test/cocoa_test_helper.h"
+#include "skia/ext/skia_utils_mac.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/gtest_mac.h"
+#include "testing/platform_test.h"
+
+@interface NSColorPanel (Private)
+// Private method returning the NSColorPanel's target.
+- (id)__target;
+@end
+
+namespace {
+
+class ColorPanelCocoaTest : public CocoaTest {
+  void SetUp() override {
+    // Create the color panel and call Init() again to update its initial
+    // window list to include it. The NSColorPanel cannot be dealloced, so
+    // without this step the tests will fail complaining that not all windows
+    // were closed.
+    [[NSColorPanel sharedColorPanel] makeKeyAndOrderFront:nil];
+    Init();
+  }
+};
+
+TEST_F(ColorPanelCocoaTest, ClearTargetAndDelegateOnEnd) {
+  NSColorPanel* nscolor_panel = [NSColorPanel sharedColorPanel];
+  EXPECT_TRUE([nscolor_panel respondsToSelector:@selector(__target)]);
+
+  // Create a ColorPanelCocoa.
+  ColorChooserMac* color_chooser_mac =
+      ColorChooserMac::Open(nullptr, SK_ColorBLACK);
+
+  // Confirm the NSColorPanel's configuration by the ColorChooserMac's
+  // ColorPanelCocoa.
+  EXPECT_TRUE([nscolor_panel delegate]);
+  EXPECT_TRUE([nscolor_panel __target]);
+
+  // Release the ColorPanelCocoa and confirm it's no longer the NSColorPanel's
+  // target or delegate.
+  color_chooser_mac->End();
+
+  EXPECT_EQ([nscolor_panel delegate], nil);
+  EXPECT_EQ([nscolor_panel __target], nil);
+}
+
+TEST_F(ColorPanelCocoaTest, ClearTargetOnEnd) {
+  NSColorPanel* nscolor_panel = [NSColorPanel sharedColorPanel];
+  EXPECT_TRUE([nscolor_panel respondsToSelector:@selector(__target)]);
+
+  // Create a ColorPanelCocoa.
+  ColorChooserMac* color_chooser_mac =
+      ColorChooserMac::Open(nullptr, SK_ColorBLACK);
+
+  // Confirm the NSColorPanel's configuration by the ColorChooserMac's
+  // ColorPanelCocoa.
+  EXPECT_TRUE([nscolor_panel delegate]);
+  EXPECT_TRUE([nscolor_panel __target]);
+
+  // Clear the delegate and release the ColorPanelCocoa.
+  [nscolor_panel setDelegate:nil];
+
+  // Release the ColorPanelCocoa.
+  color_chooser_mac->End();
+
+  // Confirm the ColorPanelCocoa is no longer the NSColorPanel's target or
+  // delegate. Previously the ColorPanelCocoa would not clear the target if
+  // the delegate had already been cleared.
+  EXPECT_EQ([nscolor_panel delegate], nil);
+  EXPECT_EQ([nscolor_panel __target], nil);
+}
+
+TEST_F(ColorPanelCocoaTest, SetColor) {
+  // Set the NSColor panel up with an intial color.
+  NSColor* blue_color = [NSColor blueColor];
+  NSColorPanel* nscolor_panel = [NSColorPanel sharedColorPanel];
+  [nscolor_panel setColor:blue_color];
+  EXPECT_TRUE([[nscolor_panel color] isEqual:blue_color]);
+
+  // Create a ColorChooserMac and confirm the NSColorPanel gets its initial
+  // color.
+  SkColor initial_color = SK_ColorBLACK;
+  ColorChooserMac* color_chooser_mac =
+      ColorChooserMac::Open(nullptr, SK_ColorBLACK);
+
+  EXPECT_NSEQ([nscolor_panel color],
+              skia::SkColorToDeviceNSColor(initial_color));
+
+  // Confirm that -[ColorPanelCocoa setColor:] sets the NSColorPanel's color.
+  SkColor test_color = SK_ColorRED;
+  color_chooser_mac->SetSelectedColor(test_color);
+
+  EXPECT_NSEQ([nscolor_panel color], skia::SkColorToDeviceNSColor(test_color));
+
+  // Clean up.
+  color_chooser_mac->End();
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -3720,6 +3720,7 @@ test("unit_tests") {
         "../browser/ui/cocoa/bubble_view_unittest.mm",
         "../browser/ui/cocoa/chrome_browser_window_unittest.mm",
         "../browser/ui/cocoa/clickhold_button_cell_unittest.mm",
+        "../browser/ui/cocoa/color_panel_cocoa_unittest.mm",
         "../browser/ui/cocoa/confirm_bubble_controller_unittest.mm",
         "../browser/ui/cocoa/confirm_quit_panel_controller_unittest.mm",
         "../browser/ui/cocoa/constrained_window/constrained_window_alert_unittest.mm",