[Media Router] Add DialMediaSinkService and DeviceDescriptionService

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

+// Copyright 2017 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/discovery/dial/device_description_service.h"
+
+#include "base/strings/string_util.h"
+#include "chrome/browser/media/router/discovery/dial/device_description_fetcher.h"
+#include "chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.h"
+#include "net/base/ip_address.h"
+#include "url/gurl.h"
+
+namespace {
+
+// How long to cache a device description.
+// TODO: Use value from chrome.dial when http://crbug.com/165289 is fixed.

[mark a. foltz, 2017/04/12 00:17:17]

I'm going to close this as WontFix.

[zhaobin, 2017/04/18 06:58:26]

Done.

+const int kDeviceDescriptionCacheTimeMillis = 30 * 60 * 1000;
+
+// Replaces "<element_name>content</element_name>" with
+// "<element_name>***</element_name>"
+void Scrub(const std::string& element_name, std::string& xml_text) {

[imcheng, 2017/04/12 19:33:50]

nit: |xml_text| should be passed in as a pointer.

[zhaobin, 2017/04/18 06:58:25]

Done.

+  size_t pos = xml_text.find("<" + element_name + ">");
+  size_t end_pos = xml_text.find("</" + element_name + ">");
+  size_t start_pos = pos + element_name.length() + 2;

[mark a. foltz, 2017/04/12 00:17:17]

What happens if pos == std::string::npos?

[zhaobin, 2017/04/18 06:58:26]

Done.

+
+  if (pos != std::string::npos && end_pos > start_pos)
+    xml_text.replace(start_pos, end_pos - start_pos, "***");
+}
+
+// Removes unique identifiers from the device description.
+// |xml_text|: xmlDoc The device description.
+// Returns false if <UDN> or <serialNumber> field does not exist in |xml_text|.

[mark a. foltz, 2017/04/12 00:17:17]

This needs to be updated

[zhaobin, 2017/04/18 06:58:25]

Done.

+std::string ScrubDeviceDescriptionXml(const std::string& xml_text) {
+  std::string scrubbed_xml(xml_text);
+  Scrub("UDN", scrubbed_xml);
+  Scrub("serialNumber", scrubbed_xml);
+  return scrubbed_xml;
+}
+
+// Validates whether a URL is valid for fetching device descriptions.
+// |descriptionUrl|: Device description URL.
+// Returns true if descriptionUrl is valid.
+static bool IsValidDeviceDescriptionUrl(const GURL& device_description_url) {

[imcheng, 2017/04/12 19:33:50]

Functions in anonymous namespace do not need to be static.

[zhaobin, 2017/04/18 06:58:25]

Done.

+  if (!device_description_url.SchemeIs(url::kHttpScheme))
+    return false;
+
+  net::IPAddress address;
+  if (!net::ParseURLHostnameToAddress(device_description_url.host(), &address))

[mark a. foltz, 2017/04/12 00:17:17]

This check could go earlier in DialService::ParseResponse.  We should also check
there that this IP address matches the address that we received the SSDP
advertisement from.   Feel free to add a TODO(crbug.com/679432) here.

[zhaobin, 2017/04/18 06:58:26]

Done.

+    return false;
+
+  return address.IsReserved();
+}
+
+static bool IsValidAppUrl(const GURL& app_url, const std::string& ip_address) {
+  return app_url.SchemeIs(url::kHttpScheme) && app_url.host() == ip_address;
+}
+
+// Checks mandatory fields.

[mark a. foltz, 2017/04/12 00:17:17]

Please document meaning of return value.

[zhaobin, 2017/04/18 06:58:25]

Done.

+static std::string Validate(

[imcheng, 2017/04/12 19:33:50]

Maybe ValidateParsedDeviceDescription to be more clear?

[zhaobin, 2017/04/18 06:58:25]

Done.

+    const std::string& expected_ip_address,
+    const media_router::ParsedDialDeviceDescription& description_data) {
+  if (description_data.unique_id.empty()) {
+    return "Missing uniqueId";

[imcheng, 2017/04/12 19:33:50]

IMO, enum is slightly preferable to strings for indicating error reasons. It's
slightly more impact and can also be used for UMA.

[zhaobin, 2017/04/18 06:58:25]

Done.

+  }
+  if (description_data.friendly_name.empty()) {
+    return "Missing friendlyName";
+  }
+  if (!description_data.app_url.is_valid()) {
+    return "Missing appUrl";
+  }
+  if (!IsValidAppUrl(description_data.app_url, expected_ip_address)) {
+    return "Invalid appUrl";
+  }
+  return "";
+}
+
+static std::string ToString(

[mark a. foltz, 2017/04/12 00:17:17]

If this is only use with D*LOG statements, use #ifndef NDEBUG around this
function to remove it from release builds.

[imcheng, 2017/04/12 19:33:50]

Naming suggestion: CachedDeviceDescriptionToString

[zhaobin, 2017/04/18 06:58:25]

Done.

[zhaobin, 2017/04/18 06:58:26]

Done.

+    const media_router::CachedParsedDialDeviceDescription& cached_data) {
+  std::stringstream ss;
+  ss << "CachedDialDeviceDescription [unique_id]: "
+     << cached_data.description_data.unique_id
+     << " [friendly_name]: " << cached_data.description_data.friendly_name
+     << " [model_name]: " << cached_data.description_data.model_name
+     << " [device_type]: " << cached_data.description_data.device_type
+     << " [app_url]: " << cached_data.description_data.app_url
+     << " [fetch_time_millis]: " << cached_data.fetch_time_millis
+     << " [expire_time_millis]: " << cached_data.expire_time_millis
+     << " [config_id]: ";

[mark a. foltz, 2017/04/12 00:17:17]

Nit: Maybe only include this if config_id is set.

[zhaobin, 2017/04/18 06:58:26]

Done.

+
+  if (cached_data.config_id)
+    ss << cached_data.config_id.value();
+
+  return ss.str();
+}
+
+}  // namespace
+
+namespace media_router {
+
+DeviceDescriptionService::DeviceDescriptionService(
+    DeviceDescriptionParseSuccessCallback success_cb,
+    DeviceDescriptionParseErrorCallback error_cb)
+    : success_cb_(success_cb), error_cb_(error_cb) {}
+
+DeviceDescriptionService::~DeviceDescriptionService() = default;
+
+void DeviceDescriptionService::GetDeviceDescriptions(
+    const std::vector<DialDeviceData>& devices,

[imcheng, 2017/04/12 19:33:50]

Is the idea that this gets called with the full device list known to
DialRegistry?

I think it makes sense to abandon pending fetches where the device description
was updated. I think we can keep pending fetches that aren't on the list though.

[zhaobin, 2017/04/18 06:58:26]

Done.

+    net::URLRequestContextGetter* request_context) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  std::map<std::string, std::unique_ptr<DeviceDescriptionFetcher>>
+      existing_fetcher_map;
+  for (const auto& device_data : devices) {
+    // Keep fetcher if it is for |device_data.label()| and has the same Url.

[mark a. foltz, 2017/04/12 00:17:17]

nit: URL

[zhaobin, 2017/04/18 06:58:25]

Done.

+    const auto& it = device_description_fetcher_map_.find(device_data.label());
+    if (it != device_description_fetcher_map_.end() &&
+        it->second->device_description_url() ==
+            device_data.device_description_url()) {
+      existing_fetcher_map.insert(
+          std::make_pair(device_data.label(), std::move(it->second)));
+    }
+  }
+
+  // Remove all out dated fetcheres.

[mark a. foltz, 2017/04/12 00:17:17]

typo in fetchers

[zhaobin, 2017/04/18 06:58:25]

Done.

+  device_description_fetcher_map_ = std::move(existing_fetcher_map);

[mark a. foltz, 2017/04/12 00:17:17]

Will this delete all fetchers whose labels are not included in |devices|?

Is it safe to delete a fetcher even while the fetch is running?

[zhaobin, 2017/04/18 06:58:26]

Seems to be safe. net::URLFetcher says "You may cancel the request by destroying
the URLFetcher"
(https://cs.chromium.org/chromium/src/net/url_request/url_fetcher.h?l=77). It
also says no callbacks will occur after cancelling.

+
+  for (const auto& device_data : devices) {
+    CachedParsedDialDeviceDescription description_data;
+    if (CheckAndUpdateCache(device_data, &description_data)) {
+      // Get device description from cache.
+      success_cb_.Run(device_data, description_data.description_data);
+      continue;
+    }
+
+    FetchDeviceDescription(device_data, request_context);
+  }
+}
+
+void DeviceDescriptionService::FetchDeviceDescription(
+    const DialDeviceData& device_data,
+    net::URLRequestContextGetter* request_context) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!IsValidDeviceDescriptionUrl(device_data.device_description_url())) {
+    std::string error_message = "Invalid device description URL: " +
+                                device_data.device_description_url().spec();
+    OnDeviceDescriptionFetchError(device_data, error_message);
+    return;
+  }
+
+  auto device_description_fetcher = base::MakeUnique<DeviceDescriptionFetcher>(
+      device_data.device_description_url(), request_context,
+      base::BindOnce(
+          &DeviceDescriptionService::OnDeviceDescriptionFetchComplete,
+          base::Unretained(this), device_data),
+      base::BindOnce(&DeviceDescriptionService::OnDeviceDescriptionFetchError,
+                     base::Unretained(this), device_data));
+
+  device_description_fetcher->Start();
+  device_description_fetcher_map_.insert(std::make_pair(
+      device_data.label(), std::move(device_description_fetcher)));
+}
+
+bool DeviceDescriptionService::CheckAndUpdateCache(
+    const DialDeviceData& device_data,
+    CachedParsedDialDeviceDescription* out_description) {
+  const auto& it = description_map_.find(device_data.label());
+  if (it == description_map_.end())
+    return false;
+
+  // If the entry's configId does not match, or it has expired, remove it.

[mark a. foltz, 2017/04/12 00:17:17]

nit: config_id

[zhaobin, 2017/04/18 06:58:25]

Done.

+  if (it->second.config_id != device_data.config_id() ||
+      base::Time::Now() >= it->second.expire_time_millis) {

[imcheng, 2017/04/12 19:33:50]

General comment: for tests you might find it useful to place base::Time::Now()
inside a virtual GetNow() method. Then in tests you can override that method to
return a fake time.

[zhaobin, 2017/04/18 06:58:25]

Done.

+    DVLOG(2) << "Removing invalid entry " << it->first;
+    description_map_.erase(it);

[mark a. foltz, 2017/04/12 00:17:17]

It looks like there will be some expired cached entries we never remove, if the
devices are not passed to CheckAndUpdateCache.  Do you want a garbage collection
pass to run at some infrequent interval?

+    return false;
+  }
+
+  // Entry is valid.
+  (*out_description) = it->second;

[mark a. foltz, 2017/04/12 00:17:17]

It it necessary to make a copy here, or could CheckAndUpdateCache return a const
pointer to CachedParsedDeviceDescription?  I guess it depends on whether
OnDeviceDescriptionFetchComplete can run before success_cb_ is finished handling
the result.

[zhaobin, 2017/04/18 06:58:26]

Done.

+  return true;
+}
+
+void DeviceDescriptionService::OnParsedDeviceDescription(
+    const DialDeviceData& device_data,
+    const GURL& app_url,
+    chrome::mojom::DialDeviceDescriptionPtr parsed_device_description) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!parsed_device_description) {
+    error_cb_.Run(device_data.label(),
+                  "Failed to parse device description xml");
+    return;
+  }
+
+  ParsedDialDeviceDescription description_data;
+  description_data.unique_id = parsed_device_description->unique_id;
+  description_data.friendly_name = parsed_device_description->friendly_name;
+  description_data.model_name = parsed_device_description->model_name;
+  description_data.device_type = parsed_device_description->device_type;
+  description_data.app_url = app_url;
+
+  std::string error =
+      Validate(device_data.device_description_url().host(), description_data);
+
+  if (error.empty()) {

[mark a. foltz, 2017/04/12 00:17:17]

Nit: I would reverse the two cases of the if() and return early, so there isn't
such a long block below.

[zhaobin, 2017/04/18 06:58:26]

Done.

+    CachedParsedDialDeviceDescription cached_description_data;
+    cached_description_data.fetch_time_millis = base::Time::Now();
+    cached_description_data.expire_time_millis =
+        cached_description_data.fetch_time_millis +
+        base::TimeDelta::FromMilliseconds(kDeviceDescriptionCacheTimeMillis);
+    cached_description_data.config_id = device_data.config_id();
+    cached_description_data.description_data = description_data;
+
+    DVLOG(2) << "Got device description for " << device_data.label();

[mark a. foltz, 2017/04/12 00:17:17]

Nit: These can be combined into one statement.

[zhaobin, 2017/04/18 06:58:26]

Done.

+    DVLOG(2) << "... device description was: "
+             << ToString(cached_description_data);
+
+    // Stick it in the cache if the description has a config (version) id.
+    // NOTE: We could cache descriptions without a config id and rely on the
+    // timeout to eventually update the cache.

[mark a. foltz, 2017/04/12 00:17:17]

This would be fine with me.  The only piece of data we really need from the
device description is the friendly name and app URL.  Nearly all devices hard
code these, and they are unlikely to change over the lifetime of the device.  We
could set a fairly long cache timeout, say 12 hours.

[zhaobin, 2017/04/18 06:58:26]

Acknowledged.

+    if (cached_description_data.config_id) {
+      DVLOG(2) << "Caching device description for " << device_data.label();
+      description_map_.insert(

[mark a. foltz, 2017/04/12 00:17:17]

Can we set a maximum size on the number of cached entries?  Say, 256?  Most
users would never hit it, but the limit would prevent this from consuming
arbitrary amounts of memory even if something on the network was advertising a
large number of devices.

[zhaobin, 2017/04/18 06:58:25]

Done.

+          std::make_pair(device_data.label(), cached_description_data));
+    }
+
+    success_cb_.Run(device_data, description_data);
+  } else {
+    DLOG(WARNING) << "Device description failed to validate: " << error;
+    error_cb_.Run(device_data.label(), "Failed to process fetch result");
+  }
+}
+
+void DeviceDescriptionService::OnDeviceDescriptionFetchComplete(
+    const DialDeviceData& device_data,
+    const DialDeviceDescriptionData& description_data) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  scoped_refptr<SafeDialDeviceDescriptionParser> parser(

[mark a. foltz, 2017/04/12 00:17:17]

What keeps the parser alive after calling Start()?  It doesn't appear to be
bound in the callback.

[zhaobin, 2017/04/18 06:58:26]

It is bound insdie parser->start()
(https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro...)

+      new SafeDialDeviceDescriptionParser());

[mark a. foltz, 2017/04/12 00:17:17]

Could this potentially spawn multiple utility processes? We probably want to
spawn one process and feed it XML serially to avoid spawning several processes
in parallel, if multiple devices are discovered at once.

One thought: Since the SafeDialDescriptionParser is ref counted, you could
create a scoped_refptr for each description that needs to be parsed, so that it
is destroyed when the last parse completes.

[zhaobin, 2017/04/18 06:58:25]

Done.

+
+  parser->Start(description_data.device_description,
+                base::Bind(&DeviceDescriptionService::OnParsedDeviceDescription,
+                           base::Unretained(this), device_data,

[mark a. foltz, 2017/04/12 00:17:17]

What prevents the callback from being run after |this| is deleted?

[zhaobin, 2017/04/18 06:58:26]

(I think) |this| is owned by DialMediaSinkService, which will be owned by
MediaRouter (BrowserContextKeyedService). Since browser context should outlive
utility process, |this| will always be valid when callback is invoked by utility
process.

+                           description_data.app_url));
+
+  std::string xml_logging =

[imcheng, 2017/04/12 19:33:50]

Similar to above, we want to guard this with #ifndef NDEBUG. 

[zhaobin, 2017/04/18 06:58:25]

Done.

+      ScrubDeviceDescriptionXml(description_data.device_description);
+  DVLOG(2) << "Device description: " << xml_logging;
+
+  device_description_fetcher_map_.erase(device_data.label());
+}
+
+void DeviceDescriptionService::OnDeviceDescriptionFetchError(
+    const DialDeviceData& device_data,
+    const std::string& error_message) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  DVLOG(2) << "OnDeviceDescriptionFetchError [label]: " << device_data.label();
+
+  error_cb_.Run(device_data.label(), error_message);
+  device_description_fetcher_map_.erase(device_data.label());
+}
+
+CachedParsedDialDeviceDescription::CachedParsedDialDeviceDescription() =
+    default;
+CachedParsedDialDeviceDescription::CachedParsedDialDeviceDescription(
+    const CachedParsedDialDeviceDescription& other) = default;
+CachedParsedDialDeviceDescription::~CachedParsedDialDeviceDescription() =
+    default;
+
+}  // namespace media_router