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..f7770e73b28c8071efd61398c262cc0f7b7eec27
--- /dev/null
+++ b/chrome/browser/safe_browsing/notification_image_reporter.cc
@@ -0,0 +1,165 @@
+// 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/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/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 "url/gurl.h"
+
+using content::BrowserThread;
+
+namespace safe_browsing {
+
+namespace {
+const int kMaxReportsPerDay = 5;

[Peter Beverloo, 2017/01/11 18:23:56]

micro nit: int -> size_t to match std::vector::size()

also please have a blank line at the beginning and end of the anonymous
namespace

[johnme, 2017/01/11 20:05:07]

Done.

+const char kReportingUploadUrl[] =
+    "https://safebrowsing.googleusercontent.com/safebrowsing/clientreport/"
+    "chrome-notification-image";  // TODO(johnme): Confirm URL.
+}  // namespace
+
+NotificationImageReporter::NotificationImageReporter(
+    net::URLRequestContext* request_context)
+    : report_sender_(base::MakeUnique<net::ReportSender>(
+          request_context,
+          net::ReportSender::CookiesPreference::DO_NOT_SEND_COOKIES)) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+}
+
+NotificationImageReporter::~NotificationImageReporter() {
+  // Thread on which this is destroyed may vary.
+}
+
+void NotificationImageReporter::ReportNotificationImageOnUI(
+    Profile* profile,
+    const GURL& origin,
+    const SkBitmap& image) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(origin, origin.GetOrigin());
+
+  // Skip reporting unless SBER2 Scout is enabled.
+  switch (GetExtendedReportingLevel(*profile->GetPrefs())) {
+    case SBER_LEVEL_OFF:
+    case SBER_LEVEL_LEGACY:
+      return;
+    case SBER_LEVEL_SCOUT:
+      break;
+  }
+
+  // Sample a Finch-controlled fraction only.
+  double report_chance = 0.2;  // TODO(johnme): Get this from Finch.
+  if (base::RandDouble() >= report_chance)
+    return;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&NotificationImageReporter::ReportNotificationImageOnIO, this,
+                 make_scoped_refptr(g_browser_process->safe_browsing_service()),

[johnme, 2017/01/11 17:50:53]

I feel a little dirty for accessing this global here in order to reach the
SafeBrowsingDatabaseManager which is this class's "uncle". But the ping manager
doesn't seem to have any references to it or the service. Any ideas?

[Peter Beverloo, 2017/01/11 18:23:56]

This question is for Nathan, but I'd love to see a DCHECK() if this is OK.
(Otherwise we may try to invoke on a nullptr on the receiving thread, which
leads to weird stack traces.)

[johnme, 2017/01/11 20:05:07]

I've added a !safe_browsing_service to the condition in
ReportNotificationImageOnIO below. Note that this is an ordinary parameter not
the "this" being used for the PostTask.

[Nathan Parker, 2017/01/11 22:34:38]

This is the common way to get the db_manager, so I think it's reasonable.

+                 origin, image));
+}
+
+void NotificationImageReporter::ReportNotificationImageOnIO(
+    scoped_refptr<SafeBrowsingService> safe_browsing_service,
+    const GURL& origin,
+    const SkBitmap& image) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Skip whitelisted origins to cut down on report volume.
+  if (!safe_browsing_service->database_manager() ||
+      safe_browsing_service->database_manager()->MatchCsdWhitelistUrl(origin))
+    return;

[Peter Beverloo, 2017/01/11 18:23:56]

micro nit: {} as this is considered a multi-line statement

[johnme, 2017/01/11 20:05:07]

Done.

+
+  // 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)
+    return;
+  report_times_.push(base::Time::Now());
+
+  BrowserThread::GetBlockingPool()->PostWorkerTask(
+      FROM_HERE,
+      base::Bind(
+          &NotificationImageReporter::DownscaleNotificationImageOnBlockingPool,
+          this, origin, image));
+}
+
+void NotificationImageReporter::DownscaleNotificationImageOnBlockingPool(
+    const GURL& origin,
+    const SkBitmap& image) {
+  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+
+  // Downscale to fit within 512x512.
+  const double MAX_SIZE = 512;

[Peter Beverloo, 2017/01/11 18:23:56]

nit: maybe this should be finch controllable too?

[johnme, 2017/01/11 20:05:07]

Added TODO.

+  double scale = std::min(MAX_SIZE / image.width(), MAX_SIZE / image.height());
+  SkBitmap downscaled_image =
+      scale >= 1.0 ? image  // already small enough
+                   : 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;
+  if (!gfx::PNGCodec::EncodeBGRASkBitmap(downscaled_image, false, &png_bytes)) {
+    NOTREACHED();

[Peter Beverloo, 2017/01/11 18:23:56]

q: this should only happen for OOM, right?

[johnme, 2017/01/11 20:05:07]

I guess so; don't know enough about how libpng operated under the hood. Also for
empty input, which I don't think can happen here.

+    return;
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &NotificationImageReporter::SendReportOnIO, this, origin,
+          base::RefCountedBytes::TakeVector(&png_bytes),
+          gfx::Size(image.width(), image.height()),
+          gfx::Size(downscaled_image.width(), downscaled_image.height())));
+}
+
+void NotificationImageReporter::SendReportOnIO(
+    const GURL& origin,
+    scoped_refptr<base::RefCountedMemory> png_data,
+    gfx::Size dimensions,
+    gfx::Size original_dimensions) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  NotificationImageReportRequest report;
+  report.set_notification_origin(origin.spec());
+  report.mutable_image()->set_png_data(png_data->front(), png_data->size());
+  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::Closure(),
+                       base::Callback<void(const GURL&, int)>());

[Peter Beverloo, 2017/01/11 18:23:56]

nit: it'd be cool to have UMA for logging (a) the amount of data transferred,
and (b) the time it took for the transfer to complete. Consider a TODO?

[johnme, 2017/01/11 20:05:07]

Added TODO.

+}
+
+}  // namespace safe_browsing