[Mac] Move UI item validation to UserInterfaceItemCommandHandler.

ui/views/cocoa/native_widget_mac_nswindow.mm

 // Copyright 2014 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 "ui/views/cocoa/native_widget_mac_nswindow.h"
 
 #include "base/mac/foundation_util.h"
 #import "ui/views/cocoa/views_nswindow_delegate.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/widget/native_widget_mac.h"
 #include "ui/views/widget/widget_delegate.h"
 
 @interface NativeWidgetMacNSWindow ()
 - (ViewsNSWindowDelegate*)viewsNSWindowDelegate;
 - (views::Widget*)viewsWidget;
 - (BOOL)hasViewsMenuActive;
 
 // Private API on NSWindow, determines whether the title is drawn on the title
 // bar. The title is still visible in menus, Expose, etc.
 - (BOOL)_isTitleHidden;
 @end
 
 @implementation NativeWidgetMacNSWindow {
  @private
   base::scoped_nsobject<CommandDispatcher> commandDispatcher_;
 }
 
+@synthesize commandHandler;

[tapted, 2015/09/04 02:56:24]

With this approach (i.e. assign not retain), I think the handler can easily be
destroyed before the NSWindow.

E.g. BrowserFrameMac can remove the last reference to its handler and destroy
it, but the NSWindow can still be receiving events, leading to UAF in this
class.

I think either this needs to be a scoped_nsobject + retain semantics, or the
commandDispatcher should own it instead.

[jackhou1, 2015/09/04 06:13:59]

Changed to retain.

+
 - (instancetype)initWithContentRect:(NSRect)contentRect
                           styleMask:(NSUInteger)windowStyle
                             backing:(NSBackingStoreType)bufferingType
                               defer:(BOOL)deferCreation {
   if ((self = [super initWithContentRect:contentRect
                                styleMask:windowStyle
                                  backing:bufferingType
                                    defer:deferCreation])) {
     commandDispatcher_.reset([[CommandDispatcher alloc] initWithOwner:self]);
   }
@@ skipping 123 matching lines @@
 // CommandDispatchingWindow implementation.
 
 - (BOOL)redispatchKeyEvent:(NSEvent*)event {
   return [commandDispatcher_ redispatchKeyEvent:event];
 }
 
 - (BOOL)defaultPerformKeyEquivalent:(NSEvent*)event {
   return [super performKeyEquivalent:event];
 }
 
+- (void)commandDispatch:(id)sender {
+  [[self commandHandler] commandDispatch:sender window:self];
+}
+
+- (void)commandDispatchUsingKeyModifiers:(id)sender {
+  [[self commandHandler] commandDispatchUsingKeyModifiers:sender window:self];
+}
+
+// NSWindow overrides (NSUserInterfaceItemValidations implementation)
+
+- (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
+  // Since this class implements these selectors, |super| will always say they

[tapted, 2015/09/04 02:56:24]

Then perhaps this logic can move into commandDispatcher and be shared

[jackhou1, 2015/09/04 06:13:59]

Ah, I agree sharing this logic would be good. But we'd need to add
defaultValidateUserInterfaceItem to CommandDispatchingWindow... and I still
don't think CommandDispatcher really needs to be involved.

+  // are enabled. Only use [super] to validate other selectors. If there is no
+  // command handler, defer to AppController.
+  if ([item action] == @selector(commandDispatch:) ||
+      [item action] == @selector(commandDispatchUsingKeyModifiers:)) {
+    return
+        [self commandHandler]
+            ? [[self commandHandler] validateUserInterfaceItem:item window:self]
+            : [[NSApp delegate] validateUserInterfaceItem:item];
+  }
+
+  return [super validateUserInterfaceItem:item];
+}
+
 @end