Suppress AppKit error message about adding an unknown subview.

chrome/browser/ui/cocoa/full_size_content_window.mm

Index: chrome/browser/ui/cocoa/full_size_content_window.mm
diff --git a/chrome/browser/ui/cocoa/full_size_content_window.mm b/chrome/browser/ui/cocoa/full_size_content_window.mm
index 555c523fa209983c74662b42b9cfb23c12e579ca..fc8ef09ff2f3841ccfb2415c6bef606fdede4f94 100644
--- a/chrome/browser/ui/cocoa/full_size_content_window.mm
+++ b/chrome/browser/ui/cocoa/full_size_content_window.mm
@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
+#include "base/mac/scoped_objc_class_swizzler.h"
 
 @interface FullSizeContentWindow ()
 
@@ -44,10 +45,55 @@
 
 @end
 
+static BOOL disableSymbolication = NO;
+
+@implementation NSObject (FullSizeContentWindowSwizzling)
+
+// This is the replacement method for
+// [_NSCallStackArray descriptionWithLocale:indent:] (see +load below).
+// _NSCallStackArray can't be linked against because it's a private class,
+// so it's not possible to add this method to the class as a category. Instead,
+// just add it to NSObject, which is an ancestor class to _NSCallStackArray.

[erikchen, 2016/01/05 20:48:44]

I'm not a huge fan of adding methods to NSObject, since that effectively exposes
this to all other code that uses the ObjC runtime. Imagine a hypothetical
situation where something is trying to override a different private class that
also has the method -descriptionWithLocale:indent:. They make a similarly named
category/method on NSObject, which results in undefined behavior.

http://stackoverflow.com/questions/15423677/categories-with-the-same-function...

Could you make a custom ObjC class, and use the constructor:
ScopedObjCClassSwizzler(Class target, Class source, SEL selector); 
to accomplish the same purpose?

[shrike, 2016/01/05 21:27:37]

Yeah, that should work - I will work on that.

[shrike, 2016/01/05 22:03:17]

Turns out that doing this makes it a little tricker to call the original
implementation, but I have made the change.

+- (NSString*)chromeSwizzledDescriptionWithLocale:(id)aLocale
+                                          indent:(unsigned long long)indent {
+  return disableSymbolication ?
+      @"" : [self chromeSwizzledDescriptionWithLocale:aLocale indent:indent];
+}
+
+@end
+
 @implementation FullSizeContentWindow
 
 #pragma mark - Lifecycle
 
+// In initWithContentRect:styleMask:backing:defer:, the call to
+// [NSView addSubview:positioned:relativeTo:] causes NSWindow to complain that
+// an unknown view is being added to it, and to generate a stack trace.
+// Not only does this stack trace pollute the console, it can also take hundreds
+// of milliseconds to generate (because of symbolication). The AppKit uses an
+// _NSCallStackArray to dump the symbols to the console - by swizzling
+// descriptionWithLocale:indent: we can disable this output. See
+// crbug.com/520373 .
++ (void)load {

[erikchen, 2016/01/05 20:48:44]

Instead of doing this in +load, can we do it right before it's actually
necessary?

[shrike, 2016/01/05 21:27:37]

Doing it that way would be simpler, and would allow me to ditch the
disableSymbolication static - I'm just not sure how kosher it is to fiddle with
the runtime like that on the fly. http://nshipster.com/method-swizzling/
suggests that you only want to do it once and early. But if it's OK to do it on
the fly I am happy to make that change.

[erikchen, 2016/01/05 21:33:28]

Hm. Fair enough. Let's keep it in +load then.

+  static dispatch_once_t onceToken;
+  dispatch_once(&onceToken, ^{
+    Class callStackArrayClass = NSClassFromString(@"_NSCallStackArray");
+    SEL originalSelector = @selector(descriptionWithLocale:indent:);
+    SEL swizzledSelector =
+        @selector(chromeSwizzledDescriptionWithLocale:indent:);
+
+    if (callStackArrayClass && originalSelector && swizzledSelector) {
+      CR_DEFINE_STATIC_LOCAL(base::mac::ScopedObjCClassSwizzler,
+                             callStackSupressor, (callStackArrayClass,
+                             originalSelector, swizzledSelector));
+
+      // Prevent compiler from complaining about callStackSupressor never being
+      // used.
+      callStackSupressor.GetOriginalImplementation();

[erikchen, 2016/01/05 20:48:44]

better to suppress the clang diagnostic error:

http://useyourloaf.com/blog/disabling-clang-compiler-warnings.html

[shrike, 2016/01/05 21:27:37]

Great! I wasn't sure how to deal with this.

+    }
+  });
+}
+
 - (instancetype)init {
   NOTREACHED();
   return nil;
@@ -87,9 +133,29 @@
       // it is positioned below the buttons.
       NSView* superview = [chromeWindowView_ superview];
       [chromeWindowView_ removeFromSuperview];
+
+      // Prevent the AppKit from sending a complaint to the console in
+      // response to our upcoming call to addSubview:positioned:relativeTo: by
+      // temporarily routing stdErr to /dev/null.
+      static int devNullFD = -1;
+      if (devNullFD == -1) {
+        devNullFD = open("/dev/null", O_WRONLY);

[erikchen, 2016/01/05 20:48:44]

Can you use [NSFileHandle fileHandleWithNullDevice] instead?

[erikchen, 2016/01/05 20:53:43]

nevermind. It's probably easier to just use what you have right now, especially
since you're using other BSD syscalls. IWYU?

+      }
+      int stdErrorDescriptor = dup(2);
+      dup2(devNullFD, 2);
+
+      // The AppKit's complaint also contains a backtrace, which can take
+      // significant time to symbolicate. See +load for more info.
+      disableSymbolication = YES;
+
       [superview addSubview:chromeWindowView_
                  positioned:NSWindowBelow
                  relativeTo:nil];
+
+      // Clean up from the AppKit console message workaround.
+      disableSymbolication = NO;
+      dup2(stdErrorDescriptor, 2);
+      close(stdErrorDescriptor);

[erikchen, 2016/01/05 20:48:44]

Is this call necessary? Doesn't the call to dup2 already do this? Potentially a
race condition to close a newly created fd?

[shrike, 2016/01/05 21:27:37]

The man page says, "dup2() makes newfd be the copy of oldfd, closing newfd first
if necessary". So I believe that means it closes 2, and then copies
stdErrorDescriptor into 2? 

[erikchen, 2016/01/05 21:33:28]

you're right. I confused stdErrorDescriptor with devNullFD

     }
   }
   return self;