Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
169443005:
Fix crash which occurs when a widget destroys itself as a result of ET_GESTURE_TAP_DOWN (Closed)
|
Created:
6 years, 10 months ago by pkotwicz Modified:
6 years, 9 months ago Reviewers:
sadrul CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org, tdanderson Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix crash which occurs when a widget destroys itself as a result of ET_GESTURE_TAP_DOWN
BUG=342867
TEST=WidgetTest.WidgetDeleted_InDispatchGestureEvent
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252214
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Total comments: 1
Messages
Total messages: 20 (0 generated)
Sadrul can you please look at the changes in ui/views
I haven't looked at the ash changes. It would be better to have someone more familiar with the code to take a look. https://codereview.chromium.org/169443005/diff/30001/ui/views/widget/root_vie... File ui/views/widget/root_view.cc (right): https://codereview.chromium.org/169443005/diff/30001/ui/views/widget/root_vie... ui/views/widget/root_view.cc:230: DispatchEventToTarget(handler, &handler_event); This is also an issue for other kinds of event dispatch, right? The better fix would be to have DispatchEventToTarget return the EventDispatchDetails, and look at dispatcher_destroyed
Sadrul, can you please take a look? I changed the signature of DispatchEventToTarget() as you suggested. I will get skuhne@ to take a look at the ash changes (again)
I looked at the ash changes this morning and had a very definitive deja vu feeling. What happened to that earlier CL?
The ash changes are actually the exact same as the previous CL. The previous CL was reverted because the test was failing on the Asan bot (hence the new code in root_view.*
Sadrul, ping?
You should separate out the ash changes in a separate CL, and reland that after the views change lands (since they are unrelated, other than the bug in views being exposed by the ash change). https://codereview.chromium.org/169443005/diff/130001/ui/views/widget/root_vi... File ui/views/widget/root_view.cc (right): https://codereview.chromium.org/169443005/diff/130001/ui/views/widget/root_vi... ui/views/widget/root_view.cc:232: } The nested scope isn't necessary, is it? https://codereview.chromium.org/169443005/diff/130001/ui/views/widget/root_vi... File ui/views/widget/root_view.h (right): https://codereview.chromium.org/169443005/diff/130001/ui/views/widget/root_vi... ui/views/widget/root_view.h:149: ui::Event* event); You should add WARN_UNUSED_RESULT here.
This CL only the views changes no. Sadrul, can you please take another look? https://codereview.chromium.org/169443005/diff/130001/ui/views/widget/root_vi... File ui/views/widget/root_view.cc (right): https://codereview.chromium.org/169443005/diff/130001/ui/views/widget/root_vi... ui/views/widget/root_view.cc:232: } Removed! https://codereview.chromium.org/169443005/diff/210001/ui/views/widget/root_vi... File ui/views/widget/root_view.cc (right): https://codereview.chromium.org/169443005/diff/210001/ui/views/widget/root_vi... ui/views/widget/root_view.cc:516: DispatchEventToTarget(mouse_pressed_handler, &mouse_released); I did not make DispatchEventToTarget() have WARN_UNUSED_RESULT because we do not want to use the result here.
Thanks for splitting up the change. https://codereview.chromium.org/169443005/diff/210001/ui/views/widget/root_vi... File ui/views/widget/root_view.cc (right): https://codereview.chromium.org/169443005/diff/210001/ui/views/widget/root_vi... ui/views/widget/root_view.cc:516: DispatchEventToTarget(mouse_pressed_handler, &mouse_released); On 2014/02/19 20:14:13, pkotwicz wrote: > I did not make DispatchEventToTarget() have WARN_UNUSED_RESULT because we do not > want to use the result here. > We should use the result here (like we do in RootWindow::ProcessedTouchEvent, for example)
Done. Sadrul, can you please take another look?
LGTM. Thanks! https://codereview.chromium.org/169443005/diff/280002/ui/views/widget/root_vi... File ui/views/widget/root_view.cc (right): https://codereview.chromium.org/169443005/diff/280002/ui/views/widget/root_vi... ui/views/widget/root_view.cc:144: if (dispatch_details.dispatcher_destroyed) target_destroyed? https://codereview.chromium.org/169443005/diff/280002/ui/views/widget/root_vi... ui/views/widget/root_view.cc:642: if (dispatch_details.dispatcher_destroyed) target_destroyed? https://codereview.chromium.org/169443005/diff/280002/ui/views/widget/root_vi... ui/views/widget/root_view.cc:768: if (dispatch_details.dispatcher_destroyed) check target_destroyed here too?
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/169443005/340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/169443005/340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/169443005/340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/169443005/340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/169443005/340001
Message was sent while issue was closed.
Change committed as 252214
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/173863002/ by dslomov@chromium.org. The reason for reverting is: Breaks ChromiumOS ASAN: http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... READ of size 8 at 0x614000007b08 thread T0 #0 0xf333f9 in handled ui/views/view.h:197 #1 0xf333f9 in views::internal::RootView::DispatchKeyEventStartAt(views::View*, ui::KeyEvent*) ui/views/widget/root_view.cc:781 #2 0x9f0363 in DispatchKeyEventStartAt ui/views/widget/root_view_test_helper.h:21 #3 0x9f0363 in views::test::RootViewTest_DeleteViewDuringKeyEventDispatch_Test::TestBody() ui/views/widget/root_view_unittest.cc:50 #4 0xbbd52a in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #5 0xbbd52a in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #6 0xbbf039 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #7 0xbbfdb3 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #8 0xbd0a0a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #9 0xbd0010 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #10 0xbd0010 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #11 0xb88c64 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #12 0xb88c64 in base::TestSuite::Run() base/test/test_suite.cc:213 #13 0xb7f077 in Run base/callback.h:401 #14 0xb7f077 in base::(anonymous namespace)::LaunchUnitTestsInternal(int, char**, base::Callback\u003Cint ()> const&, int) base/test/launcher/unit_test_launcher.cc:494 #15 0x8a931a in main ui/views/run_all_unittests.cc:44 #16 0x7f8a1f39176c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #17 0x49751c in _start (/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Tests__1_/build/src/out/Release/views_unittests+0x49751c) 0x614000007b08 is located 200 bytes inside of 408-byte region [0x614000007a40,0x614000007bd8) freed by thread T0 here: #0 0x480611 in operator delete(void*) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:85 #1 0x9f0a2b in views::test::DeleteOnKeyEventView::OnKeyPressed(ui::KeyEvent const&) ui/views/widget/root_view_unittest.cc:20 #2 0xf23b53 in OnKeyEvent ui/views/view.cc:1038 #3 0xf23b53 in non-virtual thunk to views::View::OnKeyEvent(ui::KeyEvent*) ui/views/view.cc:1041 #4 0xd2d3ae in ui::EventTarget::OnEvent(ui::Event*) ui/events/event_target.cc:64 #5 0xd29b0c in DispatchEvent ui/events/event_dispatcher.cc:185 #6 0xd29b0c in ui::EventDispatcher::ProcessEvent(ui::EventTarget*, ui::Event*) ui/events/event_dispatcher.cc:126 #7 0xd29551 in DispatchEventToTarget ui/events/event_dispatcher.cc:78 #8 0xd29551 in ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*, ui::Event*) ui/events/event_dispatcher.cc:55 #9 0xf33388 in DispatchEventToTarget ui/views/widget/root_view.cc:751 #10 0xf33388 in views::internal::RootView::DispatchKeyEventStartAt(views::View*, ui::KeyEvent*) ui/views/widget/root_view.cc:783 #11 0x9f0363 in DispatchKeyEventStartAt ui/views/widget/root_view_test_helper.h:21 #12 0x9f0363 in views::test::RootViewTest_DeleteViewDuringKeyEventDispatch_Test::TestBody() ui/views/widget/root_view_unittest.cc:50 #13 0xbbd52a in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #14 0xbbd52a in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #15 0xbbf039 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #16 0xbbfdb3 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #17 0xbd0a0a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #18 0xbd0010 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #19 0xbd0010 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #20 0xb88c64 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #21 0xb88c64 in base::TestSuite::Run() base/test/test_suite.cc:213 #22 0xb7f077 in Run base/callback.h:401 #23 0xb7f077 in base::(anonymous namespace)::LaunchUnitTestsInternal(int, char**, base::Callback\u003Cint ()> const&, int) base/test/launcher/unit_test_launcher.cc:494 #24 0x8a931a in main ui/views/run_all_unittests.cc:44 #25 0x7f8a1f39176c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 previously allocated by thread T0 here: #0 0x4801d1 in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:54 #1 0x9f0269 in views::test::RootViewTest_DeleteViewDuringKeyEventDispatch_Test::TestBody() ui/views/widget/root_view_unittest.cc:44 #2 0xbbd52a in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #3 0xbbd52a in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #4 0xbbf039 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #5 0xbbfdb3 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #6 0xbd0a0a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #7 0xbd0010 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #8 0xbd0010 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #9 0xb88c64 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #10 0xb88c64 in base::TestSuite::Run() base/test/test_suite.cc:213 #11 0xb7f077 in Run base/callback.h:401 #12 0xb7f077 in base::(anonymous namespace)::LaunchUnitTestsInternal(int, char**, base::Callback\u003Cint ()> const&, int) base/test/launcher/unit_test_launcher.cc:494 #13 0x8a931a in main ui/views/run_all_unittests.cc:44 #14 0x7f8a1f39176c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 .
Message was sent while issue was closed.
+tdanderson@ FYI Looks like |target_destroyed| isn't set correctly when the target View gets destroyed during dispatch in views. https://codereview.chromium.org/169443005/diff/340001/ui/views/widget/root_vi... File ui/views/widget/root_view.cc (right): https://codereview.chromium.org/169443005/diff/340001/ui/views/widget/root_vi... ui/views/widget/root_view.cc:784: if (dispatch_details.dispatcher_destroyed || Perhaps we are not setting |target_destroyed| correctly on dispatch_details. So it would be better to check event->handled() in this if too, before the code executes view = view->parent() |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
