[Media Router] Use DialMediaSinkService in MediaRouterMojoImpl

chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc

Index: chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
diff --git a/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc b/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
index 562dcd5b8a213b51bed1f6e38abd4a3ed215c299..6639b8cf88849655280e2d87dce085a861417c75 100644
--- a/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
+++ b/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
@@ -22,26 +22,24 @@ DialMediaSinkService::DialMediaSinkService(
     const OnSinksDiscoveredCallback& callback,
     net::URLRequestContextGetter* request_context)
     : MediaSinkService(callback), request_context_(request_context) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(request_context_);
 }
 
 DialMediaSinkService::~DialMediaSinkService() {}

[mark a. foltz, 2017/05/09 18:21:13]

There needs to be some guarantees that the following have or will happen when
this dtor is called:
- Objects created on the IO thread are destroyed.
- Listener/observer removed from DialRegistry on the IO thread (e.g. by calling
Stop()), otherwise the DialRegistry may use |this| after free.

The DeviceDescriptionService and Timer should either be deleted already, or have
a task posted to the IO thread to delete them, instead of allowing their dtors
to run on the UI thread (unless they are truly threadsafe or refcounted).

[zhaobin, 2017/05/10 18:17:34]

Marked DialMediaSinkService as content::BrowserThread::DeleteOnIOThread, it
should handle destroying DeviceDescriptionService and Timer on IO thread.

Will call Stop in dtor().

 
 void DialMediaSinkService::Start() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  dial_registry()->RegisterObserver(this);
-  dial_registry()->StartPeriodicDiscovery();
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&DialMediaSinkService::StartOnIOThread, this));
 }
 
 void DialMediaSinkService::Stop() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  dial_registry()->UnregisterObserver(this);
-}
-
-DialRegistry* DialMediaSinkService::dial_registry() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  return DialRegistry::GetInstance();
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&DialMediaSinkService::StopOnIOThread, this));
 }
 
 DeviceDescriptionService* DialMediaSinkService::GetDescriptionService() {
@@ -56,18 +54,67 @@ DeviceDescriptionService* DialMediaSinkService::GetDescriptionService() {
   return description_service_.get();
 }
 
+void DialMediaSinkService::SetDialRegistryForTest(DialRegistry* dial_registry) {
+  dial_registry_ = dial_registry;
+}
+
+void DialMediaSinkService::SetDescriptionServiceForTest(
+    std::unique_ptr<DeviceDescriptionService> description_service) {
+  description_service_ = std::move(description_service);
+}
+
+void DialMediaSinkService::SetTimerForTest(std::unique_ptr<base::Timer> timer) {
+  finish_timer_ = std::move(timer);
+}
+
+void DialMediaSinkService::StartOnIOThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  // Create a finish timer.
+  if (!finish_timer_)
+    finish_timer_.reset(new base::OneShotTimer());
+
+  if (!dial_registry_)
+    dial_registry_ = DialRegistry::GetInstance();
+
+  dial_registry_->RegisterObserver(this);
+  dial_registry_->OnListenerAdded();
+}
+
+void DialMediaSinkService::StopOnIOThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!dial_registry_)

[Kevin M, 2017/05/09 17:32:06]

Assuming the service was Start()ed, then is it possible for this to be null?

Same for the calls to DCHECK(finish_timer_) - those seem unnecessary too.

[zhaobin, 2017/05/10 18:17:34]

Will change this to DCHECK. DCHECK(xxx) should be ok...release code wont have
it, earlier DCHECK crash is easier to debug. We have a lot of DCHECk in
presentation API code to ensure pre-conditions.

https://cs.chromium.org/chromium/src/content/browser/presentation/presentatio...

+    return;
+
+  dial_registry_->OnListenerRemoved();
+  dial_registry_->UnregisterObserver(this);
+}
+
+void DialMediaSinkService::OnFetchCompletedOnUIThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (current_sinks_ == mrp_sinks_) {
+    DVLOG(2) << "No update to sink list.";
+    return;
+  }
+
+  auto sinks = current_sinks_;
+  DVLOG(2) << "Send sinks to media router, [size]: " << sinks.size();
+  sink_discovery_callback_.Run(
+      std::vector<MediaSinkInternal>(sinks.begin(), sinks.end()));
+  mrp_sinks_ = std::move(sinks);
+}
+
 void DialMediaSinkService::OnDialDeviceEvent(
     const DialRegistry::DeviceList& devices) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DVLOG(2) << "DialMediaSinkService::OnDialDeviceEvent found " << devices.size()
            << " devices";
 
-  // Add a finish timer.
-  finish_timer_.reset(new base::OneShotTimer());
+  DCHECK(finish_timer_);
   base::TimeDelta finish_delay =

[Kevin M, 2017/05/09 17:32:05]

Suggestion: refactor these calls into a method "ResetTimer()"?

[Kevin M, 2017/05/09 17:32:06]

This code resets the timer for every DIAL event, so FetchCompleted is called N
seconds after the *last* record.

OnDeviceDescriptionAvailable does not reset the timer once it's running, so
FetchCompleted is called N seconds after the *first* record.

Is this intentional?

[zhaobin, 2017/05/10 18:17:34]

Done.

[zhaobin, 2017/05/10 18:17:34]

Yes. DIAL event is not for the *last* record. It returns a device list, instead
of single device description.

DIAL event (return a list of devices)

OnDeviceDescriptionAvailable()
OnDeviceDescriptionAvailable()
OnDeviceDescriptionAvailable()
...
(N seconds)
FetchCompleted (timer set on DIAL event)

OnDeviceDescriptionAvailable() // did not come back in N seconds
...
N seconds
FetchCompleted (timer set on OnDeviceDescriptionAvailable())

       base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs);
-  finish_timer_->Start(FROM_HERE, finish_delay, this,
-                       &DialMediaSinkService::OnFetchCompleted);
+  finish_timer_->Start(FROM_HERE, finish_delay,
+                       base::Bind(&DialMediaSinkService::OnFetchCompleted,
+                                  base::Unretained(this)));
 
   current_sinks_.clear();

[mark a. foltz, 2017/05/09 18:21:13]

This is being called on the IO thread, but current_sinks_ was allocated on the
UI thread.

[zhaobin, 2017/05/10 18:17:34]

Created a delegate to handle thread hopping. DialMediaSinkService lives on IO
now :)

   current_devices_ = devices;
@@ -107,16 +154,17 @@ void DialMediaSinkService::OnDeviceDescriptionAvailable(
 
   current_sinks_.insert(MediaSinkInternal(sink, extra_data));

[mark a. foltz, 2017/05/09 18:21:13]

Ditto.  This class could be redesigned to pass these results to a task posted on
the UI thread that updates |current_sinks_|.

[zhaobin, 2017/05/10 18:17:34]

Done.

 
-  if (finish_timer_)
+  DCHECK(finish_timer_);
+  if (finish_timer_->IsRunning())
     return;
 
   // Start fetch timer again if device description comes back after
   // |finish_timer_| fires.
   base::TimeDelta finish_delay =
       base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs);
-  finish_timer_.reset(new base::OneShotTimer());
-  finish_timer_->Start(FROM_HERE, finish_delay, this,
-                       &DialMediaSinkService::OnFetchCompleted);
+  finish_timer_->Start(FROM_HERE, finish_delay,
+                       base::Bind(&DialMediaSinkService::OnFetchCompleted,
+                                  base::Unretained(this)));
 }
 
 void DialMediaSinkService::OnDeviceDescriptionError(
@@ -130,18 +178,9 @@ void DialMediaSinkService::OnFetchCompleted() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!sink_discovery_callback_.is_null());
 
-  finish_timer_.reset();
-
-  auto sinks = current_sinks_;
-  if (sinks == mrp_sinks_) {
-    DVLOG(2) << "No update to sink list.";
-    return;
-  }
-
-  DVLOG(2) << "Send sinks to media router, [size]: " << sinks.size();
-  sink_discovery_callback_.Run(
-      std::vector<MediaSinkInternal>(sinks.begin(), sinks.end()));
-  mrp_sinks_ = std::move(sinks);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&DialMediaSinkService::OnFetchCompletedOnUIThread, this));
 }
 
 }  // namespace media_router