ObserverListThreadSafeTest.CrossThreadObserver failed flakily on Fuchsia |
|||
Issue descriptionhttps://luci-milo.appspot.com/buildbot/tryserver.chromium.linux/fuchsia/22 https://chromium-review.googlesource.com/c/542945/ [00289.868] 02458.02711> 1 test crashed: [00289.868] 02458.02711> ObserverListThreadSafeTest.CrossThreadObserver (../../base/observer_list_unittest.cc:463) No other useful output, doesn't seem reproducible. And doesn't make me overly confident in the "ThreadSafe" part of the name.
,
Jun 21 2017
Also |c| and |d| are unused. https://cs.chromium.org/chromium/src/base/observer_list_unittest.cc?rcl=f89d3572342f8a1bad997cec5fbabcdfe33aa5ef&l=433 It looks like https://cs.chromium.org/chromium/src/base/observer_list_unittest.cc?rcl=596bb46345c755ac777efae231a56afa26121283&l=442 passes in |false| where it intended to pass in cross_thread_notifies. But that probably won't make this more stable (true would be the more complicated case) so I'll avoid that for now.
,
Jun 21 2017
Adding break; at the end of the loop here https://cs.chromium.org/chromium/src/base/observer_list_unittest.cc?rcl=f89d3572342f8a1bad997cec5fbabcdfe33aa5ef&l=455 (i.e. break right away) makes it normally crash on Linux. So that might be getting close to the problem.
,
Jun 21 2017
So! In this "ThreadSafe" <ahem> test there's a race between getting to ThreadMain where it creates and sets loop_ and Quit() getting called, because Quit() tries to access loop_->task_runner(), which can cause a crash depending on scheduling. Also, the access to loop_ is unguarded and on both the thread function and the main thread, so that's fairly sketchy.
,
Jun 21 2017
,
Jun 21 2017
The false instead of cross_thread_notifies was originally written like that in https://codereview.chromium.org/7353/ so it's never been testing what it meant to test.
,
Jun 22 2017
and it was leaking AddRemoveThread instances 'cuz why not
,
Jun 22 2017
Oops, nope AddRemoveThread::ThreadMain calls delete this, so they weren't leaking. Crazy like a fox!
,
Jun 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ff38a416a16591a0606b0eebf2daf96eaab24091 commit ff38a416a16591a0606b0eebf2daf96eaab24091 Author: Scott Graham <scottmg@chromium.org> Date: Thu Jun 22 18:59:11 2017 Fix various issues with ObserverListThreadSafeTest.CrossThread* In porting base to Fuchsia, ObserverListThreadSafeTest.CrossThreadObserver was sometimes crashing. This appears to be due to different thread scheduling, but was an all-platforms bug. Fix that by waiting for ThreadMains to start. Additionally, .CrossThreadNotifications wasn't working as intended because the "true" it passed into the utility function was ignored. (This has always been wrong: https://codereview.chromium.org/7353) Also, two Adder objects were unnecessarily created; remove them. Bug: 706592, 735603 Change-Id: I1be4a825d05b1a46cbf554b6cd3b4c91410d4b76 Reviewed-on: https://chromium-review.googlesource.com/544176 Commit-Queue: Scott Graham <scottmg@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#481615} [modify] https://crrev.com/ff38a416a16591a0606b0eebf2daf96eaab24091/base/observer_list_unittest.cc
,
Jun 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0b6056a479707179419d9aae51b6c1aeb6e3e302 commit 0b6056a479707179419d9aae51b6c1aeb6e3e302 Author: Scott Graham <scottmg@chromium.org> Date: Thu Jun 22 19:50:16 2017 Follow up to https://chromium-review.googlesource.com/c/544176/ Per https://chromium-review.googlesource.com/c/544176/6/base%252Fobserver_list_unittest.cc#444 Bug: 735603 Change-Id: I842500d93704793c0d805667a95f3100c17c75bc Reviewed-on: https://chromium-review.googlesource.com/544781 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Scott Graham <scottmg@chromium.org> Cr-Commit-Position: refs/heads/master@{#481634} [modify] https://crrev.com/0b6056a479707179419d9aae51b6c1aeb6e3e302/base/observer_list_unittest.cc
,
Jun 22 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by scottmg@chromium.org
, Jun 21 2017