From 280083669e402711867120ad2ea540342a704710 Mon Sep 17 00:00:00 2001
From: Mathieu Leplatre <mathieu@mozilla.com>
Date: Fri, 22 Nov 2019 08:35:30 +0000
Subject: [PATCH] Bug 1597337 - Report download errors in uptake telemetry
 r=Gijs

Differential Revision: https://phabricator.services.mozilla.com/D53783

--HG--
extra : moz-landing-system : lando
---
 netwerk/dns/PublicSuffixList.jsm              | 15 ++++++++-
 services/settings/RemoteSettingsClient.jsm    | 31 ++++++++++++++++-
 .../test/unit/test_attachments_downloader.js  | 33 +++++++++++++++++++
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/netwerk/dns/PublicSuffixList.jsm b/netwerk/dns/PublicSuffixList.jsm
index b662f8c8cc7b..ac27b3bcc84a 100644
--- a/netwerk/dns/PublicSuffixList.jsm
+++ b/netwerk/dns/PublicSuffixList.jsm
@@ -19,6 +19,12 @@ const PublicSuffixList = {
   CLIENT: RemoteSettings("public-suffix-list"),
 
   init() {
+    // Only initialize once.
+    if (this._initialized) {
+      return;
+    }
+    this._initialized = true;
+
     this.CLIENT.on("sync", this.onUpdate.bind(this));
     /* We have a single record for this collection. Let's see if we already have it locally.
      * Note that on startup, we don't need to synchronize immediately on new profiles.
@@ -26,6 +32,7 @@ const PublicSuffixList = {
     this.CLIENT.get({ syncIfEmpty: false, filters: { id: RECORD_ID } })
       .then(async records => {
         if (records.length == 1) {
+          // Get the downloaded file URI (most likely to be a no-op here, since file will exist).
           const fileURI = await this.CLIENT.attachments.download(records[0]);
           // Send a signal so that the C++ code loads the updated list on startup.
           this.notifyUpdate(fileURI);
@@ -78,7 +85,13 @@ const PublicSuffixList = {
       return;
     }
     // Download the updated file.
-    const fileURI = await this.CLIENT.attachments.download(changed[0]);
+    let fileURI;
+    try {
+      fileURI = await this.CLIENT.attachments.download(changed[0]);
+    } catch (err) {
+      Cu.reportError(err);
+      return;
+    }
     // Notify the C++ part to reload it from disk.
     this.notifyUpdate(fileURI);
   },
diff --git a/services/settings/RemoteSettingsClient.jsm b/services/settings/RemoteSettingsClient.jsm
index 404fc6c5d831..5d60027d9ead 100644
--- a/services/settings/RemoteSettingsClient.jsm
+++ b/services/settings/RemoteSettingsClient.jsm
@@ -174,6 +174,35 @@ class UnknownCollectionError extends Error {
   }
 }
 
+class AttachmentDownloader extends Downloader {
+  constructor(client) {
+    super(client.bucketName, client.collectionName);
+    this._client = client;
+  }
+
+  /**
+   * Download attachment and report Telemetry on failure.
+   *
+   * @see Downloader.download
+   */
+  async download(record, options) {
+    try {
+      // Explicitly await here to ensure we catch a network error.
+      return await super.download(record, options);
+    } catch (err) {
+      // Report download error.
+      const status = /NetworkError/.test(err.message)
+        ? UptakeTelemetry.STATUS.NETWORK_ERROR
+        : UptakeTelemetry.STATUS.DOWNLOAD_ERROR;
+      // If the file failed to be downloaded, report it as such in Telemetry.
+      await UptakeTelemetry.report(TELEMETRY_COMPONENT, status, {
+        source: this._client.identifier,
+      });
+      throw err;
+    }
+  }
+}
+
 class RemoteSettingsClient extends EventEmitter {
   static get InvalidSignatureError() {
     return InvalidSignatureError;
@@ -232,7 +261,7 @@ class RemoteSettingsClient extends EventEmitter {
     XPCOMUtils.defineLazyGetter(
       this,
       "attachments",
-      () => new Downloader(this.bucketName, collectionName)
+      () => new AttachmentDownloader(this)
     );
   }
 
diff --git a/services/settings/test/unit/test_attachments_downloader.js b/services/settings/test/unit/test_attachments_downloader.js
index e9966d76a721..25a25ff10e68 100644
--- a/services/settings/test/unit/test_attachments_downloader.js
+++ b/services/settings/test/unit/test_attachments_downloader.js
@@ -4,9 +4,15 @@ const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { RemoteSettings } = ChromeUtils.import(
   "resource://services-settings/remote-settings.js"
 );
+const { UptakeTelemetry } = ChromeUtils.import(
+  "resource://services-common/uptake-telemetry.js"
+);
 const { Downloader } = ChromeUtils.import(
   "resource://services-settings/Attachments.jsm"
 );
+const { TelemetryTestUtils } = ChromeUtils.import(
+  "resource://testing-common/TelemetryTestUtils.jsm"
+);
 const { OS } = ChromeUtils.import("resource://gre/modules/osfile.jsm");
 
 const RECORD = {
@@ -196,3 +202,30 @@ add_task(async function test_downloader_is_accessible_via_client() {
   );
 });
 add_task(clear_state);
+
+add_task(async () => {
+  const client = RemoteSettings("some-collection");
+
+  const record = {
+    attachment: {
+      ...RECORD.attachment,
+      location: "404-error.pem",
+    },
+  };
+
+  try {
+    await client.attachments.download(record, { retry: 0 });
+  } catch (e) {}
+
+  TelemetryTestUtils.assertEvents([
+    [
+      "uptake.remotecontent.result",
+      "uptake",
+      "remotesettings",
+      UptakeTelemetry.STATUS.DOWNLOAD_ERROR,
+      {
+        source: client.identifier,
+      },
+    ],
+  ]);
+});