Work around an AppKit bug that can leave two windows in a key state.

chrome/browser/mac/patch_nsmenu_key_window_bug.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 <AppKit/AppKit.h>
+#import <objc/runtime.h>
+
+// https://crbug.com/692408

[Avi (use Gerrit), 2017/02/28 16:24:24]

Radar # too?

[Sidney San Martín, 2017/03/01 00:50:20]

Done.

+//
+// Back as far as macOS 10.9, and up to at least macOS 10.12.3, an AppKit bug
+// can leave window A in what looks like a key state while window B is actually
+// key. Key presses go to window B even after you click on window A.
+//
+// To trigger the bug, open a menu which has custom views (e.g. the wrench
+// menu) and then, while the menu is open, click the titlebar of a different
+// window to bring it to the front. Then, click the first window to bring it
+// *back* to the front and try to type. Input will go to the other window.
+//
+// ## Cause
+// When a menu has any custom views, AppKit uses a different window
+// class (NSCarbonMenuWindow) to represent it than it does otherwise
+// (NSLimitedMenuViewWindow). NSCarbonMenuWindow overrides -[NSWindow
+// makeKeyWindow] to make itself the key window without telling the old key
+// window that it's lost key status (so that its appearance doesn't change). It
+// saves the old key window in an instance variable.
+//
+// When the menu closes, it doesn't just make the saved key window key again.
+// Instead, it picks the frontmost window that can become key. When the
+// frontmost window is different — and clicking a window's titlebar brings it
+// to the front — it never sends -[NSWindow resignKeyWindow] to the old key
+// window, so it never loses key appearance and doesn't ask NSApplication to
+// make it key when you click on it (since it thinks it's already key).
+//
+// ## Solution
+// This category replaces a method on NSCarbonMenuWindow to send
+// -resignKeyWindow to the saved window if the AppKit implementation made a
+// different window key. It degrades to a noop if NSCarbonMenuWindow, its
+// _rememberedKeyWindow ivar, or its
+// _restorePreviousKeyWindowFromSavedProperties method no longer exists.
+
+WEAK_IMPORT_ATTRIBUTE

[tapted, 2017/02/28 23:24:13]

Is this needed? I don't see it elsewhere in Chrome - comment?

[Sidney San Martín, 2017/03/01 00:50:20]

Added a comment — or did you just mean a comment here :)? It means "set this
symbol to NULL if it's missing at runtime". Availability annotations do the same
thing conditionally — e.g. with a deployment target <10.10, NSVisualEffectView
is imported weakly and [NSVisualEffectView class] may return nil, but when the
deployment target is >=10.10 then that symbol missing will be a crash.

+@interface NSCarbonMenuWindow : NSWindow
+- (void)_restorePreviousKeyWindowFromSavedProperties;
+@end
+
+static void (*_restorePreviousKeyWindowFromSavedProperties)(NSCarbonMenuWindow*,
+                                                            SEL);
+
+static void _restorePreviousKeyWindowFromSavedProperties_patch(
+    NSCarbonMenuWindow* self,
+    SEL _cmd) {
+  NSWindow* rememberedKeyWindow = nil;
+  object_getInstanceVariable(self, "_rememberedKeyWindow",

[tapted, 2017/03/01 02:12:20]

What happens if `_rememberedKeyWindow` gets renamed?

[Sidney San Martín, 2017/03/01 03:08:35]

In that case, object_getInstanceVariable() returns NULL and doesn't touch the
out param, so rememberedKeyWindow will be nil.

[tapted, 2017/03/01 07:56:02]

The return value of object_getInstanceVariable isn't checked (can we DCHECK?).
The risk is that if it's renamed and rememberedKeyWindow is nil, we could fail
to resign a key window that should be resigned. (yes this is a nitpick :)

[Sidney San Martín, 2017/03/01 17:42:59]

OK, done.

+                             (void**)&rememberedKeyWindow);
+  _restorePreviousKeyWindowFromSavedProperties(self, _cmd);
+  if ([NSApp keyWindow] != rememberedKeyWindow) {

[tapted, 2017/02/28 23:24:13]

What happens when switching applications? I think [NSApp keyWindow] is null then
(possibly after a round-trip to the window server). There may be lots of
asynchronous/IPC-with-window-server complications :/

[Sidney San Martín, 2017/03/01 00:50:20]

Switching applications is OK. This all happens in a nested run loop, and NSApp
won't see the deactivation until after the menu has been torn down and returned
control to the main run loop. ([NSApp keyWindow] will match rememberedKeyWindow,
so there's no work for the patch to do.) I haven't found any races or
complications.

+    [rememberedKeyWindow resignKeyWindow];

[tapted, 2017/02/28 23:24:13]

Is the menu still open at this point? Can we do something like

if ([AppMenuController isAnyMenuOpen])

to ensure we only attempt this for the Chrome App Menu, and not other menus?

[Sidney San Martín, 2017/03/01 00:50:20]

Every NSCarbonMenuWindow has this bug, not just the app menu. My goal was a
patch that fixes AppKit and doesn't know about Chrome. If we *really* just want
it to apply to the app menu, this isn't the right approach.

[tapted, 2017/03/01 02:12:20]

I was going to ask "does Chrome have any other menu with NSViews in it", but I
see it also affects the `Help` menu in the menubar, so this is super easy to
repro in every Mac app that has multiple windows (gross).

The goal of restricting it would be to minimise fallout if there's something
we've missed. E.g. some subtle interaction with other menus or special window
types or a specific OS version or a range of private frameworky stuff we don't
know about.

But if you're convinced that NSCarbonMenuWindow is only used in cases where this
bug may manifest, then that's enough for me :)

[Sidney San Martín, 2017/03/01 03:08:35]

That's awesome! I just updated the radar with repro steps for system apps.
I'm convinced that NSCarbonMenuWindow is only used for menus with custom views,
and that this bug occurs whenever it's used. I agree that fallout is
theoretically possible, but (unless I'm wrong and this window class is used
elsewhere) breaking ~2 menus doesn't sound much worse than breaking one, with
the bonus of a fix that works on all of them and isn't entwined with other parts
of Chrome.

+  }
+}
+
+@implementation NSCarbonMenuWindow (KeyWindowPatch)
+
++ (void)load {
+  _restorePreviousKeyWindowFromSavedProperties =

[tapted, 2017/02/28 23:24:13]

This needs to safely bail out (and DCHECK) if Apple remove or rename
-[NSCarbonMenuWindow _restorePreviousKeyWindowFromSavedProperties]   (or if they
remove/rename NSCarbonMenuWindow entirely).

I guess _restorePreviousKeyWindowFromSavedProperties_patch would never get
called anyway, but we need to be clued in somehow if the private API changes.

[Sidney San Martín, 2017/03/01 00:50:20]

It should be safe in these cases; let me know if it isn't.
- If NSCarbonMenuWindow goes away, WEAK_IMPORT_ATTRIBUTE makes the symbol NULL
and this category doesn't load at all.
- If _restorePreviousKeyWindowFromSavedProperties goes away,
class_getInstanceMethod does nothing and returns NULL.

Why a DCHECK? It will fail on the developer's machine if Apple removes or
changes the method, but we need the patch as long as we support macOS versions
that have the bug.

[tapted, 2017/03/01 02:12:20]

Is that how WEAK_IMPORT_ATTRIBUTE works? Wouldn't we need the NSCarbonMenuWindow
in AppKit.Framework to be declared weak? If it's private, Apple might just
remove the symbol. If it's not there at all then it won't be null - it will be a
link error. I don't think we can affect how NSCarbonMenuWindow is linked in a
framework that is already built just be redeclaring it.
What does method_setImplementation do if its first argument is null?
We need a way to detect when this may have stopped working (or, better, Apple
may have fixed it).

What fails if the method is changed? (what kind of changes will cause failure?)

If ever the stuff here would "no-op" we need some kind of alert to developers to
say that we need to check up on this (maybe just to delete it all).

[Sidney San Martín, 2017/03/01 03:08:35]

I'm as sure as I can be that this attribute affects how Chrome imports the
symbol, not how the framework exports it. Otherwise I would expect Chrome to
crash on OSs where e.g. NSTouchBar doesn't exist. Let's bring in thakis@ to
confirm.
It does nothing in that case. You can try it — just change
_restorePreviousKeyWindowFromSavedProperties to something else here and in the
@interface above.
A deployment target check would be ideal. If Apple fixes it, we can fail the
build if our deployment target is greater than that version. For now, I could
add a deployment target test for macOS 10.13, to force us to revisit this file
when we drop support for Sierra in *n* years. I'm open to other ideas.

[tapted, 2017/03/01 07:56:02]

Maybe we can make a little test project? My reading of weak_import is that you
need a runtime check - there isn't one here.

What I think will happen is that when you try to link at compile time with
`-framework AppKitWithoutFoo` then you will get a link error saying that the
implementation for Foo (Category) can't find Foo.

So I don't think WEAK_IMPORT_ATTRIBUTE achieves anything.

NSTouchBar stuff doesn't crash because there's no @implementation for anything
touchbar that relies on linkage -- it's all objc magic -- but I think making a
category makes a link-time dependency on the class you're extending. When we use
categories to reveal functionality in sdk_forward_declarations.h we don't ever
provide an @implementation as well.


As for runtime..  I'm not sure what happens - might depend on how a -load in a
category is executed which I don't know much about.


Is there a reason we want a category? Maybe we just want

@implementation ChromeFiddlingWithNSCarbonMenuWindow
+ (void)load {
  Class carbon_menu_window = NSClassFromString(@"..");
  etc.
}
@end


This is similar to the approach we already use in
render_view_context_menu_mac.mm
I think we only want to revisit if any changes to interfaces on
NSCarbonMenuWindow are detected.

I'd aim for similar level of paranoia to what ChromeSwizzleServicesMenuUpdater
has.

[tapted, 2017/03/01 08:20:35]

Is the API documented to do nothing? Is it really doing nothing or just "not
crashing" :). We shouldn't depend on the method to do something sane for null if
it's not in its contract.

Perhaps it's best here to use ScopedObjCClassSwizzler -- maybe it's not a
perfect fit but it does already have a lot of sanity checks for detecting APIs
changing from under us.

[Sidney San Martín, 2017/03/01 17:42:59]

It's not mentioned for this function, but many runtime functions are documented
(in the header) to do nothing if you pass NULL. The implementation is open
source, and there's an explicit null check:

https://opensource.apple.com/source/objc4/objc4-706/runtime/objc-runtime-new....

I think it's more of a documentation issue than anything else; it seems
implausibly risky for them to change the behavior at this point.
I guess? It seems like overkill to me.

[Sidney San Martín, 2017/03/01 17:42:59]

You're right that it'll fail at compile time if the symbol is missing from the
framework. Sounds like a good thing if we want to know if anything changes?
Passing -Wl,-U,'_OBJC_CLASS_$_NSCarbonMenuWindow' to would tell the linker to
ignore that.

Here's a test:

```
$ cat > objc_weak_import.m <<'EOF'
#import <Foundation/Foundation.h>

WEAK_IMPORT_ATTRIBUTE
@interface NotAClass : NSObject
@end

@implementation NotAClass(Wat)
+ (NSString*)wat {
  return @"success";
}
@end

int main() {
  NSLog(@"+[NotAClass class]: %@", [NotAClass class]);
  NSLog(@"+[NotAClass wat]: %@", [NotAClass wat]);
}
EOF
$ cc -framework Foundation -Wl,-U,'_OBJC_CLASS_$_NotAClass' -o objc_weak_import
objc_weak_import.m
$ ./objc_weak_import
2017-03-01 10:28:44.829 objc_weak_import[53497:770387] +[NotAClass class]:
(null)
2017-03-01 10:28:44.829 objc_weak_import[53497:770387] +[NotAClass wat]: (null)
```

Remove `WEAK_IMPORT_ATTRIBUTE` and build+run again:

```
dyld: Symbol not found: _OBJC_CLASS_$_NotAClass
  Referenced from: /Users/sdy/Desktop/./objc_weak_import
  Expected in: flat namespace
 in /Users/sdy/Desktop/./objc_weak_import
Abort trap: 6
```

[tapted, 2017/03/01 22:56:46]

I guess this thread falls under "general advice" (i.e. be careful what you pass
null to). I generally don't consider `other methods are documented this way` or
`I looked at the current source code` to be valid reasons not to check for null.

But the point of raising it is more that we need to check anyway. If the method
isn't there, we need to DCHECK so we can see if this patch has broken or changed
behaviour.
I think there's more reason to use it here than there is in
render_view_context_menu_mac.mm (where we do already use it). This is a bug that
we actually expect Apple to fix, so they may be changing code in the framework. 

E.g. Apple may add a method argument to
"_restorePreviousKeyWindowFromSavedProperties". ScopedObjCClassSwizzler also has
checks to detect if the argument *types* change, not just the @selector.

[tapted, 2017/03/01 22:56:46]

Does + (void)load behave different? I suspect it does weird things with static
initializers (or the ObjC equivalent).

What's the plan for detecting when the class goes away?

IMO, the WEAK_IMPORT_ATTRIBUTE raises too many questions. We already have an
approach we know works in render_view_context_menu_mac.mm and it doesn't raise
as many questions - we should do the same for this.

And --either way-- we should be consistent. If WEAK_IMPORT_ATTRIBUTE is truly
the Right Way to do these kinds of runtime patches then we should update
render_view_context_menu_mac.mm to do the same thing.

+      (void (*)(NSCarbonMenuWindow*, SEL))method_setImplementation(
+          class_getInstanceMethod(
+              [self class],
+              @selector(_restorePreviousKeyWindowFromSavedProperties)),
+          (IMP)_restorePreviousKeyWindowFromSavedProperties_patch);
+}
+
+@end