|
|
DescriptionAdds code to isolate use-after-free in Views
We are still seeing a fair number of crahes in View::Blur() that is
called from FocusManager. These crashes indicate the View held by
FocusManager has been deleted. Widget forwards a number of calls to
the FocusManager when views are removed, so in theory we should never
end with the FocusManager holding a deleted view. None-the-less
crashes indicate otherwise.
This patch stores the stack when focused_view_ is set, and if we end
the view is deleted we extract the stack DumpWithoutCrashing()
BUG=687232
TEST=none
R=msw@chromium.org, vapier@chromium.org
Review-Url: https://codereview.chromium.org/2750633004
Cr-Commit-Position: refs/heads/master@{#456856}
Committed: https://chromium.googlesource.com/chromium/src/+/5edcc960338a8b074127eecfb3d09ba6ab7e7418
Patch Set 1 #
Total comments: 7
Patch Set 2 : feedback #
Total comments: 6
Patch Set 3 : feedback2 #Patch Set 4 : rename to observed_view #
Total comments: 3
Messages
Total messages: 27 (14 generated)
vapier: FocusManager::OnViewIsDeleting msw: the rest
The CQ bit was checked by sky@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...
lgtm with nits and qs https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... ui/views/focus/focus_manager.cc:34: bool did_log_focused_view = false; optional nit: flip to |should_log_focused_view|? https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... ui/views/focus/focus_manager.cc:604: stack_when_focused_view_set_->Addresses(&stack_size); q: Could you just store stack_when_focused_view_set_->ToString()? https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... ui/views/focus/focus_manager.cc:620: focused_view_ = nullptr; q: Will nulling out |focused_view_| possibly prevent some of the crashes that you're hoping to capture? https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manager.h File ui/views/focus/focus_manager.h (right): https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... ui/views/focus/focus_manager.h:79: namespace ui { nit: blank line above
The CQ bit was checked by sky@chromium.org to run a CQ dry run
https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... ui/views/focus/focus_manager.cc:34: bool did_log_focused_view = false; On 2017/03/13 18:22:16, msw wrote: > optional nit: flip to |should_log_focused_view|? Done. https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... ui/views/focus/focus_manager.cc:604: stack_when_focused_view_set_->Addresses(&stack_size); On 2017/03/13 18:22:16, msw wrote: > q: Could you just store stack_when_focused_view_set_->ToString()? Indeed. The ToString() representation takes up more space and I believe is less likely to make it into the mini-dump. The crash folks suggested this approach, which mirrors that of task_annotator. https://codereview.chromium.org/2750633004/diff/1/ui/views/focus/focus_manage... ui/views/focus/focus_manager.cc:620: focused_view_ = nullptr; On 2017/03/13 18:22:15, msw wrote: > q: Will nulling out |focused_view_| possibly prevent some of the crashes that > you're hoping to capture? Yes, that is intentional, especially since this only logs once.
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.
i'm not super familiar with Chromium internals https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:608: instruction_pointers_copy[0] = reinterpret_cast<const void*>(0x12345678); could use memset here to simplify w/out lost of info ? memset(&instruction_pointers_copy[0], 0xAB, sizeof(instruction_pointers_copy[0])); https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:610: reinterpret_cast<const void*>(0x12345678); if you just assign instruction_pointers_copy[0], don't need the cast logic https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:611: std::memcpy((instruction_pointers_copy + 1), instruction_pointers, i find &instruction_pointers_copy[1] more readable myself
https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:608: instruction_pointers_copy[0] = reinterpret_cast<const void*>(0x12345678); On 2017/03/13 22:06:09, vapier wrote: > could use memset here to simplify w/out lost of info ? > > memset(&instruction_pointers_copy[0], 0xAB, > sizeof(instruction_pointers_copy[0])); Done. https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:610: reinterpret_cast<const void*>(0x12345678); On 2017/03/13 22:06:09, vapier wrote: > if you just assign instruction_pointers_copy[0], don't need the cast logic Done. https://codereview.chromium.org/2750633004/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:611: std::memcpy((instruction_pointers_copy + 1), instruction_pointers, On 2017/03/13 22:06:09, vapier wrote: > i find &instruction_pointers_copy[1] more readable myself Done.
The CQ bit was checked by sky@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.
lgtm
The CQ bit was checked by sky@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, vapier@chromium.org Link to the patchset: https://codereview.chromium.org/2750633004/#ps60001 (title: "rename to observed_view")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm with a nit https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.h File ui/views/view_observer.h (right): https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.... ui/views/view_observer.h:37: virtual void OnViewIsDeleting(View* observed_view) {} nit: Make the override decl/def match.
https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.h File ui/views/view_observer.h (right): https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.... ui/views/view_observer.h:37: virtual void OnViewIsDeleting(View* observed_view) {} On 2017/03/14 20:37:55, msw wrote: > nit: Make the override decl/def match. I don't think the style guide requires that, and in the context of FocusManager using |observed_view| feels overly verbose.
still lgtm https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.h File ui/views/view_observer.h (right): https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.... ui/views/view_observer.h:37: virtual void OnViewIsDeleting(View* observed_view) {} On 2017/03/14 21:00:59, sky wrote: > On 2017/03/14 20:37:55, msw wrote: > > nit: Make the override decl/def match. > > I don't think the style guide requires that, fair enough. > and in the context of FocusManager > using |observed_view| feels overly verbose. I'd say the same for this ViewObserver interface signature. Still, it's just a nit, the CL is fine as-is.
On 2017/03/14 21:15:21, msw wrote: > still lgtm > > https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.h > File ui/views/view_observer.h (right): > > https://codereview.chromium.org/2750633004/diff/60001/ui/views/view_observer.... > ui/views/view_observer.h:37: virtual void OnViewIsDeleting(View* observed_view) > {} > On 2017/03/14 21:00:59, sky wrote: > > On 2017/03/14 20:37:55, msw wrote: > > > nit: Make the override decl/def match. > > > > I don't think the style guide requires that, > > fair enough. > > > and in the context of FocusManager > > using |observed_view| feels overly verbose. > > I'd say the same for this ViewObserver interface signature. > > Still, it's just a nit, the CL is fine as-is. I did the rename to match another patch I'm going to land shortly https://codereview.chromium.org/2745163003/ . The reason for going with observed_view is to make it clear what the View is. This is especially important for the functions that take multiple views, such as OnChildRemoved. I would rather be overly explicit in hopes of avoiding mistakes.
On 2017/03/14 21:20:43, sky wrote: > I did the rename to match another patch I'm going to land shortly > https://codereview.chromium.org/2745163003/ . The reason for going with > observed_view is to make it clear what the View is. This is especially important > for the functions that take multiple views, such as OnChildRemoved. I would > rather be overly explicit in hopes of avoiding mistakes. Ah, thanks for the additional context. Still lgtm
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489523785127710, "parent_rev": "d1df3ad6b707ae3bfff924a4674207e4772cd5fa", "commit_rev": "5edcc960338a8b074127eecfb3d09ba6ab7e7418"}
Message was sent while issue was closed.
Description was changed from ========== Adds code to isolate use-after-free in Views We are still seeing a fair number of crahes in View::Blur() that is called from FocusManager. These crashes indicate the View held by FocusManager has been deleted. Widget forwards a number of calls to the FocusManager when views are removed, so in theory we should never end with the FocusManager holding a deleted view. None-the-less crashes indicate otherwise. This patch stores the stack when focused_view_ is set, and if we end the view is deleted we extract the stack DumpWithoutCrashing() BUG=687232 TEST=none R=msw@chromium.org, vapier@chromium.org ========== to ========== Adds code to isolate use-after-free in Views We are still seeing a fair number of crahes in View::Blur() that is called from FocusManager. These crashes indicate the View held by FocusManager has been deleted. Widget forwards a number of calls to the FocusManager when views are removed, so in theory we should never end with the FocusManager holding a deleted view. None-the-less crashes indicate otherwise. This patch stores the stack when focused_view_ is set, and if we end the view is deleted we extract the stack DumpWithoutCrashing() BUG=687232 TEST=none R=msw@chromium.org, vapier@chromium.org Review-Url: https://codereview.chromium.org/2750633004 Cr-Commit-Position: refs/heads/master@{#456856} Committed: https://chromium.googlesource.com/chromium/src/+/5edcc960338a8b074127eecfb3d0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/5edcc960338a8b074127eecfb3d0... |