New issue
Advanced search Search tips

Issue 715235 link

Starred by 1 user
Status: Assigned
Owner:
gab@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Fix thread unsafe usage of the RunLoop API

Project Member Reported by gab@chromium.org, Apr 25 2017 
Tried adding thread checks for documentation of RunLoop's API in https://codereview.chromium.org/2818533003/#ps160001 but they fire in existing codebase (which means some usage is incorrect and likely thread unsafe...).
Project Member

Comment 1 by bugdroid1@chromium.org, May 5 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7af9dc076d2349ade19a95e24f6ee6811e939765

commit 7af9dc076d2349ade19a95e24f6ee6811e939765
Author: gab <gab@chromium.org>
Date: Fri May 05 13:38:54 2017

Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

Lastly, tried to add thread safety checks for documentation purposes but turns
out there are already improper usage of the API... those will have to be
addressed first through issue 715235.

https://codereview.chromium.org/2828913003 follows to
s/nested message loop/nested run loop/ in comments.

BUG= 703346 , 715235

Review-Url: https://codereview.chromium.org/2818533003
Cr-Commit-Position: refs/heads/master@{#469636}

[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/base/message_loop/message_loop.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/base/message_loop/message_loop.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/base/message_loop/message_loop_test.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/base/message_loop/message_loop_test.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/base/run_loop.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/base/run_loop.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/base/run_loop_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/chrome/browser/app_controller_mac.mm
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/chrome/browser/metrics/chromeos_metrics_provider_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/chrome/browser/ui/views/simple_message_box_views.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/chromeos/dbus/services/service_provider_test_helper.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/components/omnibox/browser/autocomplete_provider_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/components/sync/engine_impl/sync_scheduler_impl_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/content/browser/browser_main_runner.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/content/browser/browser_thread_impl.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/device/bluetooth/bluez/bluetooth_adapter_profile_bluez_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/device/bluetooth/bluez/bluetooth_bluez_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/device/bluetooth/test/test_bluetooth_adapter_observer.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/extensions/browser/api/alarms/alarms_api_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/mash/quick_launch/quick_launch.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/mojo/public/cpp/bindings/connector.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/mojo/public/cpp/bindings/lib/connector.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/services/service_manager/tests/lifecycle/package.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/services/ui/service.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/storage/browser/fileapi/file_system_operation_impl_write_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/core/frame/PerformanceMonitor.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_delegate.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_delegate_for_test.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_delegate_for_test.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/task_time_observer.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_for_test.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_for_test.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/third_party/WebKit/Source/platform/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/ui/aura/mus/mus_mouse_location_updater.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/ui/aura/mus/mus_mouse_location_updater.h
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/7af9dc076d2349ade19a95e24f6ee6811e939765/ui/aura/window_event_dispatcher_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, May 18 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/980a5271bd212db2ad7a050c3d49628aed42deb1

commit 980a5271bd212db2ad7a050c3d49628aed42deb1
Author: gab <gab@chromium.org>
Date: Thu May 18 16:20:16 2017

Loosen thread-safety checks and update documentation on RunLoop.

RunLoop::Delegate now being the thread-affine part. RunLoop itself is now
merely thread-unsafe (->sequence checks).

And make it safe to access a RunLoop's state from another sequence while it's
being Run() as that access is technically "sequenced" (any access will rebind
the SequenceChecker to that sequence for the remainder of the run and should
still catch undesired concurrent accesses).

The lack of detach during Run() might also be the cause of issue 715235
(will try to remove TODOs after this lands).

BUG= 722537 , 715235

Review-Url: https://codereview.chromium.org/2886913003
Cr-Commit-Position: refs/heads/master@{#472835}

[modify] https://crrev.com/980a5271bd212db2ad7a050c3d49628aed42deb1/base/run_loop.cc
[modify] https://crrev.com/980a5271bd212db2ad7a050c3d49628aed42deb1/base/run_loop.h
►

Sign in to add a comment