From b4ae989be0618fd1285f206eac3fdf22bd00aa88 Mon Sep 17 00:00:00 2001 From: James Teow Date: Thu, 26 Oct 2023 00:45:34 +0000 Subject: [PATCH] Bug 1855356 - Allow download of collection attachments to re-occur after failure - r=Standard8 Depends on D189992 Differential Revision: https://phabricator.services.mozilla.com/D190247 --- .../search/SearchSERPTelemetry.sys.mjs | 80 ++++- .../test/browser/telemetry/browser.toml | 3 + ...ry_domain_categorization_download_timer.js | 310 ++++++++++++++++++ 3 files changed, 392 insertions(+), 1 deletion(-) create mode 100644 browser/components/search/test/browser/telemetry/browser_search_telemetry_domain_categorization_download_timer.js diff --git a/browser/components/search/SearchSERPTelemetry.sys.mjs b/browser/components/search/SearchSERPTelemetry.sys.mjs index 38504dedb879..35a0c0bd9904 100644 --- a/browser/components/search/SearchSERPTelemetry.sys.mjs +++ b/browser/components/search/SearchSERPTelemetry.sys.mjs @@ -27,6 +27,13 @@ const SEARCH_TELEMETRY_PRIVATE_BROWSING_KEY_SUFFIX = "pb"; // Exported for tests. export const TELEMETRY_SETTINGS_KEY = "search-telemetry-v2"; export const TELEMETRY_CATEGORIZATION_KEY = "search-categorization"; +export const TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS = { + // Units are in milliseconds. + base: 3600000, + minAdjust: 60000, + maxAdjust: 600000, + maxTriesPerSession: 2, +}; const impressionIdsWithoutEngagementsSet = new Set(); @@ -1660,6 +1667,22 @@ class DomainToCategoriesMap { */ #onSettingsSync = null; + /** + * When downloading an attachment from Remote Settings fails, this will + * contain a timer which will eventually attempt to retry downloading + * attachments. + */ + #downloadTimer = null; + + /** + * Number of times this has attempted to try another download. Will reset + * if the categorization preference has been toggled, or a sync event has + * been detected. + * + * @type {number} + */ + #downloadRetries = 0; + /** * Runs at application startup with startup idle tasks. Creates a listener * to changes of the SERP categorization preference. Additionally, if the @@ -1685,7 +1708,11 @@ class DomainToCategoriesMap { uninit() { lazy.logConsole.debug("Un-initialize domain-to-categories map."); if (this.#init) { - this.#clearClientAndMap(); + if (this.#map) { + this.#clearClientAndMap(); + } else { + this.#cancelAndNullifyTimer(); + } this.#init = false; Services.prefs.removeObserver(CATEGORIZATION_PREF, this); } @@ -1699,6 +1726,7 @@ class DomainToCategoriesMap { if (lazy.serpEventTelemetryCategorization) { this.#setupClientAndMap(); } else { + this.#cancelAndNullifyTimer(); this.#clearClientAndMap(); } } @@ -1787,6 +1815,7 @@ class DomainToCategoriesMap { this.#client.off("sync", this.#onSettingsSync); this.#client = null; this.#onSettingsSync = null; + this.#downloadRetries = 0; } if (this.#map) { @@ -1835,6 +1864,11 @@ class DomainToCategoriesMap { toDelete.map(record => this.#client.attachments.deleteDownloaded(record)) ); + // In case a user encountered network failures in the past and kept their + // session on, this will ensure the next sync event will retry downloading + // again in case there's a new download error. + this.#downloadRetries = 0; + this.#clearAndPopulateMap(data?.current); } @@ -1854,6 +1888,7 @@ class DomainToCategoriesMap { // object will be created. this.#map = null; this.#version = null; + this.#cancelAndNullifyTimer(); if (!records?.length) { lazy.logConsole.debug("No records found for domain-to-categories map."); @@ -1868,6 +1903,7 @@ class DomainToCategoriesMap { result = await this.#client.attachments.download(record); } catch (ex) { lazy.logConsole.error("Could not download file:", ex); + this.#createTimerToPopulateMap(); return; } fileContents.push(result.buffer); @@ -1915,6 +1951,48 @@ class DomainToCategoriesMap { }); } } + + #cancelAndNullifyTimer() { + if (this.#downloadTimer) { + lazy.logConsole.debug("Cancel and nullify download timer."); + this.#downloadTimer.cancel(); + this.#downloadTimer = null; + } + } + + #createTimerToPopulateMap() { + if ( + this.#downloadRetries >= + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.maxTriesPerSession + ) { + return; + } + if (!this.#downloadTimer) { + this.#downloadTimer = Cc["@mozilla.org/timer;1"].createInstance( + Ci.nsITimer + ); + } + lazy.logConsole.debug("Create timer to retry downloading attachments."); + let delay = + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.base + + randomInteger( + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.minAdjust, + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.maxAdjust + ); + this.#downloadTimer.initWithCallback( + async () => { + this.#downloadRetries += 1; + let records = await this.#client.get(); + this.#clearAndPopulateMap(records); + }, + delay, + Ci.nsITimer.TYPE_ONE_SHOT + ); + } +} + +function randomInteger(min, max) { + return Math.floor(Math.random() * (max - min + 1)) + min; } export var SearchSERPDomainToCategoriesMap = new DomainToCategoriesMap(); diff --git a/browser/components/search/test/browser/telemetry/browser.toml b/browser/components/search/test/browser/telemetry/browser.toml index 7d2ef6ae60cd..0b3095b4e9b3 100644 --- a/browser/components/search/test/browser/telemetry/browser.toml +++ b/browser/components/search/test/browser/telemetry/browser.toml @@ -46,6 +46,9 @@ support-files = [ "searchTelemetryDomainCategorizationCapProcessedDomains.html", ] +["browser_search_telemetry_domain_categorization_download_timer.js"] +support-files = ["domain_category_mappings.json"] + ["browser_search_telemetry_engagement_cached.js"] support-files = [ "cacheable.html", diff --git a/browser/components/search/test/browser/telemetry/browser_search_telemetry_domain_categorization_download_timer.js b/browser/components/search/test/browser/telemetry/browser_search_telemetry_domain_categorization_download_timer.js new file mode 100644 index 000000000000..f21cae8b6132 --- /dev/null +++ b/browser/components/search/test/browser/telemetry/browser_search_telemetry_domain_categorization_download_timer.js @@ -0,0 +1,310 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +/* + * This test ensures we are correctly restarting a download of an attachment + * after a failure. We simulate failures by not caching the attachment in + * Remote Settings. + */ + +ChromeUtils.defineESModuleGetters(this, { + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS: + "resource:///modules/SearchSERPTelemetry.sys.mjs", + RemoteSettings: "resource://services-settings/remote-settings.sys.mjs", + SearchSERPCategorization: "resource:///modules/SearchSERPTelemetry.sys.mjs", + SearchSERPDomainToCategoriesMap: + "resource:///modules/SearchSERPTelemetry.sys.mjs", + SearchUtils: "resource://gre/modules/SearchUtils.sys.mjs", + sinon: "resource://testing-common/Sinon.sys.mjs", + TELEMETRY_CATEGORIZATION_KEY: + "resource:///modules/SearchSERPTelemetry.sys.mjs", +}); + +const TEST_PROVIDER_INFO = [ + { + telemetryId: "example", + searchPageRegexp: + /^https:\/\/example.org\/browser\/browser\/components\/search\/test\/browser\/telemetry\/searchTelemetry/, + queryParamName: "s", + codeParamName: "abc", + taggedCodes: ["ff"], + adServerAttributes: ["mozAttr"], + nonAdsLinkRegexps: [/^https:\/\/example.com/], + extraAdServersRegexps: [/^https:\/\/example\.com\/ad/], + // The search telemetry entry responsible for targeting the specific results. + domainExtraction: { + ads: [ + { + selectors: "[data-ad-domain]", + method: "data-attribute", + options: { + dataAttributeKey: "adDomain", + }, + }, + { + selectors: ".ad", + method: "href", + options: { + queryParamKey: "ad_domain", + }, + }, + ], + nonAds: [ + { + selectors: "#results .organic a", + method: "href", + }, + ], + }, + components: [ + { + type: SearchSERPTelemetryUtils.COMPONENTS.AD_LINK, + default: true, + }, + ], + }, +]; + +function waitForDownloadError() { + return TestUtils.consoleMessageObserved(msg => { + return ( + typeof msg.wrappedJSObject.arguments?.[0] == "string" && + msg.wrappedJSObject.arguments[0].includes("Could not download file:") + ); + }); +} + +const client = RemoteSettings(TELEMETRY_CATEGORIZATION_KEY); +const db = client.db; + +let stub; +// Shorten the timer so that tests don't have to wait too long. +const TIMEOUT_IN_MS = 250; +add_setup(async function () { + SearchSERPTelemetry.overrideSearchTelemetryForTests(TEST_PROVIDER_INFO); + await waitForIdle(); + + await SpecialPowers.pushPrefEnv({ + set: [["browser.search.log", true]], + }); + + let defaultDownloadSettings = { + ...TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS, + }; + + // Use a much shorter interval from the default preference that when we + // simulate download failures, we don't have to wait long before another + // download attempt. + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.base = TIMEOUT_IN_MS; + + // Normally we add random time to avoid a failure resulting in everyone + // hitting the network at once. For tests, we remove this unless explicitly + // testing. + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.minAdjust = 0; + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.maxAdjust = 0; + + stub = sinon.stub(SearchSERPCategorization, "dummyLogger"); + await db.clear(); + + registerCleanupFunction(async () => { + stub.restore(); + SearchSERPTelemetry.overrideSearchTelemetryForTests(); + resetTelemetry(); + await db.clear(); + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS = { + ...defaultDownloadSettings, + }; + }); +}); + +add_task(async function test_download_after_failure() { + let { record, attachment } = await mockRecordWithAttachment({ + id: "example_id", + version: 1, + filename: "domain_category_mappings.json", + }); + await db.create(record); + await db.importChanges({}, Date.now()); + + let downloadError = waitForDownloadError(); + await SpecialPowers.pushPrefEnv({ + set: [["browser.search.serpEventTelemetryCategorization.enabled", true]], + }); + await downloadError; + + // In between the download failure and one of download retries, cache + // the attachment so that the next download attempt will be successful. + client.attachments.cacheImpl.set(record.id, attachment); + await TestUtils.topicObserved("domain-to-categories-map-update-complete"); + + let url = getSERPUrl("searchTelemetryDomainCategorizationReporting.html"); + info("Load a sample SERP with organic results."); + let promise = waitForPageWithCategorizedDomains(); + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, url); + await promise; + + Assert.deepEqual( + Array.from(stub.getCall(0).args[0]), + ["foobar.org"], + "Categorization of non-ads should match." + ); + + Assert.deepEqual( + Array.from(stub.getCall(1).args[0]), + ["abc.org", "def.org"], + "Categorization of ads should match." + ); + + // Clean up. + BrowserTestUtils.removeTab(tab); + await SpecialPowers.popPrefEnv(); + await db.clear(); + await client.attachments.cacheImpl.delete(record.id); +}); + +add_task(async function test_download_after_multiple_failures() { + let { record } = await mockRecordWithAttachment({ + id: "example_id", + version: 1, + filename: "domain_category_mappings.json", + }); + await db.create(record); + await db.importChanges({}, Date.now()); + + let downloadError = waitForDownloadError(); + await SpecialPowers.pushPrefEnv({ + set: [["browser.search.serpEventTelemetryCategorization.enabled", true]], + }); + await downloadError; + + // Following an initial download failure, the number of allowable retries + // should equal to the maximum number per session. + for ( + let i = 0; + i < TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.maxTriesPerSession; + ++i + ) { + await waitForDownloadError(); + } + + // To ensure we didn't attempt another download, wait more than what another + // download error should take. + let consoleObserved = false; + let timeout = false; + let firstPromise = waitForDownloadError().then(() => { + consoleObserved = true; + }); + let secondPromise = new Promise(resolve => + // eslint-disable-next-line mozilla/no-arbitrary-setTimeout + setTimeout(resolve, TIMEOUT_IN_MS + 100) + ).then(() => (timeout = true)); + await Promise.race([firstPromise, secondPromise]); + Assert.equal(consoleObserved, false, "Encountered download failure"); + Assert.equal(timeout, true, "Timeout occured"); + + Assert.ok(SearchSERPDomainToCategoriesMap.empty, "Map is empty"); + + // Clean up. + await SpecialPowers.popPrefEnv(); + await db.clear(); + await client.attachments.cacheImpl.delete(record.id); +}); + +add_task(async function test_cancel_download_timer() { + let { record } = await mockRecordWithAttachment({ + id: "example_id", + version: 1, + filename: "domain_category_mappings.json", + }); + await db.create(record); + await db.importChanges({}, Date.now()); + + let downloadError = waitForDownloadError(); + await SpecialPowers.pushPrefEnv({ + set: [["browser.search.serpEventTelemetryCategorization.enabled", true]], + }); + await downloadError; + + // Changing the gating preference to false before the map is populated + // should cancel the download timer. + let observeCancel = TestUtils.consoleMessageObserved(msg => { + return ( + typeof msg.wrappedJSObject.arguments?.[0] == "string" && + msg.wrappedJSObject.arguments[0].includes( + "Cancel and nullify download timer." + ) + ); + }); + await SpecialPowers.popPrefEnv(); + await observeCancel; + + // To ensure we don't attempt another download, wait a bit over how long the + // the download error should take. + let consoleObserved = false; + let timeout = false; + let firstPromise = waitForDownloadError().then(() => { + consoleObserved = true; + }); + let secondPromise = new Promise(resolve => + // eslint-disable-next-line mozilla/no-arbitrary-setTimeout + setTimeout(resolve, TIMEOUT_IN_MS + 100) + ).then(() => (timeout = true)); + await Promise.race([firstPromise, secondPromise]); + Assert.equal(consoleObserved, false, "Encountered download failure"); + Assert.equal(timeout, true, "Timeout occured"); + Assert.ok(SearchSERPDomainToCategoriesMap.empty, "Map is empty"); + + // Clean up. + await client.attachments.cacheImpl.delete(record.id); + await db.clear(); +}); + +add_task(async function test_download_adjust() { + // To test that we're actually adding a random delay to the base value, + // we set the base number to zero so that the next attempt should be + // instant but we'll wait in between 0 and 1000ms and expect the + // timer to elapse first. + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.base = 0; + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.minAdjust = 1000; + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.maxAdjust = 1000; + + let { record } = await mockRecordWithAttachment({ + id: "example_id", + version: 1, + filename: "domain_category_mappings.json", + }); + await db.create(record); + await db.importChanges({}, Date.now()); + + let downloadError = waitForDownloadError(); + await SpecialPowers.pushPrefEnv({ + set: [["browser.search.serpEventTelemetryCategorization.enabled", true]], + }); + await downloadError; + + // The timer should finish before the next error. + let consoleObserved = false; + let timeout = false; + let firstPromise = waitForDownloadError().then(() => { + consoleObserved = true; + }); + let secondPromise = new Promise(resolve => + // eslint-disable-next-line mozilla/no-arbitrary-setTimeout + setTimeout(resolve, 250) + ).then(() => (timeout = true)); + await Promise.race([firstPromise, secondPromise]); + Assert.equal(timeout, true, "Timeout occured"); + Assert.equal(consoleObserved, false, "Encountered download failure"); + + await firstPromise; + Assert.equal(consoleObserved, true, "Encountered download failure"); + + // Clean up. + await client.attachments.cacheImpl.delete(record.id); + await db.clear(); + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.base = TIMEOUT_IN_MS; + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.minAdjust = 0; + TELEMETRY_CATEGORIZATION_DOWNLOAD_SETTINGS.maxAdjust = 0; +});