Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
941543002:
Mac: Speculative fix for tracking area crashes (Closed)
DescriptionMac: Speculative fix for tracking area crashes
The fix for issue 176725 is suspected to introduce this crash.
Implement an alternative workaround that taps the mouse moved
events from the application's event stream.
BUG=315379
TEST=See http://crbug.com/176725
Committed: https://crrev.com/0fac57d7b3ebfcb25c15393ba49cc0d42eb1fc07
Cr-Commit-Position: refs/heads/master@{#318541}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use event tap #
Total comments: 6
Patch Set 3 : Fix for thakis #
Total comments: 2
Patch Set 4 : Use viewDidEndLiveResize #
Total comments: 2
Patch Set 5 : Fix for shess #Messages
Total messages: 34 (8 generated)
andresantoso@chromium.org changed reviewers: + thakis@chromium.org
Hey Nico, PTAL and let me know if you think this is the way to go or not. I considered intercepting mouseMoved in BrowserWindowController (it's in the responder chain) and forwarding it to RWHVC if necessary, but it got ugly and seemed fragile. I can't tell if the event came from firstResponder or from a tracking area, so I worry about double events or infinite recursion, and we would still be missing mouseEntered and mouseExited.
Nico, ping?
Nico, what do you think?
https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm File ui/base/cocoa/base_view.mm (left): https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm#o... ui/base/cocoa/base_view.mm:142: [super updateTrackingAreas]; Could we set a flag in dealloc and not add a new tracking area here if that flag is set? Could that fix the crash without requiring focus for the bug fix?
https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm File ui/base/cocoa/base_view.mm (left): https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm#o... ui/base/cocoa/base_view.mm:142: [super updateTrackingAreas]; On 2015/02/24 21:00:18, Nico wrote: > Could we set a flag in dealloc and not add a new tracking area here if that flag > is set? Could that fix the crash without requiring focus for the bug fix? I thought about that, but ended up not being confident that it will fix the crash. Looking at the zombie_dealloc bt from the original bug description, the crashing tracking area is the one that was deleted in -[BaseView dealloc]. I think that rules out my speculation in https://crbug.com/315379#c8, and more likely to be an AppKit bug somewhere.
andresantoso@chromium.org changed reviewers: + shess@chromium.org
+shess since he's also looking into this.
On 2015/02/24 21:20:46, Andre wrote: > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm > File ui/base/cocoa/base_view.mm (left): > > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm#o... > ui/base/cocoa/base_view.mm:142: [super updateTrackingAreas]; > On 2015/02/24 21:00:18, Nico wrote: > > Could we set a flag in dealloc and not add a new tracking area here if that > flag > > is set? Could that fix the crash without requiring focus for the bug fix? > > I thought about that, but ended up not being confident that it will fix the > crash. > Looking at the zombie_dealloc bt from the original bug description, the > crashing tracking area is the one that was deleted in -[BaseView dealloc]. > I think that rules out my speculation in https://crbug.com/315379#c8, and > more likely to be an AppKit bug somewhere. Hmm, good point, if it were generating a new tracking area under -dealloc, that area would be leaked and the zombie crash would probably be against the BaseView instance. I'm nervous about using a big stick like this to fix a problem which isn't well-understood in the first place. AFAICT there's no repro case at hand?
On 2015/02/25 23:30:54, Scott Hess wrote: > On 2015/02/24 21:20:46, Andre wrote: > > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm > > File ui/base/cocoa/base_view.mm (left): > > > > > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm#o... > > ui/base/cocoa/base_view.mm:142: [super updateTrackingAreas]; > > On 2015/02/24 21:00:18, Nico wrote: > > > Could we set a flag in dealloc and not add a new tracking area here if that > > flag > > > is set? Could that fix the crash without requiring focus for the bug fix? > > > > I thought about that, but ended up not being confident that it will fix the > > crash. > > Looking at the zombie_dealloc bt from the original bug description, the > > crashing tracking area is the one that was deleted in -[BaseView dealloc]. > > I think that rules out my speculation in https://crbug.com/315379#c8, and > > more likely to be an AppKit bug somewhere. > > Hmm, good point, if it were generating a new tracking area under -dealloc, that > area would be leaked and the zombie crash would probably be against the BaseView > instance. > > I'm nervous about using a big stick like this to fix a problem which isn't > well-understood in the first place. AFAICT there's no repro case at hand? Right, we don't have a repro case. The remaining missing events bug seems preferable to the crashes, assuming this does fix the crash. Safari suffers from the same problem too, as would any app that puts a tracking area near the bottom-left corner of the window.
On 2015/02/26 00:53:52, Andre wrote: > On 2015/02/25 23:30:54, Scott Hess wrote: > > On 2015/02/24 21:20:46, Andre wrote: > > > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm > > > File ui/base/cocoa/base_view.mm (left): > > > > > > > > > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm#o... > > > ui/base/cocoa/base_view.mm:142: [super updateTrackingAreas]; > > > On 2015/02/24 21:00:18, Nico wrote: > > > > Could we set a flag in dealloc and not add a new tracking area here if > that > > > flag > > > > is set? Could that fix the crash without requiring focus for the bug fix? > > > > > > I thought about that, but ended up not being confident that it will fix the > > > crash. > > > Looking at the zombie_dealloc bt from the original bug description, the > > > crashing tracking area is the one that was deleted in -[BaseView dealloc]. > > > I think that rules out my speculation in https://crbug.com/315379#c8, and > > > more likely to be an AppKit bug somewhere. > > > > Hmm, good point, if it were generating a new tracking area under -dealloc, > that > > area would be leaked and the zombie crash would probably be against the > BaseView > > instance. > > > > I'm nervous about using a big stick like this to fix a problem which isn't > > well-understood in the first place. AFAICT there's no repro case at hand? > > Right, we don't have a repro case. > The remaining missing events bug seems preferable to the crashes, assuming > this does fix the crash. Safari suffers from the same problem too, as would any > app that puts a tracking area near the bottom-left corner of the window. Do shess's suggestions for analyzing this more (on the bug) help? Is the crasher frequent? (The bug said something like "28 instances"?)
On 2015/02/26 04:11:37, Nico wrote: > Is the crasher frequent? (The bug said something like "28 instances"?) It's a top-10 browser-process crash, and I think it's pretty steady. I haven't analyzed if they're all this case or other CrTrackingArea zombies. I wonder if another "solution" would be to associate the proxy object with the window's lifetime. Doing so is a horrible horrible hack, but I've never had a story for why CrTrackingArea should even be necessary in the first place. I wonder if there could be anything happening which involves inspired manipulation of view<->window mappings, like perhaps shifting in or out of fullscreen.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
On 2015/02/26 04:11:37, Nico wrote: > On 2015/02/26 00:53:52, Andre wrote: > > On 2015/02/25 23:30:54, Scott Hess wrote: > > > On 2015/02/24 21:20:46, Andre wrote: > > > > > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm > > > > File ui/base/cocoa/base_view.mm (left): > > > > > > > > > > > > > > https://codereview.chromium.org/941543002/diff/1/ui/base/cocoa/base_view.mm#o... > > > > ui/base/cocoa/base_view.mm:142: [super updateTrackingAreas]; > > > > On 2015/02/24 21:00:18, Nico wrote: > > > > > Could we set a flag in dealloc and not add a new tracking area here if > > that > > > > flag > > > > > is set? Could that fix the crash without requiring focus for the bug > fix? > > > > > > > > I thought about that, but ended up not being confident that it will fix > the > > > > crash. > > > > Looking at the zombie_dealloc bt from the original bug description, the > > > > crashing tracking area is the one that was deleted in -[BaseView dealloc]. > > > > I think that rules out my speculation in https://crbug.com/315379#c8, and > > > > more likely to be an AppKit bug somewhere. > > > > > > Hmm, good point, if it were generating a new tracking area under -dealloc, > > that > > > area would be leaked and the zombie crash would probably be against the > > BaseView > > > instance. > > > > > > I'm nervous about using a big stick like this to fix a problem which isn't > > > well-understood in the first place. AFAICT there's no repro case at hand? > > > > Right, we don't have a repro case. > > The remaining missing events bug seems preferable to the crashes, assuming > > this does fix the crash. Safari suffers from the same problem too, as would > any > > app that puts a tracking area near the bottom-left corner of the window. > > Do shess's suggestions for analyzing this more (on the bug) help? > > Is the crasher frequent? (The bug said something like "28 instances"?) It's #5 of browser crashes (2.8% of browser crashes). https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%27%20AND%... The suggestion on the bug has to do with my original theory, which I think can be ruled out for now. We released the tracking area after removing it from the view, and it deallocs so there are no more retains on it. Somehow AppKit still has a weak reference to it, it seems. PTAL at patchset #2 for an alternative workaround. It's not pretty, but it seems to work well.
On 2015/02/26 21:20:29, Scott Hess wrote: > On 2015/02/26 04:11:37, Nico wrote: > > Is the crasher frequent? (The bug said something like "28 instances"?) > > It's a top-10 browser-process crash, and I think it's pretty steady. I haven't > analyzed if they're all this case or other CrTrackingArea zombies. > > I wonder if another "solution" would be to associate the proxy object with the > window's lifetime. Doing so is a horrible horrible hack, but I've never had a > story for why CrTrackingArea should even be necessary in the first place. > > I wonder if there could be anything happening which involves inspired > manipulation of view<->window mappings, like perhaps shifting in or out of > fullscreen. NSTrackingArea does not retain its owner, and it doesn't allow clearing the owner either. I think CrTrackingArea solved that problem, where the tracking area is still alive but its owner is already dead. This crash is different in that the tracking area itself is dead, but AppKit still has a weak reference to it somehow.
This seems ok to me. https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.h File ui/base/cocoa/base_view.h (right): https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.h:38: - (instancetype)initWithFrame:(NSRect)frame useEventTap:(BOOL)useEventTap; Should this be named by intent rather by workaround? ("needMouseMoveEvents:") https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.mm File ui/base/cocoa/base_view.mm (right): https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.mm:84: [[self window] setAcceptsMouseMovedEvents:YES]; Does this have to be called with NO at some point?
https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.h File ui/base/cocoa/base_view.h (right): https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.h:38: - (instancetype)initWithFrame:(NSRect)frame useEventTap:(BOOL)useEventTap; On 2015/02/26 22:16:07, Nico wrote: > Should this be named by intent rather by workaround? ("needMouseMoveEvents:") Mouse moved events are enabled either way, using a tracking area when NO, and using an event tap if YES. So calling it needMouseMoveEvents doesn't seem right. An option is to always use an event tap for mouse moved events, then won't need this parameter, but that might be too big/risky of a change since we only need this workaround when the view is positioned on the bottom left corner of the window. https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.mm File ui/base/cocoa/base_view.mm (right): https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.mm:84: [[self window] setAcceptsMouseMovedEvents:YES]; On 2015/02/26 22:16:07, Nico wrote: > Does this have to be called with NO at some point? We might if this view is removed from the window, but there likely are still views that wants this as YES, such as RWHVCocoa of another tab. Seems like it should be safe to leave it set, in practice a browser window will always have this set because it always has an active RWHVCocoa.
https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.h File ui/base/cocoa/base_view.h (right): https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.h:38: - (instancetype)initWithFrame:(NSRect)frame useEventTap:(BOOL)useEventTap; On 2015/02/26 22:29:58, Andre wrote: > On 2015/02/26 22:16:07, Nico wrote: > > Should this be named by intent rather by workaround? ("needMouseMoveEvents:") > > Mouse moved events are enabled either way, using a tracking area when NO, and > using an event > tap if YES. So calling it needMouseMoveEvents doesn't seem right. needReliableMouseMoveEvents? :-) > An option is to always use an event tap for mouse moved events, then won't need > this parameter, > but that might be too big/risky of a change since we only need this workaround > when the view > is positioned on the bottom left corner of the window.
https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.h File ui/base/cocoa/base_view.h (right): https://codereview.chromium.org/941543002/diff/60001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.h:38: - (instancetype)initWithFrame:(NSRect)frame useEventTap:(BOOL)useEventTap; On 2015/02/26 23:43:24, Nico wrote: > On 2015/02/26 22:29:58, Andre wrote: > > On 2015/02/26 22:16:07, Nico wrote: > > > Should this be named by intent rather by workaround? > ("needMouseMoveEvents:") > > > > Mouse moved events are enabled either way, using a tracking area when NO, and > > using an event > > tap if YES. So calling it needMouseMoveEvents doesn't seem right. > > needReliableMouseMoveEvents? :-) > > > An option is to always use an event tap for mouse moved events, then won't > need > > this parameter, > > but that might be too big/risky of a change since we only need this workaround > > when the view > > is positioned on the bottom left corner of the window. > Sounds good, went with wantsReliableMouseEvents. PTAL.
lgtm
Patchset #4 (id:100001) has been deleted
I'm still nervous about perturbing the event-distribution machinery to work around the fact that the window is retaining a reference to an object it most definitely should not have a reference to. That's why I asked about whether it would make sense to bind the tracking area lifetime to the window lifetime more directly. Right now we think that the problem is down to a bug in Cocoa, but we do some different things to our views, until we know what leads to the crash we can't say for sure it's not our fault. I'm not super enthusiastic about just tossing things at the problem without any attempt to quantify the circumstances it happens under. These various things we do to go around Cocoa add up over time to lots of cruft. https://codereview.chromium.org/941543002/diff/80001/ui/base/cocoa/base_view.mm File ui/base/cocoa/base_view.mm (right): https://codereview.chromium.org/941543002/diff/80001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.mm:35: - (void)initBaseViewUsingEventTap:(BOOL)wantsReliableMouseEvents { IMHO this should not be called -init*, since the caller is handling the -init chain. -setupEventTap: maybe? https://codereview.chromium.org/941543002/diff/80001/ui/base/cocoa/base_view.... ui/base/cocoa/base_view.mm:40: trackingOptions |= NSTrackingMouseMoved; If this approach is sufficiently performant to use for the web contents, why even bother having the fallback case?
I have another idea, I think it's safer and simpler. PTAL at patchset 4 and let me know what you think. Scott, I'm not sure what you mean by binding the tracking area lifetime to the window lifetime. Do you mean removing the tracking area from the view when the view is removed from a window? I think NSView does that for us, but maybe there's a bug there?
On 2015/02/27 18:48:05, Andre wrote: > I have another idea, I think it's safer and simpler. > PTAL at patchset 4 and let me know what you think. > > Scott, I'm not sure what you mean by binding the tracking area lifetime to the > window lifetime. > Do you mean removing the tracking area from the view when the view is removed > from a window? > I think NSView does that for us, but maybe there's a bug there? What we seem to be observing is that when you register a tracking area with the view it punts a reference up to the window where it is handled in event dispatch. When the view is removed from the window entirely, the tracking area still seems to be in the window in some cases. One suspects that it's an obscure edge case, because otherwise it would have been noticed and fixed long ago. So if we bound the proxy object's lifetime to the window's lifetime, then it would still exist after the view is gone, and would be marked inactive. [Proxy object being CrTrackingAreaOwnerProxy.] I wonder if what's happening is that the tracking area is being messed with during event tracking, so when the stack unwinds -sendEvent: is still working with stale data. The zombie code has limited space for zombie_dealloc_bt storage, it looks a lot like the main event loop, but I don't think there's enough data to distinguish that from a nested tracking loop. A nested tracking loop would be consistent with having this under the scope of -[NSWindow sendEvent:], and also with breaking expectations by using a local autorelease pool. If this hypothesis could be somehow proven, one option would be to post the proxy release to something out in the main event loop. I'll think on how one could prove this. Maybe we could show that -[BaseView dealloc] is happening during a -sendEvent: using [crapp handlingSendEvent]. It's possible that's the common case for tag or window close or something, though, in which case it would prove nothing.
LGTM, minor request below. I think that this is probably a better way than the original change even if it doesn't fix the crash, because it only needs to fire on the resize case and not all cases. https://codereview.chromium.org/941543002/diff/120001/ui/base/cocoa/base_view.mm File ui/base/cocoa/base_view.mm (right): https://codereview.chromium.org/941543002/diff/120001/ui/base/cocoa/base_view... ui/base/cocoa/base_view.mm:62: if ([self mouse:cornerPoint inRect:[self bounds]]) { I think it would make sense to just always bounce the tracking area at this point, rather than attempt to cover only the specific case. Special-casing results in a bit more code and allows for additional failure modes. It does make sense to retain a ref to the original bug, though, since this is non-obvious. Do you actually need to replace the tracking area, or is it sufficient to remove/add the same tracking area? The current code is probably sensible WRT sharing with -init*, just asking in terms of understanding the original resize bug.
lgtm
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/941543002/diff/120001/ui/base/cocoa/base_view.mm File ui/base/cocoa/base_view.mm (right): https://codereview.chromium.org/941543002/diff/120001/ui/base/cocoa/base_view... ui/base/cocoa/base_view.mm:62: if ([self mouse:cornerPoint inRect:[self bounds]]) { On 2015/02/27 20:47:33, Scott Hess wrote: > I think it would make sense to just always bounce the tracking area at this > point, rather than attempt to cover only the specific case. Special-casing > results in a bit more code and allows for additional failure modes. > > It does make sense to retain a ref to the original bug, though, since this is > non-obvious. > > Do you actually need to replace the tracking area, or is it sufficient to > remove/add the same tracking area? The current code is probably sensible WRT > sharing with -init*, just asking in terms of understanding the original resize > bug. Done. Unfortunately, just removing+adding the same tracking area does not work as a workaround.
The CQ bit was checked by andresantoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, shess@chromium.org Link to the patchset: https://codereview.chromium.org/941543002/#ps160001 (title: "Fix for shess")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/941543002/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0fac57d7b3ebfcb25c15393ba49cc0d42eb1fc07 Cr-Commit-Position: refs/heads/master@{#318541} |
