CastMediaSinkServiceimpl cache runs from the wrong thread |
||
Issue descriptionChrome Version: 64.0.3263 OS: All What steps will reproduce the problem? (1) Start chrome with DCHECK enabled on a network with at least one chromecast device, and with in-browser discovery enabled (--enable-features=MediaRouterUIRouteController,EnableCastDiscovery,EnableDialLocalDiscovery). (2) Change the set of networks the machine is connected to, including disconnecting from any (e.g. ip link set eth0 down) (3) Return to the same set of networks that were first connected (e.g. ip link set eth0 up). What is the expected result? chrome doesn't crash What happens instead? chrome crashes from a SequenceChecker DCHECK. This happens because CastMediaSinkServiceImpl calls DiscoveryNetworkMonitor::AddObserver in its constructor, which runs on the UI thread. DiscoveryNetworkMonitor calls its observers from whichever sequence they added themselves, so CastMediaSinkServiceImpl::OnNetworksChanged runs on the UI thread. This in turn calls CastMediaSinkServiceImpl::OpenChannels which is meant to run on the IO thread (or a blocking sequence).
,
Nov 13 2017
Issue 784314 has been merged into this issue.
,
Nov 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4935983c64668fb4a3d0e1eb8a4fcc4d3933113f commit 4935983c64668fb4a3d0e1eb8a4fcc4d3933113f Author: btolsch <btolsch@chromium.org> Date: Thu Nov 16 18:34:35 2017 Fix cache bugs in CastMediaSinkServiceImpl This change fixes two bugs with caching of media sinks and a potential initialization race that could also affect the cache's performance. The first fix is to allow network ID changes of network1->network2 to cache sinks for network1 rather than only caching for network1->disconnected/unknown. The cache originally used the latter logic because net::NetworkChangeNotifier broadcasts a disconnect between any two network changes. However, DiscoveryNetworkMonitor doesn't necessarily forward this behavior to its observers because it actually calculates the current network ID on every net::NetworkChangeNotifier notification. Not caching in the former case could account for some of the cache's low utilization currently seen in metrics. The second fix is to add CastMediaSinkServiceImpl as a DiscoveryNetworkMonitor::Observer from the correct sequence so it receives notifications on the correct sequence. This in turn exacerbates a race between the first network ID update and CastMediaSinkServiceImpl's AddObserver call. The race was therefore also addressed in this change. Bug: 782788 , 782859 Change-Id: I064bd184508e66f1806e55dc5afe1eea907836ea Reviewed-on: https://chromium-review.googlesource.com/760667 Commit-Queue: Brandon Tolsch <btolsch@chromium.org> Reviewed-by: Derek Cheng <imcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#517134} [modify] https://crrev.com/4935983c64668fb4a3d0e1eb8a4fcc4d3933113f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc [modify] https://crrev.com/4935983c64668fb4a3d0e1eb8a4fcc4d3933113f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h [modify] https://crrev.com/4935983c64668fb4a3d0e1eb8a4fcc4d3933113f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
,
Nov 16 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by btolsch@chromium.org
, Nov 8 2017