Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494433003:
Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost (Closed)
DescriptionFixes a crash when InkDropDelegate gets destroyed after its InkDropHost
When a CustomButton that has an InkDropDelegate installed on it is
deleted it needs to clear the InkDropDelegate first since the delegate
may need to call methods on its host (which is the CustomButton that is
being deleted).
BUG=563358
Committed: https://crrev.com/8dcd56e004a9d3dcd269aaf95ffbb70ea1fb5f20
Cr-Commit-Position: refs/heads/master@{#363848}
Patch Set 1 #Patch Set 2 : Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost (corrections and test) #
Total comments: 8
Patch Set 3 : Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost (comments) #
Total comments: 2
Patch Set 4 : Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost (using ViewHierarchyChanged) #Patch Set 5 : Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost (restore to PS3) #
Total comments: 5
Patch Set 6 : Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost (delegate owned by a leaf c… #
Messages
Total messages: 57 (20 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494433003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494433003/1
varkha@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@, can you please take a look? I have traces that prove both the crash explanation (interestingly enough it doesn't crash on Chrome OS even though it clearly goes through a bad code path that generates a virtual call on a destroyed pointer) and the effectiveness of the fix. Thanks!
On 2015/12/02 20:19:58, varkha wrote: > sadrul@, can you please take a look? I have traces that prove both the crash > explanation (interestingly enough it doesn't crash on Chrome OS even though it > clearly goes through a bad code path that generates a virtual call on a > destroyed pointer) and the effectiveness of the fix. > Thanks! Looks good. Can this have a test?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494433003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494433003/60001
sadrul@, this has proven to be a bit more involving since the first solution was leaky. Please see if the current solution is better. I have also added a test (which actually helped reveal the problem with the first try).
bruthig@chromium.org changed reviewers: + bruthig@chromium.org
Drive by https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate_unittest.cc (right): https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:15: // An InkDropDelegate that handles animations for toolbar buttons. Incorrect documentation. i.e. copy/pasted https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:27: EXPECT_FALSE(*button_deleted_); This should probably be an ASSERT_FALSE instead of an EXPECT_FALSE, no? https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:62: EXPECT_TRUE(*layer_added_); ASSERT instead of EXPECT https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:66: EXPECT_TRUE(*layer_removed_); ASSERT instead of EXPECT
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494433003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494433003/80001
Talked about ASSERTs vs. EXPECTs offline. The 2 setup classes are essential parts of the specific tests and it is hard otherwise to verify the sequence of calls. I prefer to leave those as expectations to allow easier investigation of sequencing failures. https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate_unittest.cc (right): https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:15: // An InkDropDelegate that handles animations for toolbar buttons. On 2015/12/03 04:11:35, bruthig wrote: > Incorrect documentation. i.e. copy/pasted Done. https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:27: EXPECT_FALSE(*button_deleted_); On 2015/12/03 04:11:35, bruthig wrote: > This should probably be an ASSERT_FALSE instead of an EXPECT_FALSE, no? Not sure, I wanted to allow the test to continue since a failure here doesn't break the test execution logic or crash it. Continuing could help a dev to understand why a breakage occurred. https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:62: EXPECT_TRUE(*layer_added_); On 2015/12/03 04:11:35, bruthig wrote: > ASSERT instead of EXPECT Acknowledged. https://codereview.chromium.org/1494433003/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:66: EXPECT_TRUE(*layer_removed_); On 2015/12/03 04:11:35, bruthig wrote: > ASSERT instead of EXPECT Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
sadrul@, ping?
https://codereview.chromium.org/1494433003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1494433003/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:61: DCHECK(!ink_drop_delegate_); It seems somewhat unfortunate to require the subclass to have to explicitly destroy this, when CustomButton is the owner. Would it be possible to reset state from, e.g. View::ViewHierarchyChanged() callback when the View is unparented instead ('reset state' is not necessarily destroy, though, since the button could be moved from one view to another, and in that case, just any in-progress animation should be canceled, without affecting ink-drop effects from future interaction)?
Is patch set #4 what you had in mind, sadrul@? https://codereview.chromium.org/1494433003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1494433003/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:61: DCHECK(!ink_drop_delegate_); On 2015/12/07 18:04:17, sadrul wrote: > It seems somewhat unfortunate to require the subclass to have to explicitly > destroy this, when CustomButton is the owner. Would it be possible to reset > state from, e.g. View::ViewHierarchyChanged() callback when the View is > unparented instead ('reset state' is not necessarily destroy, though, since the > button could be moved from one view to another, and in that case, just any > in-progress animation should be canceled, without affecting ink-drop effects > from future interaction)? Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494433003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494433003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks like hierarchy change notification is not guaranteed. Sadrul@, would you research that or return to PS3?
Hrm, OK, yeah, let's go back to PS3 then. PS3 lgtm
varkha@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdevlin.cronin for OWNERS in chrome/browser/ui/views/toolbar/toolbar_action_view.cc
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494433003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494433003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:112: SetInkDropDelegate(scoped_ptr<views::InkDropDelegate>()); Would it be better to move this to the CustomButton dtor so that subclasses don't have to worry about it?
https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:112: SetInkDropDelegate(scoped_ptr<views::InkDropDelegate>()); On 2015/12/08 16:56:37, Devlin wrote: > Would it be better to move this to the CustomButton dtor so that subclasses > don't have to worry about it? Would be nice but this is exactly what this CL is trying to fix. The concrete implementations of InkDropDelegate may need to call into the concrete View (such as this ToolbarActionView class) in their destructors so ToolbarActionView still needs to be around which is not the case by the time the base CustomButton::~CustomButton is getting called.
https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:112: SetInkDropDelegate(scoped_ptr<views::InkDropDelegate>()); On 2015/12/08 17:09:15, varkha wrote: > On 2015/12/08 16:56:37, Devlin wrote: > > Would it be better to move this to the CustomButton dtor so that subclasses > > don't have to worry about it? > > Would be nice but this is exactly what this CL is trying to fix. The concrete > implementations of InkDropDelegate may need to call into the concrete View (such > as this ToolbarActionView class) in their destructors so ToolbarActionView still > needs to be around which is not the case by the time the base > CustomButton::~CustomButton is getting called. Ah, gotcha. Well, that's disappointing. So, let's experiment with some C++ dirty details! We get a pure virtual when ~InkDropDelegate() is called in ~CustomButton. But if we were to have the InkDropDelegate owned by the ToolbarActionView, then it's called between ~ToolbarActionView and ~CustomButton - but I don't know when the vptr is moved. Can you quickly see what happens if we have the InkDropDelegate owned by ToolbarActionView, and CustomButton::SetInkDropDelegate take a InkDropDelegate*? (Assuming the unittest you add is right, you'll get a quick answer :))
PTAL, see if you prefer this version. https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:112: SetInkDropDelegate(scoped_ptr<views::InkDropDelegate>()); On 2015/12/08 17:33:39, Devlin wrote: > On 2015/12/08 17:09:15, varkha wrote: > > On 2015/12/08 16:56:37, Devlin wrote: > > > Would it be better to move this to the CustomButton dtor so that subclasses > > > don't have to worry about it? > > > > Would be nice but this is exactly what this CL is trying to fix. The concrete > > implementations of InkDropDelegate may need to call into the concrete View > (such > > as this ToolbarActionView class) in their destructors so ToolbarActionView > still > > needs to be around which is not the case by the time the base > > CustomButton::~CustomButton is getting called. > > Ah, gotcha. Well, that's disappointing. > > So, let's experiment with some C++ dirty details! > > We get a pure virtual when ~InkDropDelegate() is called in ~CustomButton. But > if we were to have the InkDropDelegate owned by the ToolbarActionView, then it's > called between ~ToolbarActionView and ~CustomButton - but I don't know when the > vptr is moved. Can you quickly see what happens if we have the InkDropDelegate > owned by ToolbarActionView, and CustomButton::SetInkDropDelegate take a > InkDropDelegate*? (Assuming the unittest you add is right, you'll get a quick > answer :)) Yeah, I've tried it before and initially rejected it as it required a little bit more of handholding through the destruction sequence. Please see if that is what you've meant and if you in fact like that better. I've restored to that version in the next patch set.
> We get a pure virtual when ~InkDropDelegate() is called in ~CustomButton. What actually happens is this: With the original (crashing) code we get a pure virtual call when a scoped ButtonInkDropDelegate instance is destroyed in scalar delete of CustomButton. This is inside a scalar delete of a ButtonInkDropDelegate in a place where it destroys its scoped ink_drop_animation_controller_ which in its destructor calls into a ConcreteCustomButton::RemoveInkDropLayer (and ConcreteCustomButton portion of CustomButton's vtable is by then destroyed).
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494433003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494433003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1494433003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:112: SetInkDropDelegate(scoped_ptr<views::InkDropDelegate>()); On 2015/12/08 19:00:50, varkha wrote: > On 2015/12/08 17:33:39, Devlin wrote: > > On 2015/12/08 17:09:15, varkha wrote: > > > On 2015/12/08 16:56:37, Devlin wrote: > > > > Would it be better to move this to the CustomButton dtor so that > subclasses > > > > don't have to worry about it? > > > > > > Would be nice but this is exactly what this CL is trying to fix. The > concrete > > > implementations of InkDropDelegate may need to call into the concrete View > > (such > > > as this ToolbarActionView class) in their destructors so ToolbarActionView > > still > > > needs to be around which is not the case by the time the base > > > CustomButton::~CustomButton is getting called. > > > > Ah, gotcha. Well, that's disappointing. > > > > So, let's experiment with some C++ dirty details! > > > > We get a pure virtual when ~InkDropDelegate() is called in ~CustomButton. But > > if we were to have the InkDropDelegate owned by the ToolbarActionView, then > it's > > called between ~ToolbarActionView and ~CustomButton - but I don't know when > the > > vptr is moved. Can you quickly see what happens if we have the > InkDropDelegate > > owned by ToolbarActionView, and CustomButton::SetInkDropDelegate take a > > InkDropDelegate*? (Assuming the unittest you add is right, you'll get a quick > > answer :)) > > Yeah, I've tried it before and initially rejected it as it required a little bit > more of handholding through the destruction sequence. Please see if that is what > you've meant and if you in fact like that better. I've restored to that version > in the next patch set. My hope was that if ToolbarActionView owns the InkDropDelegate, we don't need to explicitly reset() it in the destructor, since it will be destroyed *before* ~CustomButton. On 2015/12/08 19:09:26, varkha wrote: > > We get a pure virtual when ~InkDropDelegate() is called in ~CustomButton. > > What actually happens is this: > With the original (crashing) code we get a pure virtual > call when a scoped ButtonInkDropDelegate instance is > destroyed in scalar delete of CustomButton. This is inside > a scalar delete of a ButtonInkDropDelegate in a place where > it destroys its scoped ink_drop_animation_controller_ which > in its destructor calls into a ConcreteCustomButton::RemoveInkDropLayer > (and ConcreteCustomButton portion of CustomButton's vtable is by > then destroyed). Right - but if we have ToolbarActionView own it, it should be cleaned up prior to the ~CustomButton.
We don't technically need to call reset() in dtor but we still get a call into the class implementation of RemoveInkDropLayer AFTER its destructor has been called which I thought was not entirely kosher.
On 2015/12/08 19:55:41, varkha wrote: > We don't technically need to call reset() in dtor but we still get a call into > the class implementation of RemoveInkDropLayer AFTER its destructor has been > called which I thought was not entirely kosher. I would think that wherever the vptr is moved is defined somewhere deep in the C++ spec, and if it works, it's safe. But I could be wrong. Okay, I've caused a long enough bikeshed on this. If we're not 100% sure, it's probably best to do the more obvious thing, if only for readability. I might still have a slight preference for having the ToolbarActionView own the delegate, since it makes it more clear that it is responsible for the cleanup, but if you felt strongly that it makes it messier, either way is fine. LGTM.
> I might still have a slight preference for having the ToolbarActionView own the delegate... I think I agree with you. We will continue to search for some way to simplify this further.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1494433003/#ps140001 (title: "Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost (delegate owned by a leaf c…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494433003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494433003/140001
FTR I was also thinking of possibility of forcing dynamic allocation on
CustomButton, Button or View by making its dtor private and implementing a
"virtual void dispose() {delete this;}" which could be overridden in
CustomButton to destroy the animation delegate in the base class dtor before
doing "delete this". Something not unlike
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Requiring_or_Prohibiting_He....
This would be safe but would have a few things I think are problematic:
1. It prevents code like this:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view_unit...
which is widespread at least in tests.
2. Doing this for all Views seems intrusive, doing it just for CustomButtons
seems arbitrary.
3. Maybe too complicated for the case at hand.
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost When a CustomButton that has an InkDropDelegate installed on it is deleted it needs to clear the InkDropDelegate first since the delegate may need to call methods on its host (which is the CustomButton that is being deleted). BUG=563358 ========== to ========== Fixes a crash when InkDropDelegate gets destroyed after its InkDropHost When a CustomButton that has an InkDropDelegate installed on it is deleted it needs to clear the InkDropDelegate first since the delegate may need to call methods on its host (which is the CustomButton that is being deleted). BUG=563358 Committed: https://crrev.com/8dcd56e004a9d3dcd269aaf95ffbb70ea1fb5f20 Cr-Commit-Position: refs/heads/master@{#363848} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8dcd56e004a9d3dcd269aaf95ffbb70ea1fb5f20 Cr-Commit-Position: refs/heads/master@{#363848} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
