[Media Router] Conditionally enable mDNS on Windows.

chrome/browser/media/router/media_router_mojo_impl.cc

Index: chrome/browser/media/router/media_router_mojo_impl.cc
diff --git a/chrome/browser/media/router/media_router_mojo_impl.cc b/chrome/browser/media/router/media_router_mojo_impl.cc
index 54c50ee20c89b837ddffe385d4c6fe37d90b0fdd..4ecddc4e21a0ddc98030f0d5a9d404bc3cdd9564 100644
--- a/chrome/browser/media/router/media_router_mojo_impl.cc
+++ b/chrome/browser/media/router/media_router_mojo_impl.cc
@@ -15,6 +15,7 @@
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/media/router/issues_observer.h"
+#include "chrome/browser/media/router/media_route_provider_util_win.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"
@@ -81,6 +82,11 @@ MediaRouterMojoImpl::MediaRouterMojoImpl(
       current_wake_reason_(MediaRouteProviderWakeReason::TOTAL_COUNT),
       weak_factory_(this) {
   DCHECK(event_page_tracker_);
+#if defined(OS_WIN)

[mark a. foltz, 2016/03/25 23:14:16]

Prefer to check initial vale of should_ here instead of #ifdef

[btolsch, 2016/03/26 10:53:59]

There is nothing for this function call to link to on other platforms. I could
add a stub that just calls the callback with true but I think that obfuscates
things more than the #if. Let me know how you feel about that.

+  CanFirewallUseLocalPorts(
+      base::Bind(&MediaRouterMojoImpl::EnableMdnsFromFirewallCheck,
+                 base::Unretained(this)));

[mark a. foltz, 2016/03/25 23:14:16]

There's two design issues here (first is mandatory to fix).

- Unless you can guarantee that the task posted to the FILE thread will be
cancelled before |this| is destroyed, it could create a use after free when the
browser context is being shut down.  Option (1) is to store the callback in a
member and explicitly cancel it in the dtor.  Option (2) is to add a
WeakPtrFactory to the MediaRouterMojoImpl and use that to generate a weak
pointer to pass to the callback.  I generally prefer (1) 

- In principle, this check needs to be done only once per browser process (we're
not trying to handle firewall changes at runtime).  Since each active browser
profile will instantiate its own MediaRouterMojoImpl, there will be some extra
checks for users with multiple profiles.  Not a big deal since few users do
that, but maybe worth noting.  (One fix would be to move this check to
MediaRouterFactory (?) where it is cached and passed to each instance, but that
adds complexity for a little bit of performance gain.)

[btolsch, 2016/03/26 10:53:59]

MediaRouterMojoImpl already has a WeakPtrFactory for other similar callback
cases. I will reuse that to implement (2).

+#endif
 }
 
 MediaRouterMojoImpl::~MediaRouterMojoImpl() {
@@ -157,6 +163,11 @@ void MediaRouterMojoImpl::RegisterMediaRouteProvider(
   callback.Run(instance_id_);
   ExecutePendingRequests();
   wakeup_attempt_count_ = 0;
+#if defined(OS_WIN)

[mark a. foltz, 2016/03/25 23:14:16]

Why only for Windows? Are we assuming that the MRPM has already enabled MDNS? 
Please add a comment explaining this.

[btolsch, 2016/03/26 10:53:59]

Yes, the extension is also aware of whether it's on Windows or not and enables
mDNS accordingly. I added a comment here. I think it would be cleaner but less
efficient to remove that logic from the extension. I can leave a TODO to
consider that if you think it would be worthwhile.

+  if (should_enable_mdns_discovery_) {
+    media_route_provider_->EnableMdnsDiscovery();

[mark a. foltz, 2016/03/25 23:14:16]

There's a code path L149-158 where the event page is not yet awake when the
registration request is processed.  In that case, this code won't be run.

[btolsch, 2016/03/26 10:53:59]

Doesn't this path get retried once the extension wakes up though? So this will
execute once the extension is awake, which is the same as queueing it. Am I
missing something?

+  }
+#endif
 }
 
 void MediaRouterMojoImpl::OnIssue(const interfaces::IssuePtr issue) {
@@ -402,6 +413,17 @@ void MediaRouterMojoImpl::ClearIssue(const Issue::Id& issue_id) {
   issue_manager_.ClearIssue(issue_id);
 }
 
+bool MediaRouterMojoImpl::should_enable_mdns_discovery() const {
+  return should_enable_mdns_discovery_;
+}
+
+void MediaRouterMojoImpl::EnsureMdnsDiscoveryEnabled() {
+  SetWakeReason(MediaRouteProviderWakeReason::ENABLE_MDNS_DISCOVERY);

[mark a. foltz, 2016/03/25 23:14:16]

I'm not sure what this method should do when should_ == true.  How do you
prevent Enable...() from being called multiple times?

[btolsch, 2016/03/26 10:53:58]

I'm not preventing it from being called multiple times since calling it multiple
times isn't a correctness issue. I added another state variable to indicate
whether we have already told the extension since it has been registered. If this
is too much noise then I think the only option is to leave it like this.

+  RunOrDefer(base::Bind(&MediaRouterMojoImpl::DoEnsureMdnsDiscoveryEnabled,
+                        base::Unretained(this)));
+  should_enable_mdns_discovery_ = true;
+}
+
 bool MediaRouterMojoImpl::RegisterMediaSinksObserver(
     MediaSinksObserver* observer) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -823,6 +845,11 @@ void MediaRouterMojoImpl::DoStopObservingMediaRoutes(
   routes_queries_.erase(source_id);
 }
 
+void MediaRouterMojoImpl::DoEnsureMdnsDiscoveryEnabled() {
+  DVLOG_WITH_INSTANCE(1) << "DoEnsureMdnsDiscoveryEnabled";
+  media_route_provider_->EnableMdnsDiscovery();

[mark a. foltz, 2016/03/25 23:14:16]

Another task could have already called this.

[btolsch, 2016/03/26 10:53:59]

See the comment above.

+}
+
 void MediaRouterMojoImpl::EnqueueTask(const base::Closure& closure) {
   pending_requests_.push_back(closure);
   if (pending_requests_.size() > kMaxPendingRequests) {
@@ -930,4 +957,11 @@ void MediaRouterMojoImpl::ClearWakeReason() {
   current_wake_reason_ = MediaRouteProviderWakeReason::TOTAL_COUNT;
 }
 
+void MediaRouterMojoImpl::EnableMdnsFromFirewallCheck(
+    bool firewall_can_use_local_ports) {
+  if (firewall_can_use_local_ports) {

[mark a. foltz, 2016/03/25 23:14:16]

Nit: Remove { }

[btolsch, 2016/03/26 10:53:58]

Done.

+    EnsureMdnsDiscoveryEnabled();
+  }
+}
+
 }  // namespace media_router