Submit a sample of notification images to Safe Browsing

chrome/browser/safe_browsing/notification_image_reporter.cc

Index: chrome/browser/safe_browsing/notification_image_reporter.cc
diff --git a/chrome/browser/safe_browsing/notification_image_reporter.cc b/chrome/browser/safe_browsing/notification_image_reporter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1c80e5e627782e3cf9f46f12f81b782a73b0c0b2
--- /dev/null
+++ b/chrome/browser/safe_browsing/notification_image_reporter.cc
@@ -0,0 +1,220 @@
+// 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/safe_browsing/notification_image_reporter.h"
+
+#include <cmath>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/rand_util.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+#include "components/safe_browsing_db/database_manager.h"
+#include "components/safe_browsing_db/safe_browsing_prefs.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/base/net_errors.h"
+#include "net/url_request/report_sender.h"
+#include "skia/ext/image_operations.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/geometry/size.h"
+#include "url/gurl.h"
+
+using content::BrowserThread;
+
+namespace safe_browsing {
+
+namespace {
+
+const size_t kMaxReportsPerDay = 5;
+const char kImageReporting[] = "NotificationImageReporting";
+const char kDefaultMimeType[] = "image/png";
+
+void LogReportResult(const GURL& url, int net_error) {
+  UMA_HISTOGRAM_SPARSE_SLOWLY("SafeBrowsing.NotificationImageReporter.NetError",
+                              net_error);
+}
+
+}  // namespace
+
+const char NotificationImageReporter::kReportingUploadUrl[] =
+    "https://safebrowsing.googleusercontent.com/safebrowsing/clientreport/"
+    "notification-image";
+
+NotificationImageReporter::NotificationImageReporter(
+    net::URLRequestContext* request_context)
+    : NotificationImageReporter(base::MakeUnique<net::ReportSender>(
+          request_context,
+          net::ReportSender::CookiesPreference::DO_NOT_SEND_COOKIES)) {}
+
+NotificationImageReporter::NotificationImageReporter(
+    std::unique_ptr<net::ReportSender> report_sender)
+    : report_sender_(std::move(report_sender)), weak_factory_on_io_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Create an experiment to control whether this instance sends image reports.
+  // The group will have 0% enabled and will only be turned on using Finch.
+  scoped_refptr<base::FieldTrial> trial(
+      base::FieldTrialList::FactoryGetFieldTrial(
+          kImageReporting, 100, "disabled", 2018, 1, 1,
+          base::FieldTrial::SESSION_RANDOMIZED, nullptr));
+  trial->AppendGroup("enabled", 0);
+}
+
+NotificationImageReporter::~NotificationImageReporter() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+}
+
+void NotificationImageReporter::ReportNotificationImageOnIO(
+    Profile* profile,
+    const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager,
+    const GURL& origin,
+    const SkBitmap& image) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(profile);
+  DCHECK_EQ(origin, origin.GetOrigin());
+  DCHECK(origin.is_valid());
+
+  // Skip whitelisted origins to cut down on report volume.
+  if (!database_manager || database_manager->MatchCsdWhitelistUrl(origin)) {
+    SkippedReporting();
+    return;
+  }
+
+  // Sample a Finch-controlled fraction only.
+  if (!IsReportingEnabled()) {

[Jialiu Lin, 2017/01/19 18:54:47]

Finch checking does not need to be in the IO thread, right?
I'll suggest move the finch checking and scout checking logic to
SafeBrowsingPingManager::ReportNotificationImage. such that, you don't need to
worry about underreporting. 

[harkness, 2017/01/19 20:09:09]

Unfortunately, IsReportingEnabled is virtual, and ReportNotificationImageOnUI is
static, so I can't move it there.

[Jialiu Lin, 2017/01/19 23:10:40]

Acknowledged.

+    SkippedReporting();
+    return;
+  }
+
+  // Avoid exceeding kMaxReportsPerDay.
+  base::Time a_day_ago = base::Time::Now() - base::TimeDelta::FromDays(1);
+  while (!report_times_.empty() &&
+         report_times_.front() < /* older than */ a_day_ago) {
+    report_times_.pop();
+  }
+  if (report_times_.size() >= kMaxReportsPerDay) {
+    SkippedReporting();
+    return;
+  }
+  // n.b. we write to report_times_ here even if we'll later end up skipping
+  // reporting because GetExtendedReportingLevel was not SBER_LEVEL_SCOUT. That
+  // saves us two thread hops, with the downside that we may underreport
+  // notifications on the first day that a user opts in to SBER_LEVEL_SCOUT.
+  report_times_.push(base::Time::Now());
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&NotificationImageReporter::ReportNotificationImageOnUI,
+                 weak_factory_on_io_.GetWeakPtr(), profile, origin, image));
+}
+
+bool NotificationImageReporter::IsReportingEnabled() const {
+  return (base::FieldTrialList::FindFullName(kImageReporting) == "enabled");
+}
+
+void NotificationImageReporter::SkippedReporting() {}
+
+// static
+void NotificationImageReporter::ReportNotificationImageOnUI(
+    const base::WeakPtr<NotificationImageReporter>& weak_this_on_io,
+    Profile* profile,
+    const GURL& origin,
+    const SkBitmap& image) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Skip reporting unless SBER2 Scout is enabled.
+  switch (GetExtendedReportingLevel(*profile->GetPrefs())) {

[Jialiu Lin, 2017/01/19 18:54:47]

nit: since you only care about scout, it is more concise to use if statement
here. 

if(GetExtendedReportingLevel(*profile->GetPrefs()) == SBER_LEVEL_SCOUT) {
 ...send report...
}

or you can do 
if (IsExtendedReportingEnabled(prefs) && IsScout(prefs)) {
   ...
}

Also, maybe move this logic to SafeBrowsingPingManager::ReportNotificationImage.

[harkness, 2017/01/19 20:09:08]

I refactored the switch into an if which is much clearer. However, because the
scout check uses the preferences service, it needs to be run on the UI thread,
so I left the code here instead of moving it into the PingManager which runs on
the IO thread.

[Jialiu Lin, 2017/01/19 23:10:40]

Acknowledged.

+    case SBER_LEVEL_OFF:
+    case SBER_LEVEL_LEGACY:
+      BrowserThread::PostTask(
+          BrowserThread::IO, FROM_HERE,
+          base::Bind(&NotificationImageReporter::SkippedReporting,
+                     weak_this_on_io));
+      return;
+    case SBER_LEVEL_SCOUT:
+      break;
+  }
+
+  BrowserThread::GetBlockingPool()->PostWorkerTask(
+      FROM_HERE,
+      base::Bind(
+          &NotificationImageReporter::DownscaleNotificationImageOnBlockingPool,
+          weak_this_on_io, origin, image));
+}
+
+// static
+void NotificationImageReporter::DownscaleNotificationImageOnBlockingPool(
+    const base::WeakPtr<NotificationImageReporter>& weak_this_on_io,
+    const GURL& origin,
+    const SkBitmap& image) {
+  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

[Jialiu Lin, 2017/01/19 18:54:47]

Is this still on UI thread? Maybe add
DCHECK_CURRENTLY_ON(BrowserThread::UI); 

[harkness, 2017/01/19 20:09:09]

No, the blocking pool threads are distinct from the UI thread, so this cannot
run on the UI thread.

[Jialiu Lin, 2017/01/19 23:10:40]

Acknowledged.

+
+  // Downscale to fit within 512x512. TODO(johnme): Get this from Finch.
+  const double MAX_SIZE = 512;
+  SkBitmap downscaled_image = image;
+  if ((image.width() > MAX_SIZE || image.height() > MAX_SIZE) &&
+      image.width() > 0 && image.height() > 0) {
+    double scale =
+        std::min(MAX_SIZE / image.width(), MAX_SIZE / image.height());
+    downscaled_image =
+        skia::ImageOperations::Resize(image, skia::ImageOperations::RESIZE_GOOD,
+                                      std::lround(scale * image.width()),
+                                      std::lround(scale * image.height()));
+  }
+
+  // Encode as PNG.
+  std::vector<unsigned char> png_bytes;

[Jialiu Lin, 2017/01/19 18:54:46]

how about
DCHECK(gfx::PNGCodec::EncodeBGRASkBitmap(downscaled_image, false, &png_bytes));

[harkness, 2017/01/19 20:09:09]

That would break things if DCHECKs weren't enabled, since the entire line of
code would be compiled out.

[Jialiu Lin, 2017/01/19 23:10:39]

Acknowledged.

+  if (!gfx::PNGCodec::EncodeBGRASkBitmap(downscaled_image, false, &png_bytes)) {
+    NOTREACHED();
+    return;
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&NotificationImageReporter::SendReportOnIO, weak_this_on_io,
+                 origin, base::RefCountedBytes::TakeVector(&png_bytes),
+                 gfx::Size(downscaled_image.width(), downscaled_image.height()),
+                 gfx::Size(image.width(), image.height())));
+}
+
+void NotificationImageReporter::SendReportOnIO(
+    const GURL& origin,
+    scoped_refptr<base::RefCountedMemory> data,
+    const gfx::Size& dimensions,
+    const gfx::Size& original_dimensions) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  NotificationImageReportRequest report;
+  report.set_notification_origin(origin.spec());
+  report.mutable_image()->set_data(data->front(), data->size());
+  report.mutable_image()->set_mime_type(kDefaultMimeType);
+  report.mutable_image()->mutable_dimensions()->set_width(dimensions.width());
+  report.mutable_image()->mutable_dimensions()->set_height(dimensions.height());
+  if (dimensions != original_dimensions) {
+    report.mutable_image()->mutable_original_dimensions()->set_width(
+        original_dimensions.width());
+    report.mutable_image()->mutable_original_dimensions()->set_height(
+        original_dimensions.height());
+  }
+
+  std::string serialized_report;
+  report.SerializeToString(&serialized_report);
+  report_sender_->Send(
+      GURL(kReportingUploadUrl), "application/octet-stream", serialized_report,
+      base::Bind(&LogReportResult, GURL(kReportingUploadUrl), net::OK),
+      base::Bind(&LogReportResult));
+  // TODO(johnme): Consider logging bandwidth and/or duration to UMA.
+}
+
+}  // namespace safe_browsing