New issue
Advanced search Search tips

Issue 735603 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
EstimatedDays: ----
NextAction: ----
OS: Fuchsia
Pri: 3
Type: Bug

Blocking:
issue 706592



Sign in to add a comment

ObserverListThreadSafeTest.CrossThreadObserver failed flakily on Fuchsia

Project Member Reported by scottmg@chromium.org, Jun 21 2017

Issue description

https://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.
 
https://cs.chromium.org/chromium/src/base/observer_list_unittest.cc?rcl=596bb46345c755ac777efae231a56afa26121283&l=422 cross_thread_notifies isn't even referenced :/. So ObserverListThreadSafeTest.CrossThreadObserver and
ObserverListThreadSafeTest.CrossThreadNotifications are identical except for number of threads.
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.
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.
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.
Owner: scottmg@chromium.org
Status: Started (was: Untriaged)
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.
and it was leaking AddRemoveThread instances 'cuz why not
Oops, nope AddRemoveThread::ThreadMain calls delete this, so they weren't leaking. Crazy like a fox!
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment