Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1560183002:
Suppress AppKit error message about adding an unknown subview. (Closed)
DescriptionSuppress AppKit error message about adding an unknown subview.
After switching Chrome to the 10.10 SDK, the AppKit now complains at
start-up about an unknown subview being added to a window. This message
spams the console with a backtrace which also has the downside of taking
up to hundreds of milliseconds to generate (because of symbolication).
This change disables the backtrace-generation step by method swizzling.
Suppressing the backtrace dropped the time spent in
[FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to
4ms in a debug build on my Mac Pro.
BUG=520373
Committed: https://crrev.com/edd5522d32e30f222e8bc945d5a66a5db0b2a828
Cr-Commit-Position: refs/heads/master@{#367946}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Change swizzled method location, remove stderr shunting. #
Total comments: 6
Patch Set 3 : Fix style, limit swizzling to browser process. #
Total comments: 2
Patch Set 4 : Change swizzling to target callStackSymbols method #
Total comments: 3
Patch Set 5 : Fix nits. #Messages
Total messages: 29 (7 generated)
Description was changed from ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change silences the complaint by temporarily shunting stdErr to /dev/null, and prevents the symbolication step by method swizzling. Supressing the symbolication dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 8ms in a debug build on my Mac Pro. BUG=520373 ========== to ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change silences the complaint by temporarily shunting stdErr to /dev/null, and prevents the symbolication step by method swizzling. Supressing the symbolication dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 8ms in a debug build on my Mac Pro. BUG=520373 ==========
shrike@chromium.org changed reviewers: + erikchen@chromium.org, rsesek@chromium.org
Hello erikchen and rsesek - this is a proposed cl for suppressing the "unknown subview" error message that occurs at start-up. I got tired of seeing it yesterday and decided to see what it might take to make it go away. Part of this solution involves method swizzling - I'd just like to say that even though I've proposed using swizzling here and elsewhere recently I've never been big on swizzling. It just seems like given what erikchen has tried that there's no good solution otherwise. I'm also suppressing the other console output by shunting stderr to /dev/null - I don't know if that's frowned upon. If it's no good I could remove that part and keep the swizzling. Or, if the swizzling is distasteful I could remove that and keep the stderr shunting. Blah blah blah. PTAL
https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:56: // just add it to NSObject, which is an ancestor class to _NSCallStackArray. 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? https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:77: + (void)load { Instead of doing this in +load, can we do it right before it's actually necessary? https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:92: callStackSupressor.GetOriginalImplementation(); better to suppress the clang diagnostic error: http://useyourloaf.com/blog/disabling-clang-compiler-warnings.html https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:142: devNullFD = open("/dev/null", O_WRONLY); Can you use [NSFileHandle fileHandleWithNullDevice] instead? https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:158: close(stdErrorDescriptor); Is this call necessary? Doesn't the call to dup2 already do this? Potentially a race condition to close a newly created fd?
https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:142: devNullFD = open("/dev/null", O_WRONLY); On 2016/01/05 20:48:44, erikchen wrote: > Can you use [NSFileHandle fileHandleWithNullDevice] instead? nevermind. It's probably easier to just use what you have right now, especially since you're using other BSD syscalls. IWYU?
https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:56: // just add it to NSObject, which is an ancestor class to _NSCallStackArray. On 2016/01/05 20:48:44, erikchen wrote: > 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? Yeah, that should work - I will work on that. https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:77: + (void)load { On 2016/01/05 20:48:44, erikchen wrote: > Instead of doing this in +load, can we do it right before it's actually > necessary? 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. https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:92: callStackSupressor.GetOriginalImplementation(); On 2016/01/05 20:48:44, erikchen wrote: > better to suppress the clang diagnostic error: > > http://useyourloaf.com/blog/disabling-clang-compiler-warnings.html Great! I wasn't sure how to deal with this. https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:158: close(stdErrorDescriptor); On 2016/01/05 20:48:44, erikchen wrote: > Is this call necessary? Doesn't the call to dup2 already do this? Potentially a > race condition to close a newly created fd? 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?
https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:77: + (void)load { On 2016/01/05 21:27:37, shrike wrote: > On 2016/01/05 20:48:44, erikchen wrote: > > Instead of doing this in +load, can we do it right before it's actually > > necessary? > > 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. Hm. Fair enough. Let's keep it in +load then. https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:158: close(stdErrorDescriptor); On 2016/01/05 21:27:37, shrike wrote: > On 2016/01/05 20:48:44, erikchen wrote: > > Is this call necessary? Doesn't the call to dup2 already do this? Potentially > a > > race condition to close a newly created fd? > > 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? you're right. I confused stdErrorDescriptor with devNullFD
Have you looked at the solution WebKit used? https://bugs.webkit.org/show_bug.cgi?id=134813 Does that not work here? Also, I think just swizzling out the stack trace call is sufficient, since at some point we should probably resolve this warning for real. Because this isn't threadsafe at all, I'm not sure we want to be messing with the standard descriptors process-wide.
On 2016/01/05 21:49:34, Robert Sesek wrote: > Have you looked at the solution WebKit used? > https://bugs.webkit.org/show_bug.cgi?id=134813 Does that not work here? erikchen says here that their solution (calling _addKnownSubview:) results in a behavior change and that it's not clear what the repercussions are - https://code.google.com/p/chromium/issues/detail?id=520373#c3 > Also, I think just swizzling out the stack trace call is sufficient, since at > some point we should probably resolve this warning for real. Because this isn't > threadsafe at all, I'm not sure we want to be messing with the standard > descriptors process-wide. Makes sense - I will take that part out.
On 2016/01/05 21:49:34, Robert Sesek wrote: > Have you looked at the solution WebKit used? > https://bugs.webkit.org/show_bug.cgi?id=134813 Does that not work here? > > Also, I think just swizzling out the stack trace call is sufficient, since at > some point we should probably resolve this warning for real. Because this isn't > threadsafe at all, I'm not sure we want to be messing with the standard > descriptors process-wide. The WebKit solution doesn't work out of the box: https://code.google.com/p/chromium/issues/detail?id=520373#c3
PTAL https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/1/chrome/browser/ui/cocoa/ful... chrome/browser/ui/cocoa/full_size_content_window.mm:56: // just add it to NSObject, which is an ancestor class to _NSCallStackArray. On 2016/01/05 20:48:44, erikchen wrote: > 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. Turns out that doing this makes it a little tricker to call the original implementation, but I have made the change.
One last question: Why sizzle on _NSCallStackArray instead of +[NSThread callStackSymbols] (it should be possible to swizzle a class method…)? It looks like swizzling at the _NSCallStackArray method doesn't skip over the call to backtrace(), just the symbolization. Also, sorry for missing #c3 on the bug. https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:48: static BOOL disableSymbolication = NO; naming: g_disable_symbolication, same as below https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:80: + (void)load { Can you limit this to just the browser process, like here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:140: disableSymbolication = YES; You can use base::AutoReset<BOOL>
On 2016/01/05 22:17:00, Robert Sesek wrote: > One last question: Why sizzle on _NSCallStackArray instead of +[NSThread > callStackSymbols] (it should be possible to swizzle a class method…)? It looks > like swizzling at the _NSCallStackArray method doesn't skip over the call to > backtrace(), just the symbolization. I'm not really well versed in how the AppKit generates backtraces (e.g. if there's a single chokepoint through which all backtraces flow, etc.). tapted's screenshot showed _NSCallStackArray, so I knew that I could attack the problem there. I'm sure my approach still generates a backtrace, but I figured symbolication accounted for pretty much all of the time spent. Would you like me to change it to target [NSThread callStackSymbols]?
PTAL https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:48: static BOOL disableSymbolication = NO; On 2016/01/05 22:17:00, Robert Sesek wrote: > naming: g_disable_symbolication, same as below Thank you - I had looked for the naming convention for globals but couldn't find it. The IMP below it is also a static - I preprended "g_" but wasn't sure if I should do anything else to it. The g_ makes it not want to be camel case but it's an ObjectiveC variable type in an ObjectiveC chunk of code. Or should maybe it be gOriginalDescriptionWithLocaleImplementation? https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:80: + (void)load { On 2016/01/05 22:16:59, Robert Sesek wrote: > Can you limit this to just the browser process, like here: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Thank you. I wondered about this but didn't see how to tell so early on if the process was browser or renderer. https://codereview.chromium.org/1560183002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:140: disableSymbolication = YES; On 2016/01/05 22:16:59, Robert Sesek wrote: > You can use base::AutoReset<BOOL> Thank you again! I wondered if there was a mechanism like this to guard against forgetting to reset the bool.
lgtm https://codereview.chromium.org/1560183002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:52: static IMP g_originalDescriptionWithLocaleImplementation; I would probably use the same nomenclature as g_disable_symbolication, but rsesek's preferences take precedence.
On 2016/01/05 22:52:52, shrike wrote: > On 2016/01/05 22:17:00, Robert Sesek wrote: > > One last question: Why sizzle on _NSCallStackArray instead of +[NSThread > > callStackSymbols] (it should be possible to swizzle a class method…)? It looks > > like swizzling at the _NSCallStackArray method doesn't skip over the call to > > backtrace(), just the symbolization. > > I'm not really well versed in how the AppKit generates backtraces (e.g. if > there's a single chokepoint through which all backtraces flow, etc.). tapted's > screenshot showed _NSCallStackArray, so I knew that I could attack the problem > there. I'm sure my approach still generates a backtrace, but I figured > symbolication accounted for pretty much all of the time spent. Would you like me > to change it to target [NSThread callStackSymbols]? Yes, since +[NSThread callStackSymbols] is the public API, that's probably a better choice to swizzle. I'd maybe even have it return something like @[ @"+callStackSymbols disabled for performance reasons" ] to make it clear that an empty array is not a bug. https://codereview.chromium.org/1560183002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:52: static IMP g_originalDescriptionWithLocaleImplementation; On 2016/01/05 23:52:40, erikchen wrote: > I would probably use the same nomenclature as g_disable_symbolication, but > rsesek's preferences take precedence. Yes, full under_scores is the correct way to write this. We only use camelCase in actual ObjC methods.
OK, PTAL
LGTM https://codereview.chromium.org/1560183002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:62: return g_disable_call_stack_symbols ? This would probably read easier as if it weren't ternary. https://codereview.chromium.org/1560183002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:79: // _NSCallStackArray to dump the symbols to the console - by swizzling This needs a comment update now.
The CL description also needs to be updated.
Description was changed from ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change silences the complaint by temporarily shunting stdErr to /dev/null, and prevents the symbolication step by method swizzling. Supressing the symbolication dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 8ms in a debug build on my Mac Pro. BUG=520373 ========== to ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change disables the backtrace-generation step by method swizzling. Suppressing the backtrace dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 4ms in a debug build on my Mac Pro. BUG=520373 ==========
https://codereview.chromium.org/1560183002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/1560183002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:62: return g_disable_call_stack_symbols ? On 2016/01/06 19:36:29, Robert Sesek wrote: > This would probably read easier as if it weren't ternary. Please look at my new variable name - I assume it's along the lines of what you mean.
LGTM
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/1560183002/#ps80001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1560183002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1560183002/80001
Message was sent while issue was closed.
Description was changed from ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change disables the backtrace-generation step by method swizzling. Suppressing the backtrace dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 4ms in a debug build on my Mac Pro. BUG=520373 ========== to ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change disables the backtrace-generation step by method swizzling. Suppressing the backtrace dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 4ms in a debug build on my Mac Pro. BUG=520373 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change disables the backtrace-generation step by method swizzling. Suppressing the backtrace dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 4ms in a debug build on my Mac Pro. BUG=520373 ========== to ========== Suppress AppKit error message about adding an unknown subview. After switching Chrome to the 10.10 SDK, the AppKit now complains at start-up about an unknown subview being added to a window. This message spams the console with a backtrace which also has the downside of taking up to hundreds of milliseconds to generate (because of symbolication). This change disables the backtrace-generation step by method swizzling. Suppressing the backtrace dropped the time spent in [FramedBrowserWindow initWithContentRect:hasTabStrip:] from 126ms to 4ms in a debug build on my Mac Pro. BUG=520373 Committed: https://crrev.com/edd5522d32e30f222e8bc945d5a66a5db0b2a828 Cr-Commit-Position: refs/heads/master@{#367946} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/edd5522d32e30f222e8bc945d5a66a5db0b2a828 Cr-Commit-Position: refs/heads/master@{#367946} |
