[Media Router] Conditionally enable mDNS on Windows.

chrome/browser/media/router/media_router_mojo_impl.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/router/media_router_mojo_impl.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/files/file_path.h"
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
+#include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
+#include "build/build_config.h"
 #include "chrome/browser/media/router/issues_observer.h"
 #include "chrome/browser/media/router/media_router_factory.h"
 #include "chrome/browser/media/router/media_router_metrics.h"
 #include "chrome/browser/media/router/media_router_type_converters.h"
 #include "chrome/browser/media/router/media_routes_observer.h"
 #include "chrome/browser/media/router/media_sinks_observer.h"
 #include "chrome/browser/media/router/presentation_session_messages_observer.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
+#include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/firewall_manager_win.h"
 #include "extensions/browser/process_manager.h"
 
+

[apacible, 2016/03/23 22:04:45]

nit: remove newline.

[btolsch, 2016/03/24 01:44:03]

Done.

 #define DVLOG_WITH_INSTANCE(level) \
   DVLOG(level) << "MR #" << instance_id_ << ": "
 
 #define DLOG_WITH_INSTANCE(level) DLOG(level) << "MR #" << instance_id_ << ": "
 
 namespace media_router {
 
 using SinkAvailability = interfaces::MediaRouter::SinkAvailability;
 
 namespace {
@@ skipping 37 matching lines @@
 MediaRouterMojoImpl::MediaSinksQuery::~MediaSinksQuery() = default;
 
 MediaRouterMojoImpl::MediaRouterMojoImpl(
     extensions::EventPageTracker* event_page_tracker)
     : event_page_tracker_(event_page_tracker),
       instance_id_(base::GenerateGUID()),
       availability_(interfaces::MediaRouter::SinkAvailability::UNAVAILABLE),
       current_wake_reason_(MediaRouteProviderWakeReason::TOTAL_COUNT),
       weak_factory_(this) {
   DCHECK(event_page_tracker_);
+#if defined(OS_WIN)

[mark a. foltz, 2016/03/24 00:04:41]

Please put this into its own function in an anonymous namespace, i.e.
CanFirewallUseLocalPorts()

Also, I would not be opposed to putting it into its own file as well to keep the
include list for this file smaller (maybe in media_route_provider_util.cc).

[btolsch, 2016/03/24 01:44:03]

Done.

+  base::FilePath exe_path;

[imcheng, 2016/03/24 01:17:19]

Ideally this check should happen in FILE thread, see:
https://bugs.chromium.org/p/chromium/issues/detail?id=593167#c68

That would mean is_mdns_discovery_enabled_ won't be set to true on construction
time, but I think that is fine.

[btolsch, 2016/03/24 01:44:03]

How do I run this from a file thread?

[imcheng, 2016/03/24 02:29:31]

You can use BrowserThread::PostTask, see example here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Also, once you find out that CanUseLocalPorts is true, you should post a task to
enqueue a request to enable mdns on the extension on the UI thread.

[btolsch, 2016/03/24 21:32:26]

Done but currently this is making tests on Windows fail, possibly because it is
using BrowserThread::PostTask without some sort of required initialization.
Specifically ThreadTaskRunnerHandle doesn't seem to ever be initialized, but I'm
not sure how to go about fixing this.

+  if (base::PathService::Get(base::FILE_EXE, &exe_path)) {
+    auto firewall_manager = installer::FirewallManager::Create(
+        BrowserDistribution::GetDistribution(), exe_path);
+    if (firewall_manager) {
+      is_mdns_discovery_enabled_ = firewall_manager->CanUseLocalPorts();
+    }
+  }
+#endif

[mark a. foltz, 2016/03/24 00:04:41]

What if !defined(OS_WIN)?  Should we set the flag to true so we can ensure mDNS
is enabled on startup?

[btolsch, 2016/03/24 01:44:03]

If !defined(OS_WIN) then the extension will already have enabled mDNS.

 }
 
 MediaRouterMojoImpl::~MediaRouterMojoImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 // static
 void MediaRouterMojoImpl::BindToRequest(
     const extensions::Extension* extension,
     content::BrowserContext* context,
@@ skipping 56 matching lines @@
     AttemptWakeEventPage();
     return;
   }
 
   media_route_provider_ = std::move(media_route_provider_ptr);
   media_route_provider_.set_connection_error_handler(base::Bind(
       &MediaRouterMojoImpl::OnConnectionError, base::Unretained(this)));
   callback.Run(instance_id_);
   ExecutePendingRequests();
   wakeup_attempt_count_ = 0;
+  if (is_mdns_discovery_enabled_) {

[mark a. foltz, 2016/03/24 00:04:41]

Did you mean !is_mdns_discovery_enabled_?

Nit: Remove { }

[imcheng, 2016/03/24 01:17:19]

I think this is correct. |is_mdns_discovery_enabled_| really means
|should_enable_mdns_discovery_|. Perhaps it should be renamed as such.

[btolsch, 2016/03/24 01:44:03]

I think this (and the renaming) would be correct if the extension weren't saving
its mDNS flag on suspension. Now that it is saving the flag itself and doesn't
need to be reminded every time it re-registers with the MR, I think the flag
could keep its name and this could become !is_mdns_discovery_enabled_. What do
you think?

[imcheng, 2016/03/24 02:29:31]

Actually, it does need to be reminded on every wakeup, because extension updates
will wipe out the mdns_enabled flag on the extension. So the solution here is to
always call EnableMdnsDiscovery() on wakeup.

+    media_route_provider_->EnableMdnsDiscovery();

[imcheng, 2016/03/24 01:17:19]

Since we already enable mdns discovery on the extension by default on
non-Windows, should this code be Windows only?

[btolsch, 2016/03/24 01:44:03]

Yes, this could also be Windows-only. Done.

+  }
 }
 
 void MediaRouterMojoImpl::OnIssue(const interfaces::IssuePtr issue) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG_WITH_INSTANCE(1) << "OnIssue " << issue->title;
   const Issue& issue_converted = issue.To<Issue>();
   issue_manager_.AddIssue(issue_converted);
 }
 
 void MediaRouterMojoImpl::OnSinksReceived(
@@ skipping 225 matching lines @@
 void MediaRouterMojoImpl::AddIssue(const Issue& issue) {
   DCHECK(thread_checker_.CalledOnValidThread());
   issue_manager_.AddIssue(issue);
 }
 
 void MediaRouterMojoImpl::ClearIssue(const Issue::Id& issue_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   issue_manager_.ClearIssue(issue_id);
 }
 
+bool MediaRouterMojoImpl::is_mdns_discovery_enabled() const {
+  return is_mdns_discovery_enabled_;
+}
+
+void MediaRouterMojoImpl::EnableMdnsDiscovery() {
+  if (media_route_provider_) {

[mark a. foltz, 2016/03/24 00:04:41]

What if is_mdns_discovery_enabled_ is already true?

Nit: Remove { } but leave newline after if.

[btolsch, 2016/03/24 01:44:03]

I think I wrote this because the extension wasn't saving the mDNS flag on
suspension so this was also being called when the extension wakes up to restore
the flag. Now, however, the extension is saving that flag, so I don't think this
is necessary.

[btolsch, 2016/03/24 21:32:26]

Per Derek's comment about extension upgrade, this is still necessary.

+    media_route_provider_->EnableMdnsDiscovery();
+  }
+  is_mdns_discovery_enabled_ = true;
+}
+
 bool MediaRouterMojoImpl::RegisterMediaSinksObserver(
     MediaSinksObserver* observer) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Create an observer list for the media source and add |observer|
   // to it. Fail if |observer| is already registered.
   const std::string& source_id = observer->source().id();
   auto* sinks_query = sinks_queries_.get(source_id);
   bool new_query = false;
   if (!sinks_query) {
@@ skipping 509 matching lines @@
   if (current_wake_reason_ == MediaRouteProviderWakeReason::TOTAL_COUNT)
     current_wake_reason_ = reason;
 }
 
 void MediaRouterMojoImpl::ClearWakeReason() {
   DCHECK(current_wake_reason_ != MediaRouteProviderWakeReason::TOTAL_COUNT);
   current_wake_reason_ = MediaRouteProviderWakeReason::TOTAL_COUNT;
 }
 
 }  // namespace media_router