Update ActivityLog detection to use previous values, hrefs.

chrome/browser/extensions/activity_log/activity_actions.cc

 // Copyright (c) 2013 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/extensions/activity_log/activity_actions.h"
 
 #include <algorithm>  // for std::find.
 #include <string>
 
 #include "base/command_line.h"
 #include "base/format_macros.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/activity_log/activity_action_constants.h"
 #include "chrome/browser/extensions/activity_log/ad_network_database.h"
 #include "chrome/browser/extensions/activity_log/fullstream_ui_policy.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/rappor/rappor_service.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/common/ad_injection_constants.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/dom_action_types.h"
 #include "sql/statement.h"
 #include "url/gurl.h"
 
 namespace constants = activity_log_constants;
 
 namespace extensions {
 
 namespace {
 
@@ skipping 46 matching lines @@
   object->GetString(keys::kType, &type);
 
   std::string url_key;
   if (IsSrcElement(type))
     url_key = keys::kSrc;
   else if (IsHrefElement(type))
     url_key = keys::kHref;
 
   if (!url_key.empty()) {
     std::string url;
-    if (object->GetString(url_key, &url) &&
-        AdNetworkDatabase::Get()->IsAdNetwork(GURL(url))) {
-      return Action::INJECTION_NEW_AD;
+    if (object->GetString(url_key, &url)) {
+      GURL gurl(url);
+      if (AdNetworkDatabase::Get()->IsAdNetwork(gurl))
+        return Action::INJECTION_NEW_AD;
+      // If the extension injected an URL which is not local to itself, there is
+      // a good chance it could be a new ad, and our database missed it.
+      // This could be noisier than other metrics, because there are perfectly
+      // acceptable uses for this, like "Show my mail".

[felt, 2014/05/09 01:03:54]

Hmm, this does seem very noisy (especially for links). Are you sure about this
one? It seems to make more sense down in CheckSrcModification().

[Devlin, 2014/05/09 15:48:17]

TL;DR: Yes, it could be noisy, but just putting it in CheckSrcModification()
won't help.  It will only affect this metric, so we can ignore it if it's
ridiculously noisy, but if there are ways to improve, I'd be happy to hear them.
:)

It could be potentially noisy, yeah, but it will be equally noisy here and in
CheckSrcModification(), because, chances are, if the extension injects an
iframe/anchor/embed, they set its src/href at some point, too.  My hope is that:
- There are actually relatively few extensions which actively inject any kind of
link or iframe into the DOM.  It seems like most valid cases would actually have
the link/iframe be static HTML (like maybe seen on a browser action popup or an
options page), rather than dynamically generated.
- The noisiness is fully contained to this metric.  If it's drastically higher
than we'd ever expect, then we can know that it's too simple a heuristic, and
re-evaluate.

In an ideal world, we'd actually have much more finely-grained metrics, with
individual counts for non-local iframes/embeds/hrefs (and in the future, images,
etc), but this gets unwieldy fast, when we consider that we track injections,
modifications, potential removals, etc.  We might be able to sit down and figure
out a better metrics system for these (maybe using histogram enum instead of
just a count), but that's a discussion for a later branch point. ;)

+      if (gurl.is_valid() &&
+          !gurl.is_empty() &&
+          !gurl.SchemeIs(kExtensionScheme)) {
+        return Action::INJECTION_LIKELY_NEW_AD;
+      }
     }
   }
 
   const base::ListValue* children = NULL;
   if (object->GetList(keys::kChildren, &children)) {
     const base::DictionaryValue* child = NULL;
     for (size_t i = 0;
          i < children->GetSize() &&
              i < ad_injection_constants::kMaximumChildrenToCheck;
          ++i) {
-      if (children->GetDictionary(i, &child) && CheckDomObject(child))
-        return Action::INJECTION_NEW_AD;
+      if (children->GetDictionary(i, &child)) {
+        Action::InjectionType type = CheckDomObject(child);
+        if (type != Action::NO_AD_INJECTION)
+          return type;
+      }
     }
   }
 
   return Action::NO_AD_INJECTION;
 }
 
 }  // namespace
 
 using api::activity_log_private::ExtensionActivity;
 
@@ skipping 35 matching lines @@
 Action::InjectionType Action::DidInjectAd(
     rappor::RapporService* rappor_service) const {
   MaybeUploadUrl(rappor_service);
 
   // Currently, we do not have the list of ad networks, so we exit immediately
   // with NO_AD_INJECTION (unless the database has been set by a test).
   if (!AdNetworkDatabase::Get())
     return NO_AD_INJECTION;
 
   if (api_name_ == ad_injection_constants::kHtmlIframeSrcApiName ||
-      api_name_ == ad_injection_constants::kHtmlEmbedSrcApiName) {
+      api_name_ == ad_injection_constants::kHtmlEmbedSrcApiName ||
+      api_name_ == ad_injection_constants::kHtmlAnchorHrefApiName) {
     return CheckSrcModification();
   } else if (EndsWith(api_name_,
                       ad_injection_constants::kAppendChildApiSuffix,
                       true /* case senstive */)) {
     return CheckAppendChild();
   }
 
   return NO_AD_INJECTION;
 }
 
@@ skipping 236 matching lines @@
   if (!can_inject_ads)
     return;
 
   // Record the URL - an ad *may* have been injected.
   rappor_service->RecordSample(kExtensionAdInjectionRapporMetricName,
                                rappor::ETLD_PLUS_ONE_RAPPOR_TYPE,
                                host);
 }
 
 Action::InjectionType Action::CheckSrcModification() const {
-  bool injected_ad = arg_url_.is_valid() &&
-                     !arg_url_.is_empty() &&
-                     AdNetworkDatabase::Get()->IsAdNetwork(arg_url_);
-  return injected_ad ? INJECTION_NEW_AD : NO_AD_INJECTION;
+  const AdNetworkDatabase* database = AdNetworkDatabase::Get();
+
+  bool arg_url_valid = arg_url_.is_valid() && !arg_url_.is_empty();
+
+  GURL prev_url;
+  std::string prev_url_string;
+  if (args_.get() && args_->GetString(1u, &prev_url_string))
+    prev_url = GURL(prev_url_string);
+
+  bool prev_url_valid = prev_url.is_valid() && !prev_url.is_empty();
+
+  bool injected_ad = arg_url_valid && database->IsAdNetwork(arg_url_);
+  bool replaced_ad = prev_url_valid && database->IsAdNetwork(prev_url);
+
+  if (injected_ad && replaced_ad)
+    return INJECTION_REPLACED_AD;
+  if (injected_ad)
+    return INJECTION_NEW_AD;
+  if (replaced_ad)
+    return INJECTION_REMOVED_AD;
+
+  // If the extension modified the URL with an external, valid URL then there's
+  // a good chance it's ad injection. Log it as a likely one, which also helps
+  // us determine the effectiveness of our IsAdNetwork() recognition.
+  if (arg_url_valid && !arg_url_.SchemeIs(kExtensionScheme)) {
+    if (prev_url_valid)
+      return INJECTION_LIKELY_REPLACED_AD;
+    return INJECTION_LIKELY_NEW_AD;
+  }
+
+  return NO_AD_INJECTION;
 }
 
 Action::InjectionType Action::CheckAppendChild() const {
   const base::DictionaryValue* child = NULL;
   if (!args_->GetDictionary(0u, &child))
     return NO_AD_INJECTION;
 
   return CheckDomObject(child);
 }
 
@@ skipping 55 matching lines @@
     std::string rhs_other = ActivityLogPolicy::Util::Serialize(rhs->other());
     if (lhs_other != rhs_other)
       return lhs_other < rhs_other;
   }
 
   // All fields compare as equal if this point is reached.
   return false;
 }
 
 }  // namespace extensions