Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710763003:
Work around an AppKit bug that can leave two windows in a key state.
|
Created:
3 years, 10 months ago by Sidney San Martín Modified:
3 years, 9 months ago CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWork around an AppKit bug that can leave two windows in a key state.
BUG=692408
Patch Set 1 #
Total comments: 24
Patch Set 2 : Add rdar: link and a comment about WEAK_IMPORT_ATTRIBUTE. #Patch Set 3 : Remove a comment that I don't think I needed to add. #Patch Set 4 : Add a DCHECK to give us a heads up if the ivar changes or goes away. #Patch Set 5 : Use base::mac::ScopedObjCClassSwizzler. #
Total comments: 6
Patch Set 6 : Nits (#import->#include, variable naming, C++ cast) #
Total comments: 2
Messages
Total messages: 55 (37 generated)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Work around an AppKit bug that can leave two windows in a key state. BUG=692408 ========== to ========== Work around an AppKit bug that can leave two windows in a key state. BUG=692408 ==========
sdy@chromium.org changed reviewers: + shrike@chromium.org
Take a look when you have a minute. The main question is whether a case-by-case fix would be better than this "fix AppKit" approach.
sdy@chromium.org changed reviewers: + rsesek@chromium.org, tapted@chromium.org
+tapted +rsesek This patches the AppKit bug, so it should fix any menu that could trigger it.
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:8: // https://crbug.com/692408 Radar # too?
yah still no update on rdar://30572648 yet it all frightens me a little.. there are so many unknowns without seeing the code. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:41: WEAK_IMPORT_ATTRIBUTE Is this needed? I don't see it elsewhere in Chrome - comment? https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:56: if ([NSApp keyWindow] != rememberedKeyWindow) { 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 :/ https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:57: [rememberedKeyWindow resignKeyWindow]; 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? https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = 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.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:8: // https://crbug.com/692408 On 2017/02/28 16:24:24, Avi wrote: > Radar # too? Done. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:41: WEAK_IMPORT_ATTRIBUTE On 2017/02/28 23:24:13, tapted wrote: > Is this needed? I don't see it elsewhere in Chrome - comment? 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. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:56: if ([NSApp keyWindow] != rememberedKeyWindow) { On 2017/02/28 23:24:13, tapted wrote: > 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 :/ 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. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:57: [rememberedKeyWindow resignKeyWindow]; On 2017/02/28 23:24:13, tapted wrote: > 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? 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. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/02/28 23:24:13, tapted wrote: > 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. 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:53: object_getInstanceVariable(self, "_rememberedKeyWindow", What happens if `_rememberedKeyWindow` gets renamed? https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:57: [rememberedKeyWindow resignKeyWindow]; On 2017/03/01 00:50:20, sdy wrote: > On 2017/02/28 23:24:13, tapted wrote: > > 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? > > 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. 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 :) https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 00:50:20, sdy wrote: > On 2017/02/28 23:24:13, tapted wrote: > > 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. > > 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. 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. > - If _restorePreviousKeyWindowFromSavedProperties goes away, > class_getInstanceMethod does nothing and returns NULL. What does method_setImplementation do if its first argument is 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. 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).
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdy@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:53: object_getInstanceVariable(self, "_rememberedKeyWindow", On 2017/03/01 02:12:20, tapted wrote: > What happens if `_rememberedKeyWindow` gets renamed? In that case, object_getInstanceVariable() returns NULL and doesn't touch the out param, so rememberedKeyWindow will be nil. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:57: [rememberedKeyWindow resignKeyWindow]; On 2017/03/01 02:12:20, tapted wrote: > On 2017/03/01 00:50:20, sdy wrote: > > On 2017/02/28 23:24:13, tapted wrote: > > > 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? > > > > 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. > > 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). That's awesome! I just updated the radar with repro steps for system apps. > 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 :) 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. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 02:12:20, tapted wrote: > On 2017/03/01 00:50:20, sdy wrote: > > On 2017/02/28 23:24:13, tapted wrote: > > > 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. > > > > 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. > > 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. 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. > > - If _restorePreviousKeyWindowFromSavedProperties goes away, > > class_getInstanceMethod does nothing and returns NULL. > > What does method_setImplementation do if its first argument is null? It does nothing in that case. You can try it — just change _restorePreviousKeyWindowFromSavedProperties to something else here and in the @interface above. > > > > 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. > > 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). 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:53: object_getInstanceVariable(self, "_rememberedKeyWindow", On 2017/03/01 03:08:35, sdy wrote: > On 2017/03/01 02:12:20, tapted wrote: > > What happens if `_rememberedKeyWindow` gets renamed? > > In that case, object_getInstanceVariable() returns NULL and doesn't touch the > out param, so rememberedKeyWindow will be nil. 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 :) https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 03:08:35, sdy wrote: > On 2017/03/01 02:12:20, tapted wrote: > > On 2017/03/01 00:50:20, sdy wrote: > > > On 2017/02/28 23:24:13, tapted wrote: > > > > 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. > > > > > > 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. > > > > 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. > > 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. 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 > > > - If _restorePreviousKeyWindowFromSavedProperties goes away, > > > class_getInstanceMethod does nothing and returns NULL. > > > > What does method_setImplementation do if its first argument is null? > > It does nothing in that case. You can try it — just change > _restorePreviousKeyWindowFromSavedProperties to something else here and in the > @interface above. > > > > > > > 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. > > > > 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). > > 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. 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.
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 03:08:35, sdy wrote: > On 2017/03/01 02:12:20, tapted wrote: > > On 2017/03/01 00:50:20, sdy wrote: > > > - If _restorePreviousKeyWindowFromSavedProperties goes away, > > > class_getInstanceMethod does nothing and returns NULL. > > > > What does method_setImplementation do if its first argument is null? > > It does nothing in that case. You can try it — just change > _restorePreviousKeyWindowFromSavedProperties to something else here and in the > @interface above. 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.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:53: object_getInstanceVariable(self, "_rememberedKeyWindow", On 2017/03/01 07:56:02, tapted wrote: > On 2017/03/01 03:08:35, sdy wrote: > > On 2017/03/01 02:12:20, tapted wrote: > > > What happens if `_rememberedKeyWindow` gets renamed? > > > > In that case, object_getInstanceVariable() returns NULL and doesn't touch the > > out param, so rememberedKeyWindow will be nil. > > 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 :) OK, done. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 07:56:02, tapted wrote: > On 2017/03/01 03:08:35, sdy wrote: > > On 2017/03/01 02:12:20, tapted wrote: > > > On 2017/03/01 00:50:20, sdy wrote: > > > > On 2017/02/28 23:24:13, tapted wrote: > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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 > 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 ``` > > > > > - If _restorePreviousKeyWindowFromSavedProperties goes away, > > > > class_getInstanceMethod does nothing and returns NULL. > > > > > > What does method_setImplementation do if its first argument is null? > > > > It does nothing in that case. You can try it — just change > > _restorePreviousKeyWindowFromSavedProperties to something else here and in the > > @interface above. > > > > > > > > > > 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. > > > > > > 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). > > > > 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. > > 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. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 08:20:35, tapted wrote: > On 2017/03/01 03:08:35, sdy wrote: > > On 2017/03/01 02:12:20, tapted wrote: > > > On 2017/03/01 00:50:20, sdy wrote: > > > > - If _restorePreviousKeyWindowFromSavedProperties goes away, > > > > class_getInstanceMethod does nothing and returns NULL. > > > > > > What does method_setImplementation do if its first argument is null? > > > > It does nothing in that case. You can try it — just change > > _restorePreviousKeyWindowFromSavedProperties to something else here and in the > > @interface above. > > 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. 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. > 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. I guess? It seems like overkill to me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 17:42:59, sdy wrote: > On 2017/03/01 08:20:35, tapted wrote: > > On 2017/03/01 03:08:35, sdy wrote: > > > On 2017/03/01 02:12:20, tapted wrote: > > > > On 2017/03/01 00:50:20, sdy wrote: > > > > > - If _restorePreviousKeyWindowFromSavedProperties goes away, > > > > > class_getInstanceMethod does nothing and returns NULL. > > > > > > > > What does method_setImplementation do if its first argument is null? > > > > > > It does nothing in that case. You can try it — just change > > > _restorePreviousKeyWindowFromSavedProperties to something else here and in > the > > > @interface above. > > > > 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. > > 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 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. > > 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. > > I guess? It seems like overkill to me. 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. https://codereview.chromium.org/2710763003/diff/1/chrome/browser/mac/patch_ns... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:64: _restorePreviousKeyWindowFromSavedProperties = On 2017/03/01 17:42:59, sdy wrote: > On 2017/03/01 07:56:02, tapted wrote: > > On 2017/03/01 03:08:35, sdy wrote: > > > On 2017/03/01 02:12:20, tapted wrote: > > > > On 2017/03/01 00:50:20, sdy wrote: > > > > > On 2017/02/28 23:24:13, tapted wrote: > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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 > > > > 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 > ``` 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.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sdy@chromium.org
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sdy@chromium.org
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdy@chromium.org changed reviewers: - thakis@chromium.org
tapted@: OK, these are all good points. New patch set — thoughts?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but wait for rsesek to chime in (I'm still mildly spooked by the whole thing). And thanks a ton for digging into this. https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:7: #import "base/logging.h" nit: include https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:54: Class NSCarbonMenuWindowClass = NSClassFromString(@"NSCarbonMenuWindow"); nit: should start with lowercase (perhaps theNSCarbon..) https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:69: (void**)&rememberedKeyWindow); nit: static_cast<void**> ? We try to avoid c-style casts.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:7: #import "base/logging.h" On 2017/03/02 09:34:59, tapted wrote: > nit: include Done. https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:54: Class NSCarbonMenuWindowClass = NSClassFromString(@"NSCarbonMenuWindow"); On 2017/03/02 09:34:59, tapted wrote: > nit: should start with lowercase (perhaps theNSCarbon..) Done. I made 'ns' lowercase, which seems to match what we do in other places. https://codereview.chromium.org/2710763003/diff/120001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:69: (void**)&rememberedKeyWindow); On 2017/03/02 09:34:59, tapted wrote: > nit: static_cast<void**> ? We try to avoid c-style casts. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:42: // The app/wrench/hotdog menu uses custom views, which gives Chrome more This CL makes sense, but I'm a little concerned about patching this AppKit-wide. Did you look into using the NSMenuDelegate to do this instead, just for the wrench menu?
https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... chrome/browser/mac/patch_nsmenu_key_window_bug.mm:42: // The app/wrench/hotdog menu uses custom views, which gives Chrome more On 2017/03/06 20:57:38, Robert Sesek wrote: > This CL makes sense, but I'm a little concerned about patching this AppKit-wide. > Did you look into using the NSMenuDelegate to do this instead, just for the > wrench menu? The bug also affects AppKit-provided menus like the Help menu, so I didn't look into using NSMenuDelegate to fix it only for the wrench menu. I can if you'd like.
On 2017/03/06 21:38:49, sdy wrote: > https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... > File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): > > https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... > chrome/browser/mac/patch_nsmenu_key_window_bug.mm:42: // The app/wrench/hotdog > menu uses custom views, which gives Chrome more > On 2017/03/06 20:57:38, Robert Sesek wrote: > > This CL makes sense, but I'm a little concerned about patching this > AppKit-wide. > > Did you look into using the NSMenuDelegate to do this instead, just for the > > wrench menu? > > The bug also affects AppKit-provided menus like the Help menu, so I didn't look > into using NSMenuDelegate to fix it only for the wrench menu. I can if you'd > like. Yeah, I understand that it also affects the Help menu, but I think focusing on the wrench menu is probably fine and will account for most of the impact of fixing this. The internals of NSMenu are rather terrifying, which is why I'm a little hesitant about patching this. If a solution works without needing private API and patching, I think that'd be preferable.
On 2017/03/06 22:41:00, Robert Sesek wrote: > On 2017/03/06 21:38:49, sdy wrote: > > > https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... > > File chrome/browser/mac/patch_nsmenu_key_window_bug.mm (right): > > > > > https://codereview.chromium.org/2710763003/diff/140001/chrome/browser/mac/pat... > > chrome/browser/mac/patch_nsmenu_key_window_bug.mm:42: // The app/wrench/hotdog > > menu uses custom views, which gives Chrome more > > On 2017/03/06 20:57:38, Robert Sesek wrote: > > > This CL makes sense, but I'm a little concerned about patching this > > AppKit-wide. > > > Did you look into using the NSMenuDelegate to do this instead, just for the > > > wrench menu? > > > > The bug also affects AppKit-provided menus like the Help menu, so I didn't > look > > into using NSMenuDelegate to fix it only for the wrench menu. I can if you'd > > like. > > Yeah, I understand that it also affects the Help menu, but I think focusing on > the wrench menu is probably fine and will account for most of the impact of > fixing this. The internals of NSMenu are rather terrifying, which is why I'm a > little hesitant about patching this. If a solution works without needing private > API and patching, I think that'd be preferable. Looking at the notifications that are available, I'm wondering if this may be fixable globally using NSMenuDidBeginTrackingNotification and NSMenuDidEndTrackingNotification, but without the hooking. |
