commit 849adf87f73d5c28963793599549b618d0439495 parent 55acbeb41cbe0bc91f93e773f163c14bbbefef7c Author: Mark Banner <standard8@mozilla.com> Date: Tue, 18 Nov 2025 18:46:49 +0000 Bug 1999462 - Remove OHTTP Search Suggestion code. r=adw,urlbar-reviewers Differential Revision: https://phabricator.services.mozilla.com/D272687 Diffstat:
20 files changed, 42 insertions(+), 1737 deletions(-)
diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js @@ -895,12 +895,6 @@ pref("browser.search.totalSearches", 0); // Feature gate for visual search. pref("browser.search.visualSearch.featureGate", true); -// Feature gate for ohttp based suggestions. -pref("browser.search.suggest.ohttp.featureGate", false); - -// User preference to enable/disable ohttp based suggestions. -pref("browser.search.suggest.ohttp.enabled", true); - // Spin the cursor while the page is loading pref("browser.spin_cursor_while_busy", false); diff --git a/browser/components/search/content/searchbar.js b/browser/components/search/content/searchbar.js @@ -584,8 +584,6 @@ // If the input field is still focused then a different window has // received focus, ignore the next focus event. this._ignoreFocus = document.activeElement == this._textbox; - - this.textbox.mController.resetSession(); }, true ); diff --git a/browser/components/search/test/browser/browser.toml b/browser/components/search/test/browser/browser.toml @@ -124,8 +124,6 @@ skip-if = [ "tsan", # Bug 1792718 ] -["browser_searchbar_ohttpSessions.js"] - ["browser_searchbar_openpopup.js"] ["browser_searchbar_results.js"] diff --git a/browser/components/search/test/browser/browser_searchbar_ohttpSessions.js b/browser/components/search/test/browser/browser_searchbar_ohttpSessions.js @@ -1,220 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -// End-to-end browser smoke test for Search Suggestion OHTTP sessions in the -// search bar. More comprehensive tests are in test_quicksuggest_merinoSessions.js. -// See also ../quicksuggest/browser/browser_quicksuggest_merinoSessions.js. - -"use strict"; - -const ENGINE_ID = "suggestions-engine-test"; -const CONFIG = [ - { - identifier: ENGINE_ID, - base: { - name: "other", - urls: { - suggestions: { - base: "https://example.com", - params: [ - { - name: "parameter", - value: "135246", - }, - ], - searchTermParamName: "suggest", - }, - }, - }, - }, -]; - -const { MerinoTestUtils } = ChromeUtils.importESModule( - "resource://testing-common/MerinoTestUtils.sys.mjs" -); -const { ObliviousHTTP } = ChromeUtils.importESModule( - "resource://gre/modules/ObliviousHTTP.sys.mjs" -); -const { SearchSuggestionController } = ChromeUtils.importESModule( - "moz-src:///toolkit/components/search/SearchSuggestionController.sys.mjs" -); - -const searchPopup = document.getElementById("PopupSearchAutoComplete"); -let searchBar; - -add_setup(async function () { - await SpecialPowers.pushPrefEnv({ - set: [ - ["browser.search.suggest.enabled", true], - ["browser.urlbar.suggest.searches", true], - ["browser.search.suggest.ohttp.featureGate", true], - ["browser.urlbar.merino.ohttpConfigURL", "https://example.com/config"], - ["browser.urlbar.merino.ohttpRelayURL", "https://example.com/relay"], - ["browser.urlbar.merino.endpointURL", "https://example.com/endpoint"], - ], - }); - - await gCUITestUtils.addSearchBar(); - await clearSearchbarHistory(); - - await PlacesUtils.history.clear(); - await PlacesUtils.bookmarks.eraseEverything(); - await UrlbarTestUtils.formHistory.clear(); - - await SearchTestUtils.updateRemoteSettingsConfig(CONFIG); - - SearchSuggestionController.oHTTPEngineId = ENGINE_ID; - - sinon.stub(ObliviousHTTP, "getOHTTPConfig").resolves({}); - sinon - .stub(ObliviousHTTP, "ohttpRequest") - .callsFake((relayUrl, config, url) => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [ - { - title: "", - url: "https://merino.services.mozilla.com", - provider: "google_suggest", - is_sponsored: false, - score: 1, - custom_details: { - google_suggest: { - suggestions: [new URL(url).searchParams.get("q"), []], - }, - }, - }, - ], - }), - ok: true, - }; - }); - - searchBar = window.document.getElementById("searchbar"); - - registerCleanupFunction(async () => { - await clearSearchbarHistory(); - gCUITestUtils.removeSearchBar(); - sinon.restore(); - }); -}); - -async function checkAndClearRequests(expectedParams) { - await TestUtils.waitForCondition( - () => ObliviousHTTP.ohttpRequest.callCount == 1, - "Should have recieved an OHTTP search" - ); - - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should request via OHTTP once per search" - ); - let args = ObliviousHTTP.ohttpRequest.firstCall.args; - Assert.deepEqual( - args[0], - "https://example.com/relay", - "Should have called the Relay URL" - ); - let url = new URL(args[2]); - Assert.deepEqual( - url.origin + url.pathname, - Services.prefs.getCharPref("browser.urlbar.merino.endpointURL"), - "Should have the correct URL base" - ); - for (let [paramName, paramValue] of Object.entries(expectedParams)) { - Assert.equal( - url.searchParams.get(paramName), - paramValue, - `Should have the expected parameter value for ${paramName}` - ); - } - - ObliviousHTTP.ohttpRequest.resetHistory(); -} - -// In a single engagement, all requests should use the same session ID and the -// sequence number should be incremented. -add_task(async function singleEngagement() { - for (let i = 0; i < 3; i++) { - let searchString = "search" + i; - await searchInSearchbar(searchString, window, i != 0); - - await checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: i, - }); - } - - // End the engagement to reset the session for the next test. - gURLBar.focus(); -}); - -// In a single engagement, all requests should use the same session ID and the -// sequence number should be incremented. This task closes the panel between -// searches but keeps the input focused, so the engagement should not end. -add_task(async function singleEngagement_panelClosed() { - for (let i = 0; i < 3; i++) { - let searchString = "search" + i; - await searchInSearchbar(searchString, window); - - await checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: i, - }); - - let promise = promiseEvent(searchPopup, "popuphidden"); - EventUtils.synthesizeKey("KEY_Escape"); - searchPopup.hidePopup(); - await promise; - Assert.ok(searchBar.textbox.focused, "Input remains focused"); - } - - // End the engagement to reset the session for the next test. - searchBar.blur(); -}); - -// New engagements should not use the same session ID as previous engagements -// and the sequence number should be reset. This task completes each engagement -// successfully. -add_task(async function manyEngagements_engagement() { - for (let i = 0; i < 3; i++) { - // Open a new tab since we'll load the mock default search engine page. - await BrowserTestUtils.withNewTab("about:blank", async () => { - let searchString = "search" + i; - await searchInSearchbar(searchString, window); - - await checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: 0, - }); - - // Press enter on the heuristic result to load the search engine page and - // complete the engagement. - let loadPromise = BrowserTestUtils.browserLoaded( - gBrowser.selectedBrowser - ); - EventUtils.synthesizeKey("KEY_Enter"); - await loadPromise; - }); - } -}); - -// New engagements should not use the same session ID as previous engagements -// and the sequence number should be reset. This task abandons each engagement. -add_task(async function manyEngagements_abandonment() { - for (let i = 0; i < 3; i++) { - let searchString = "search" + i; - await searchInSearchbar(searchString, window); - - await checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: 0, - }); - - // Focus the URLBar to abandon the engagement. - gURLBar.focus(); - } -}); diff --git a/browser/components/search/test/browser/head.js b/browser/components/search/test/browser/head.js @@ -15,7 +15,6 @@ ChromeUtils.defineESModuleGetters(this, { TelemetryTestUtils: "resource://testing-common/TelemetryTestUtils.sys.mjs", UrlbarSearchUtils: "moz-src:///browser/components/urlbar/UrlbarSearchUtils.sys.mjs", - sinon: "resource://testing-common/Sinon.sys.mjs", }); ChromeUtils.defineLazyGetter(this, "UrlbarTestUtils", () => { diff --git a/browser/components/urlbar/MerinoClient.sys.mjs b/browser/components/urlbar/MerinoClient.sys.mjs @@ -57,17 +57,6 @@ export class MerinoClient { } /** - * @returns {boolean} - * Returns true if the OHTTP Prefs are defined for use. - */ - static get hasOHTTPPrefs() { - return ( - lazy.UrlbarPrefs.get("merinoOhttpConfigURL") && - lazy.UrlbarPrefs.get("merinoOhttpRelayURL") - ); - } - - /** * @param {string} [name] * An optional name for the client. It will be included in log messages. * @param {object} [options] diff --git a/browser/components/urlbar/UrlbarProviderSearchSuggestions.sys.mjs b/browser/components/urlbar/UrlbarProviderSearchSuggestions.sys.mjs @@ -300,20 +300,6 @@ export class UrlbarProviderSearchSuggestions extends UrlbarProvider { } /** - * Called when a search session concludes regardless of how it ends - - * whether through engagement or abandonment or otherwise. This is - * called for all providers who have implemented this method. - * - * @param {UrlbarQueryContext} _queryContext - * The current query context. - * @param {UrlbarController} _controller - * The associated controller. - */ - onSearchSessionEnd(_queryContext, _controller) { - this.#suggestionsController?.resetSession(); - } - - /** * Gets the provider's priority. * * @param {UrlbarQueryContext} queryContext The query context object diff --git a/browser/components/urlbar/tests/browser/browser.toml b/browser/components/urlbar/tests/browser/browser.toml @@ -570,12 +570,6 @@ support-files = [ "searchSuggestionEngine.sjs", ] -["browser_searchSuggestions_ohttpSessions.js"] -support-files = [ - "searchSuggestionEngine.xml", - "searchSuggestionEngine.sjs", -] - ["browser_searchTelemetry.js"] support-files = [ "searchSuggestionEngine.xml", diff --git a/browser/components/urlbar/tests/browser/browser_searchSuggestions_ohttpSessions.js b/browser/components/urlbar/tests/browser/browser_searchSuggestions_ohttpSessions.js @@ -1,220 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -// End-to-end browser smoke test for Search Suggestion OHTTP sessions. More -// comprehensive tests are in test_quicksuggest_merinoSessions.js. -// See also ../quicksuggest/browser/browser_quicksuggest_merinoSessions.js. - -"use strict"; - -const ENGINE_ID = "suggestions-engine-test"; -const CONFIG = [ - { - identifier: ENGINE_ID, - base: { - name: "other", - urls: { - suggestions: { - base: "https://example.com", - params: [ - { - name: "parameter", - value: "135246", - }, - ], - searchTermParamName: "suggest", - }, - }, - }, - }, -]; - -const { MerinoTestUtils } = ChromeUtils.importESModule( - "resource://testing-common/MerinoTestUtils.sys.mjs" -); -const { ObliviousHTTP } = ChromeUtils.importESModule( - "resource://gre/modules/ObliviousHTTP.sys.mjs" -); -const { SearchSuggestionController } = ChromeUtils.importESModule( - "moz-src:///toolkit/components/search/SearchSuggestionController.sys.mjs" -); - -SearchTestUtils.init(this); - -add_setup(async function () { - await SpecialPowers.pushPrefEnv({ - set: [ - ["browser.search.suggest.enabled", true], - ["browser.urlbar.suggest.searches", true], - ["browser.search.suggest.ohttp.featureGate", true], - ["browser.urlbar.merino.ohttpConfigURL", "https://example.com/config"], - ["browser.urlbar.merino.ohttpRelayURL", "https://example.com/relay"], - ["browser.urlbar.merino.endpointURL", "https://example.com/endpoint"], - ], - }); - - await PlacesUtils.history.clear(); - await PlacesUtils.bookmarks.eraseEverything(); - await UrlbarTestUtils.formHistory.clear(); - - await SearchTestUtils.updateRemoteSettingsConfig(CONFIG); - - SearchSuggestionController.oHTTPEngineId = ENGINE_ID; - - sinon.stub(ObliviousHTTP, "getOHTTPConfig").resolves({}); - sinon - .stub(ObliviousHTTP, "ohttpRequest") - .callsFake((relayUrl, config, url) => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [ - { - title: "", - url: "https://merino.services.mozilla.com", - provider: "google_suggest", - is_sponsored: false, - score: 1, - custom_details: { - google_suggest: { - suggestions: [new URL(url).searchParams.get("q"), []], - }, - }, - }, - ], - }), - ok: true, - }; - }); - - registerCleanupFunction(async () => { - sinon.restore(); - }); -}); - -function checkAndClearRequests(expectedParams) { - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should request via OHTTP once per search" - ); - let args = ObliviousHTTP.ohttpRequest.firstCall.args; - Assert.deepEqual( - args[0], - "https://example.com/relay", - "Should have called the Relay URL" - ); - let url = new URL(args[2]); - Assert.deepEqual( - url.origin + url.pathname, - Services.prefs.getCharPref("browser.urlbar.merino.endpointURL"), - "Should have the correct URL base" - ); - for (let [paramName, paramValue] of Object.entries(expectedParams)) { - Assert.equal( - url.searchParams.get(paramName), - paramValue, - "Should have the expected parameter" - ); - } - - ObliviousHTTP.ohttpRequest.resetHistory(); -} - -// In a single engagement, all requests should use the same session ID and the -// sequence number should be incremented. -add_task(async function singleEngagement() { - for (let i = 0; i < 3; i++) { - let searchString = "search" + i; - await UrlbarTestUtils.promiseAutocompleteResultPopup({ - window, - value: searchString, - fireInputEvent: true, - }); - - checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: i, - }); - } - - await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur()); -}); - -// In a single engagement, all requests should use the same session ID and the -// sequence number should be incremented. This task closes the panel between -// searches but keeps the input focused, so the engagement should not end. -add_task(async function singleEngagement_panelClosed() { - for (let i = 0; i < 3; i++) { - let searchString = "search" + i; - await UrlbarTestUtils.promiseAutocompleteResultPopup({ - window, - value: searchString, - fireInputEvent: true, - }); - - checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: i, - }); - - EventUtils.synthesizeKey("KEY_Escape"); - Assert.ok(!UrlbarTestUtils.isPopupOpen(window), "Panel is closed"); - Assert.ok(gURLBar.focused, "Input remains focused"); - } - - // End the engagement to reset the session for the next test. - gURLBar.blur(); -}); - -// New engagements should not use the same session ID as previous engagements -// and the sequence number should be reset. This task completes each engagement -// successfully. -add_task(async function manyEngagements_engagement() { - for (let i = 0; i < 3; i++) { - // Open a new tab since we'll load the mock default search engine page. - await BrowserTestUtils.withNewTab("about:blank", async () => { - let searchString = "search" + i; - await UrlbarTestUtils.promiseAutocompleteResultPopup({ - window, - value: searchString, - fireInputEvent: true, - }); - - checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: 0, - }); - - // Press enter on the heuristic result to load the search engine page and - // complete the engagement. - let loadPromise = BrowserTestUtils.browserLoaded( - gBrowser.selectedBrowser - ); - EventUtils.synthesizeKey("KEY_Enter"); - await loadPromise; - }); - } -}); - -// New engagements should not use the same session ID as previous engagements -// and the sequence number should be reset. This task abandons each engagement. -add_task(async function manyEngagements_abandonment() { - for (let i = 0; i < 3; i++) { - let searchString = "search" + i; - await UrlbarTestUtils.promiseAutocompleteResultPopup({ - window, - value: searchString, - fireInputEvent: true, - }); - - checkAndClearRequests({ - [MerinoTestUtils.SEARCH_PARAMS.QUERY]: searchString, - [MerinoTestUtils.SEARCH_PARAMS.SEQUENCE_NUMBER]: 0, - }); - - // Blur the urlbar to abandon the engagement. - await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur()); - } -}); diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js @@ -3466,7 +3466,6 @@ pref("browser.search.log", false); pref("browser.search.update", true); pref("browser.search.suggest.enabled", true); pref("browser.search.suggest.enabled.private", false); -pref("browser.search.suggest.ohttp.enabled", false); pref("browser.search.separatePrivateDefault", true); pref("browser.search.separatePrivateDefault.ui.enabled", false); pref("browser.search.removeEngineInfobar.enabled", true); diff --git a/toolkit/components/nimbus/FeatureManifest.yaml b/toolkit/components/nimbus/FeatureManifest.yaml @@ -360,14 +360,6 @@ urlbar: type: int fallbackPref: browser.urlbar.merino.timeoutMs description: Timeout for Merino fetches (ms) - suggestOhttp: - type: boolean - description: >- - Whether search suggestions should be fetched over OHTTP. - Currently this applies only to Google suggestions. - setPref: - pref: browser.search.suggest.ohttp.featureGate - branch: user exposureResults: type: string setPref: diff --git a/toolkit/components/search/SearchService.sys.mjs b/toolkit/components/search/SearchService.sys.mjs @@ -1620,11 +1620,6 @@ export class SearchService { onUpdate: () => this._maybeReloadEngines(Ci.nsISearchService.CHANGE_REASON_EXPERIMENT), }, - suggestOhttpEnabled: { - pref: "browser.search.suggest.ohttp.enabled", - default: false, - onUpdate: this.#recordPreferencesTelemetry.bind(this), - }, }); /** @@ -1735,7 +1730,6 @@ export class SearchService { Glean.searchService.startupTime.stopAndAccumulate(timerId); this.#recordDefaultEngineTelemetryData(); - this.#recordPreferencesTelemetry(); Services.obs.notifyObservers( null, @@ -3679,15 +3673,6 @@ export class SearchService { } /** - * Records in telemetry any user preferences that we monitor. - */ - #recordPreferencesTelemetry() { - Glean.searchSuggestionsOhttp.enabled.set( - this.#lazyPrefs.suggestOhttpEnabled - ); - } - - /** * Records the user's current default engine (normal and private) data to * telemetry. */ diff --git a/toolkit/components/search/SearchSuggestionController.sys.mjs b/toolkit/components/search/SearchSuggestionController.sys.mjs @@ -7,18 +7,10 @@ import { XPCOMUtils } from "resource://gre/modules/XPCOMUtils.sys.mjs"; const DEFAULT_FORM_HISTORY_PARAM = "searchbar-history"; const HTTP_OK = 200; const REMOTE_TIMEOUT_DEFAULT = 500; -// If this value is updated, the labels for the -// `search.suggestions.ohttp.request_counter` metric should also be updated. -const MAX_OHTTP_FAILURES_BEFORE_FALLBACK = 5; -// Minimum of 2 hours once the fallback is activated. -const MIN_DURATION_FOR_FALLBACK_MS = 2 * 60 * 60 * 1000; const lazy = XPCOMUtils.declareLazy({ FormHistory: "resource://gre/modules/FormHistory.sys.mjs", SearchUtils: "moz-src:///toolkit/components/search/SearchUtils.sys.mjs", - // This is currently only used within experiment code. - // eslint-disable-next-line mozilla/no-browser-refs-in-toolkit - MerinoClient: "moz-src:///browser/components/urlbar/MerinoClient.sys.mjs", logConsole: () => console.createInstance({ prefix: "SearchSuggestionController", @@ -41,14 +33,6 @@ const lazy = XPCOMUtils.declareLazy({ pref: "browser.search.suggest.timeout", default: REMOTE_TIMEOUT_DEFAULT, }, - ohttpFeatureGateEnabled: { - pref: "browser.search.suggest.ohttp.featureGate", - default: false, - }, - ohttpEnabled: { - pref: "browser.search.suggest.ohttp.enabled", - default: false, - }, }); /** @@ -62,10 +46,6 @@ const lazy = XPCOMUtils.declareLazy({ */ /** - * @import {MerinoClient} from "moz-src:///browser/components/urlbar/MerinoClient.sys.mjs" - */ - -/** * @typedef {object} SuggestionFetchOptions * @property {string} searchString * The term to provide suggestions for. @@ -260,12 +240,6 @@ export class SearchSuggestionController { } /** - * The maximum number of OHTTP failures before we'll fallback to direct HTTP. - */ - static MAX_OHTTP_FAILURES_BEFORE_FALLBACK = - MAX_OHTTP_FAILURES_BEFORE_FALLBACK; - - /** * The maximum number of local form history results to return. This limit is * only enforced if remote results are also returned. * @@ -283,12 +257,6 @@ export class SearchSuggestionController { maxRemoteResults = 10; /** - * The identifier of the search engine that can currently be enabled for - * OHTTP. May be overridden for tests. - */ - static oHTTPEngineId = "google"; - - /** * The additional parameter used when searching form history. * * @type {string} @@ -435,38 +403,11 @@ export class SearchSuggestionController { } /** - * Should be called at the end of a search engagement (e.g. on blur / search - * complete), to reset the Merino session. - */ - resetSession() { - this.#merino?.resetSession(); - } - - /** * @type {SuggestionRequestContext} */ #context; /** - * @type {MerinoClient} - * The MerinoClient associated with any ObliviousHTTP requests. - */ - #merino; - - /** - * @type {number} - * The count of failed OHTTP requests. - */ - #ohttpFailedRequestCount = 0; - - /** - * @type {?number} - * The fractional number of milliseconds from process startup, see - * ChromeUtils.now(). Exposed for tests. - */ - _ohttpLastFailureTimeMs; - - /** * Fetches search suggestions from the form history. * * @param {SuggestionRequestContext} context @@ -497,42 +438,25 @@ export class SearchSuggestionController { * * @param {SuggestionRequestContext} context * The search context. - * @param {boolean} usedOHTTP - * True if OHTTP was used for the suggestion request. */ - #reportTelemetryForEngine(context, usedOHTTP) { + #reportTelemetryForEngine(context) { // If the timer id has been reset, then we have already handled telemetry. // This might occur in the context of an abort or or cancel. if (context.gleanTimerId) { - let category = usedOHTTP - ? Glean.searchSuggestionsOhttp - : Glean.searchSuggestions; let engineId = context.engine.isConfigEngine ? context.engine.id : "other"; // Stop the latency stopwatch. if (context.aborted) { - category.latency[engineId].cancel(context.gleanTimerId); + Glean.searchSuggestions.latency[engineId].cancel(context.gleanTimerId); } else { - category.latency[engineId].stopAndAccumulate(context.gleanTimerId); + Glean.searchSuggestions.latency[engineId].stopAndAccumulate( + context.gleanTimerId + ); } context.gleanTimerId = 0; - if (usedOHTTP) { - if (context.aborted) { - Glean.searchSuggestionsOhttp.requestCounter - .get(engineId, "aborted") - .add(1); - } else if (context.errorWasReceived) { - Glean.searchSuggestionsOhttp.requestCounter - .get(engineId, "failed" + this.#ohttpFailedRequestCount) - .add(1); - } else { - Glean.searchSuggestionsOhttp.requestCounter - .get(engineId, "success") - .add(1); - } - } else if (context.engine.isConfigEngine) { + if (context.engine.isConfigEngine) { if (context.aborted) { Glean.searchSuggestions.abortedRequests[context.engine.id].add(); } else if (context.errorWasReceived) { @@ -562,46 +486,6 @@ export class SearchSuggestionController { : lazy.SearchUtils.URL_TYPE.TRENDING_JSON ); - // Note: when we enable this for all engines, we need to make sure we have - // the capability for POST submissions handled. - if ( - lazy.ohttpFeatureGateEnabled && - lazy.ohttpEnabled && - lazy.MerinoClient.hasOHTTPPrefs && - context.engine.id == SearchSuggestionController.oHTTPEngineId - ) { - let expiredLastFailure = - ChromeUtils.now() - this._ohttpLastFailureTimeMs > - MIN_DURATION_FOR_FALLBACK_MS; - if ( - this.#ohttpFailedRequestCount < MAX_OHTTP_FAILURES_BEFORE_FALLBACK || - expiredLastFailure - ) { - if (expiredLastFailure) { - this.#ohttpFailedRequestCount = 0; - } - return this.#fetchRemoteObliviousHTTP(context, submission); - } - lazy.logConsole.debug( - "Maximum failures reached, falling back to direct HTTP" - ); - } - return this.#fetchRemoteNormalHTTP(context, submission); - } - - /** - * Fetch suggestions from the search engine over the network using normal - * HTTP(s). - * - * @param {SuggestionRequestContext} context - * The search context. - * @param {nsISearchSubmission} submission - * The submission URL and data for the search suggestion. - * @returns {Promise} - * Returns a promise that is resolved when the response is received, or - * rejected if there is an error. - */ - #fetchRemoteNormalHTTP(context, submission) { let deferredResponse = Promise.withResolvers(); let request = (context.request = new XMLHttpRequest()); // Expect the response type to be JSON, so that the network layer will @@ -643,7 +527,7 @@ export class SearchSuggestionController { request.addEventListener("load", () => { context.timer.cancel(); - this.#reportTelemetryForEngine(context, false); + this.#reportTelemetryForEngine(context); if (!this.#context || context != this.#context || context.aborted) { deferredResponse.resolve( "Got HTTP response after the request was cancelled" @@ -676,7 +560,7 @@ export class SearchSuggestionController { request.addEventListener("error", () => { this.#context.errorWasReceived = true; - this.#reportTelemetryForEngine(context, false); + this.#reportTelemetryForEngine(context); deferredResponse.resolve("HTTP error"); }); @@ -684,7 +568,7 @@ export class SearchSuggestionController { // shouldn't return local or remote results for existing searches. request.addEventListener("abort", () => { context.timer.cancel(); - this.#reportTelemetryForEngine(context, false); + this.#reportTelemetryForEngine(context); deferredResponse.reject( `HTTP request aborted for ${submission.uri.spec}}` ); @@ -703,77 +587,6 @@ export class SearchSuggestionController { return deferredResponse.promise; } - - /** - * Fetch suggestions from the search engine over the network using Oblivious - * HTTP. - * - * POST submissions are not currently supported. - * - * @param {SuggestionRequestContext} context - * The search context. - * @param {nsISearchSubmission} submission - * The submission URL and data for the search suggestion. - * @returns {Promise} - * Returns a promise that is resolved when the response is received, or - * rejected if there is an error. - */ - async #fetchRemoteObliviousHTTP(context, submission) { - if (!this.#merino) { - this.#merino = new lazy.MerinoClient("SearchSuggestions", { - allowOhttp: true, - }); - } - - let submissionURL = URL.fromURI(submission.uri); - - lazy.logConsole.debug(`OHTTP request started for ${submission.uri.spec}`); - - context.gleanTimerId = - Glean.searchSuggestionsOhttp.latency[ - context.engine.isConfigEngine ? context.engine.id : "other" - ].start(); - - let merinoSuggestions = await this.#merino.fetch({ - query: context.searchString, - providers: ["google_suggest"], - timeoutMs: lazy.remoteTimeout, - otherParams: { - google_suggest_params: submissionURL.searchParams.toString(), - }, - }); - - if (!this.#context || context != this.#context || context.aborted) { - this.#reportTelemetryForEngine(context, true); - return "Got OHTTP response after the request was cancelled"; - } - - // The last fetch status covers errors as well as "no_suggestions". Currently - // Merino will return no suggestions if it receives an error response from - // the search engine, hence we have to handle that case here as well. - // The suggestions list here is different from the search engine suggestions - // which are live within the specific provider record within `merinoSuggestions`. - if (this.#merino.lastFetchStatus != "success") { - this.#context.errorWasReceived = true; - this.#ohttpFailedRequestCount++; - this.#reportTelemetryForEngine(context, true); - this._ohttpLastFailureTimeMs = ChromeUtils.now(); - return "No suggestions received from Merino, the search engine probably failed to respond"; - } - - this.#reportTelemetryForEngine(context, true); - this.#ohttpFailedRequestCount = 0; - this._ohttpLastFailureTimeMs = undefined; - - return new Promise(resolve => { - this.#onRemoteLoaded( - context, - merinoSuggestions[0]?.custom_details?.google_suggest?.suggestions || [], - resolve - ); - }); - } - /** * Called when the request completed successfully so we can handle the * response data. diff --git a/toolkit/components/search/SearchSuggestions.sys.mjs b/toolkit/components/search/SearchSuggestions.sys.mjs @@ -387,9 +387,7 @@ class SuggestAutoComplete { /** * Handles the session being reset. */ - resetSession() { - this.#suggestionController?.resetSession(); - } + resetSession() {} #suggestionController; diff --git a/toolkit/components/search/metrics.yaml b/toolkit/components/search/metrics.yaml @@ -702,15 +702,14 @@ browser.searchinit: expires: 151 unit: updates -# New metrics added to this section should also be duplicated to search.suggestions.ohttp. search.suggestions: latency: type: labeled_timing_distribution description: > Records the latencies (ms) of search suggestions fetches per search - engine when not using OHTTP. Keys in this histogram are the search engine - identifier for configuration provided search engines and 'other' for - search engines installed via other methods. + engine. Keys in this histogram are the search engine identifier for + configuration provided search engines and 'other' for search engines + installed via other methods. This metric was generated to correspond to the Legacy Telemetry exponential histogram SEARCH_SUGGESTIONS_LATENCY_MS. @@ -785,79 +784,3 @@ search.suggestions: notification_emails: - fx-search-telemetry@mozilla.com expires: 153 - -# New metrics added to this section should also be duplicated to search.suggestions. -search.suggestions.ohttp: - latency: - type: labeled_timing_distribution - description: > - Records the latencies (ms) of search suggestions fetches per search - engine when using OHTTP. Keys in this histogram are the search engine - identifier for configuration provided search engines and 'other' for - search engines installed via other methods. - time_unit: millisecond - bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1743885 - - https://bugzilla.mozilla.org/show_bug.cgi?id=1987911 - data_reviews: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1743885 - - https://bugzilla.mozilla.org/show_bug.cgi?id=1987911 - data_sensitivity: - - technical - notification_emails: - - fx-search-telemetry@mozilla.com - expires: never - - request_counter: - type: dual_labeled_counter - description: > - Counts the number of search suggestion requests for OHTTP. - dual_labels: - key: - description: > - Only records config engines using their short IDs ('id', not - 'identifier') as labels. Non-configuration engines have the id - recorded as "other". - category: - description: > - The result of the request. - - Failed requests are counted according to the number of failures. The first - failure will be added to the `failed1` bucket, the second to `failed2` and - so on. - - After the 5th failure, the client will fallback to direct HTTP, which is - reported separately. - labels: - - success - - aborted - - failed1 - - failed2 - - failed3 - - failed4 - - failed5 - bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1986019 - data_reviews: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1986019 - data_sensitivity: - - interaction - notification_emails: - - fx-search-telemetry@mozilla.com - expires: 153 - - enabled: - type: boolean - description: | - Reflects the value of the user's browser.search.suggest.ohttp.enabled - preference. This does not reflect the value of the featureGate preference. - bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1991473 - data_reviews: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1991473 - data_sensitivity: - - interaction - notification_emails: - - fx-search-telemetry@mozilla.com - expires: never - lifetime: application diff --git a/toolkit/components/search/tests/xpcshell/test_searchSuggest_ohttp.js b/toolkit/components/search/tests/xpcshell/test_searchSuggest_ohttp.js @@ -1,488 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -/** - * Testing search suggestions from SearchSuggestionController.sys.mjs. - */ - -"use strict"; - -const { SearchSuggestionController } = ChromeUtils.importESModule( - "moz-src:///toolkit/components/search/SearchSuggestionController.sys.mjs" -); -const { ObliviousHTTP } = ChromeUtils.importESModule( - "resource://gre/modules/ObliviousHTTP.sys.mjs" -); - -const ENGINE_ID = "suggestions-engine-test"; - -let server = useHttpServer(); -server.registerContentType("sjs", "sjs"); - -const CONFIG = [ - { - identifier: ENGINE_ID, - base: { - name: "other", - urls: { - suggestions: { - base: `${gHttpURL}/sjs/searchSuggestions.sjs`, - params: [ - { - name: "parameter", - value: "14235", - }, - ], - searchTermParamName: "q", - }, - }, - }, - }, -]; - -let configEngine; - -add_setup(async function () { - Services.fog.initializeFOG(); - Services.prefs.setBoolPref("browser.search.suggest.enabled", true); - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpConfigURL", - "https://example.com/config" - ); - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpRelayURL", - "https://example.com/relay" - ); - Services.prefs.setBoolPref("browser.search.suggest.ohttp.featureGate", true); - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", true); - - SearchTestUtils.setRemoteSettingsConfig(CONFIG); - await Services.search.init(); - - configEngine = Services.search.getEngineById(CONFIG[0].identifier); - - SearchSuggestionController.oHTTPEngineId = CONFIG[0].identifier; - - sinon.stub(ObliviousHTTP, "getOHTTPConfig").resolves({}); - sinon.stub(ObliviousHTTP, "ohttpRequest").callsFake(() => {}); -}); - -add_task(async function test_preference_enabled_telemetry() { - // The search service was initialised in add_setup after - // `browser.search.suggest.ohttp.enabled` was set to true, so Glean should - // have recorded the correct value here. - Assert.ok( - Glean.searchSuggestionsOhttp.enabled.testGetValue(), - "Should have recorded the enabled preference on init" - ); - - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", false); - - Assert.ok( - !Glean.searchSuggestionsOhttp.enabled.testGetValue(), - "Should have recorded the enabled preference after toggling it" - ); - - // Reset back to true for the rest of the tests. - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", true); -}); - -add_task(async function simple_remote_results_merino() { - const suggestions = ["Mozilla", "modern", "mom"]; - - ObliviousHTTP.ohttpRequest.callsFake(() => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [ - { - title: "", - url: "https://merino.services.mozilla.com", - provider: "google_suggest", - is_sponsored: false, - score: 1, - custom_details: { - google_suggest: { - suggestions: ["mo", suggestions], - }, - }, - }, - ], - }), - ok: true, - }; - }); - - let expectedParams = { - q: "mo", - providers: "google_suggest", - google_suggest_params: new URLSearchParams([ - ["parameter", 14235], - ["q", "mo"], - ]), - }; - - // Now do the actual request. - let controller = new SearchSuggestionController(); - let result = await controller.fetch({ - searchString: "mo", - inPrivateBrowsing: false, - engine: Services.search.defaultEngine, - }); - Assert.equal(result.term, "mo", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.deepEqual( - result.remote.map(r => r.value), - suggestions, - "Should have the expected remote suggestions" - ); - - assertLatencyCollection(configEngine, true); - - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should have requested via OHTTP once" - ); - let args = ObliviousHTTP.ohttpRequest.firstCall.args; - Assert.deepEqual( - args[0], - "https://example.com/relay", - "Should have called the Relay URL" - ); - let url = new URL(args[2]); - Assert.deepEqual( - url.origin + url.pathname, - Services.prefs.getCharPref("browser.urlbar.merino.endpointURL"), - "Should have the correct URL base" - ); - for (let [param, value] of Object.entries(expectedParams)) { - if (URLSearchParams.isInstance(value)) { - Assert.equal( - url.searchParams.get(param), - value.toString(), - `Should have set the correct value for ${param}` - ); - } else { - Assert.equal( - url.searchParams.get(param), - value, - `Should have set the correct value for ${param}` - ); - } - } -}); - -add_task(async function simple_merino_empty_result() { - // Tests the case when Merino returns an empty response, e.g. due to an error - // there may be no suggestions returned. - - ObliviousHTTP.ohttpRequest.resetHistory(); - - consoleAllowList = consoleAllowList.concat([ - "SearchSuggestionController found an unexpected string value", - ]); - - ObliviousHTTP.ohttpRequest.callsFake(() => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [], - }), - ok: true, - }; - }); - - let expectedParams = { - q: "mo", - providers: "google_suggest", - google_suggest_params: new URLSearchParams([ - ["parameter", 14235], - ["q", "mo"], - ]), - }; - - // Now do the actual request. - let controller = new SearchSuggestionController(); - let result = await controller.fetch({ - searchString: "mo", - inPrivateBrowsing: false, - engine: Services.search.defaultEngine, - }); - Assert.equal(result.term, "mo", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.deepEqual( - result.remote.map(r => r.value), - [], - "Should have no remote suggestions" - ); - - assertLatencyCollection(configEngine, true); - - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should have requested via OHTTP once" - ); - let args = ObliviousHTTP.ohttpRequest.firstCall.args; - Assert.deepEqual( - args[0], - "https://example.com/relay", - "Should have called the Relay URL" - ); - let url = new URL(args[2]); - Assert.deepEqual( - url.origin + url.pathname, - Services.prefs.getCharPref("browser.urlbar.merino.endpointURL"), - "Should have the correct URL base" - ); - for (let [param, value] of Object.entries(expectedParams)) { - if (URLSearchParams.isInstance(value)) { - Assert.equal( - url.searchParams.get(param), - value.toString(), - `Should have set the correct value for ${param}` - ); - } else { - Assert.equal( - url.searchParams.get(param), - value, - `Should have set the correct value for ${param}` - ); - } - } -}); - -add_task(async function simple_remote_results_merino_third_party() { - let thirdPartyData = { - baseURL: `${gHttpURL}/sjs/`, - name: "Third Party", - method: "GET", - }; - let thirdPartyEngine = await SearchTestUtils.installOpenSearchEngine({ - url: `${gHttpURL}/sjs/engineMaker.sjs?${JSON.stringify(thirdPartyData)}`, - }); - - SearchSuggestionController.oHTTPEngineId = thirdPartyEngine.id; - - const suggestions = ["Mozilla", "modern", "mom"]; - - ObliviousHTTP.ohttpRequest.resetHistory(); - ObliviousHTTP.ohttpRequest.callsFake(() => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [ - { - title: "", - url: "https://merino.services.mozilla.com", - provider: "google_suggest", - is_sponsored: false, - score: 1, - custom_details: { - google_suggest: { - suggestions: ["mo", suggestions], - }, - }, - }, - ], - }), - ok: true, - }; - }); - - let expectedParams = { - q: "mo", - providers: "google_suggest", - google_suggest_params: new URLSearchParams([["q", "mo"]]), - }; - - // Now do the actual request. - let controller = new SearchSuggestionController(); - let result = await controller.fetch({ - searchString: "mo", - inPrivateBrowsing: false, - engine: thirdPartyEngine, - }); - Assert.equal(result.term, "mo", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.deepEqual( - result.remote.map(r => r.value), - suggestions, - "Should have the expected remote suggestions" - ); - - assertLatencyCollection(thirdPartyEngine, true); - - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should have requested via OHTTP once" - ); - let args = ObliviousHTTP.ohttpRequest.firstCall.args; - Assert.deepEqual( - args[0], - "https://example.com/relay", - "Should have called the Relay URL" - ); - let url = new URL(args[2]); - Assert.deepEqual( - url.origin + url.pathname, - Services.prefs.getCharPref("browser.urlbar.merino.endpointURL"), - "Should have the correct URL base" - ); - for (let [param, value] of Object.entries(expectedParams)) { - if (URLSearchParams.isInstance(value)) { - Assert.equal( - url.searchParams.get(param), - value.toString(), - `Should have set the correct value for ${param}` - ); - } else { - Assert.equal( - url.searchParams.get(param), - value, - `Should have set the correct value for ${param}` - ); - } - } - SearchSuggestionController.oHTTPEngineId = configEngine.id; -}); - -async function testUsesOHttp() { - ObliviousHTTP.ohttpRequest.resetHistory(); - - const suggestions = ["Mozilla", "modern", "mom"]; - - // Now do the actual request. - let controller = new SearchSuggestionController(); - let result = await controller.fetch({ - searchString: "mo", - inPrivateBrowsing: false, - engine: Services.search.defaultEngine, - }); - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should have requested via OHTTP once" - ); - - Assert.equal(result.term, "mo", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.deepEqual( - result.remote.map(r => r.value), - suggestions, - "Should have the expected remote suggestions" - ); -} - -async function testUsesDirectHTTP(message) { - ObliviousHTTP.ohttpRequest.resetHistory(); - - // Now do the actual request - let controller = new SearchSuggestionController(); - let result = await controller.fetch({ - searchString: "mo", - inPrivateBrowsing: false, - engine: Services.search.defaultEngine, - }); - Assert.equal(ObliviousHTTP.ohttpRequest.callCount, 0, message); - - Assert.equal(result.term, "mo", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.deepEqual( - result.remote.map(r => r.value), - ["Mozilla", "modern", "mom"], - "Should have no remote suggestions" - ); -} - -add_task(async function test_merino_not_used_when_ohttp_feature_turned_off() { - // These should already be set, but we'll set them here again for completeness - // and clarity within this sub-test. - Services.prefs.setBoolPref("browser.search.suggest.ohttp.featureGate", true); - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", true); - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpConfigURL", - "https://example.com/config" - ); - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpRelayURL", - "https://example.com/relay" - ); - - // With everything set, we should be using OHTTP. - await testUsesOHttp(); - - // Test turning off the feature gate. - Services.prefs.setBoolPref("browser.search.suggest.ohttp.featureGate", false); - - await testUsesDirectHTTP( - "Should not have requested via OHTTP when featureGate is false" - ); - - // Now the OHTTP preference - Services.prefs.setBoolPref("browser.search.suggest.ohttp.featureGate", true); - - // Test we've re-enabled everything, just in case. - await testUsesOHttp(); - - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", false); - - await testUsesDirectHTTP( - "Should not have requested via OHTTP when enabled is false" - ); - - // Now the relay preferences. - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", true); - - // Test we've re-enabled everything, just in case. - await testUsesOHttp(); - - Services.prefs.clearUserPref("browser.urlbar.merino.ohttpConfigURL"); - - await testUsesDirectHTTP( - "Should not have requested via OHTTP when ohttpConfigURL is not defined" - ); - - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpConfigURL", - "https://example.com/config" - ); - - // Test we've re-enabled everything in-between, just in case. - await testUsesOHttp(); - - Services.prefs.clearUserPref("browser.urlbar.merino.ohttpRelayURL"); - - await testUsesDirectHTTP( - "Should not have requested via OHTTP when ohttpRelayURL is not defined" - ); -}); - -function assertLatencyCollection(engine, shouldRecord) { - let latencyDistribution = - Glean.searchSuggestionsOhttp.latency[ - // Third party engines are always recorded as "other". - engine.isConfigEngine ? engine.id : "other" - ].testGetValue(); - - if (shouldRecord) { - Assert.deepEqual( - latencyDistribution.count, - 1, - "Should have recorded a latency count" - ); - } else { - Assert.deepEqual( - latencyDistribution, - null, - "Should not have recorded a latency count" - ); - } - - Services.fog.testResetFOG(); -} diff --git a/toolkit/components/search/tests/xpcshell/test_searchSuggest_ohttp_fallback.js b/toolkit/components/search/tests/xpcshell/test_searchSuggest_ohttp_fallback.js @@ -1,262 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -/** - * Testing search suggestions from SearchSuggestionController.sys.mjs. - */ - -"use strict"; - -const { SearchSuggestionController } = ChromeUtils.importESModule( - "moz-src:///toolkit/components/search/SearchSuggestionController.sys.mjs" -); -const { ObliviousHTTP } = ChromeUtils.importESModule( - "resource://gre/modules/ObliviousHTTP.sys.mjs" -); - -const ENGINE_ID = "suggestions-engine-test"; - -let server = useHttpServer(); -server.registerContentType("sjs", "sjs"); - -const CONFIG = [ - { - identifier: ENGINE_ID, - base: { - name: "other", - urls: { - suggestions: { - base: `${gHttpURL}/sjs/searchSuggestions.sjs`, - params: [ - { - name: "parameter", - value: "14235", - }, - ], - searchTermParamName: "q", - }, - }, - }, - }, -]; - -add_setup(async function () { - consoleAllowList = consoleAllowList.concat([ - "SearchSuggestionController found an unexpected string value", - ]); - - Services.fog.initializeFOG(); - Services.prefs.setBoolPref("browser.search.suggest.enabled", true); - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpConfigURL", - "https://example.com/config" - ); - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpRelayURL", - "https://example.com/relay" - ); - Services.prefs.setBoolPref("browser.search.suggest.ohttp.featureGate", true); - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", true); - - SearchTestUtils.setRemoteSettingsConfig(CONFIG); - await Services.search.init(); - - SearchSuggestionController.oHTTPEngineId = CONFIG[0].identifier; - - sinon.stub(ObliviousHTTP, "getOHTTPConfig").resolves({}); - sinon.stub(ObliviousHTTP, "ohttpRequest").callsFake(() => {}); -}); - -async function do_successful_request(controller) { - ObliviousHTTP.ohttpRequest.callsFake(() => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [ - { - title: "", - url: "https://merino.services.mozilla.com", - provider: "google_suggest", - is_sponsored: false, - score: 1, - custom_details: { - google_suggest: { - suggestions: ["oh", ["ohttp"]], - }, - }, - }, - ], - }), - ok: true, - }; - }); - - let result = await controller.fetch({ - searchString: "oh", - inPrivateBrowsing: false, - engine: Services.search.defaultEngine, - }); - - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should have requested via OHTTP once" - ); - - Assert.equal(result.term, "oh", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.deepEqual( - result.remote.map(r => r.value), - ["ohttp"], - "Should have the expected remote suggestions" - ); - - ObliviousHTTP.ohttpRequest.resetHistory(); -} - -async function do_failed_request(controller) { - ObliviousHTTP.ohttpRequest.callsFake(() => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [], - }), - ok: true, - }; - }); - - let result = await controller.fetch({ - searchString: "oh", - inPrivateBrowsing: false, - engine: Services.search.defaultEngine, - }); - - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 1, - "Should have requested via OHTTP once" - ); - - Assert.equal(result.term, "oh", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.equal(result.remote.length, 0, "Should have no remote suggestions"); - - ObliviousHTTP.ohttpRequest.resetHistory(); -} - -async function do_request_expect_fallback_direct(controller) { - ObliviousHTTP.ohttpRequest.callsFake(() => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [], - }), - ok: true, - }; - }); - - let result = await controller.fetch({ - searchString: "mo", - inPrivateBrowsing: false, - engine: Services.search.defaultEngine, - }); - - Assert.equal( - ObliviousHTTP.ohttpRequest.callCount, - 0, - "Should not have requested via OHTTP" - ); - - Assert.equal(result.term, "mo", "Should have the term matching the query"); - Assert.equal(result.local.length, 0, "Should have no local suggestions"); - Assert.deepEqual( - result.remote.map(r => r.value), - ["Mozilla", "modern", "mom"], - "Should have remote suggestions from searchSuggestions.sjs" - ); -} - -add_task(async function search_suggestions_fallsback_to_direct_http() { - let controller = new SearchSuggestionController(); - - info("Initial request via OHTTP should be successful"); - await do_successful_request(controller); - await assertTelemetry({ success: 1, failed: 0 }); - - info("First failed request"); - await do_failed_request(controller); - await assertTelemetry({ success: 1, failed: 1 }); - - info("Second failed request"); - await do_failed_request(controller); - await assertTelemetry({ success: 1, failed: 2 }); - - // Reset fog for easier counting. - Services.fog.testResetFOG(); - - info("Successful Request should reset the counter"); - await do_successful_request(controller); - await assertTelemetry({ success: 1, failed: 0 }); - - info("Start 5 failed requests"); - for ( - let i = 0; - i < SearchSuggestionController.MAX_OHTTP_FAILURES_BEFORE_FALLBACK; - i++ - ) { - info(`Failed request ${i + 1}`); - await do_failed_request(controller); - } - - await assertTelemetry({ success: 1, failed: 5 }); - - // Reset fog for easier counting. - Services.fog.testResetFOG(); - - info("Request should fallback to direct HTTP"); - await do_request_expect_fallback_direct(controller); - await assertTelemetry({ success: 0, failed: 0 }); - - info("Request should fallback to direct HTTP with longer time in past"); - // Subtract an hour. - controller._ohttpLastFailureTimeMs -= 1 * 60 * 60 * 1000; - await do_request_expect_fallback_direct(controller); - await assertTelemetry({ success: 0, failed: 0 }); - - info("Requests should resume OHTTP after time has expired, but an extra"); - info("failed request should not cause fallback straight away."); - controller._ohttpLastFailureTimeMs -= (1 * 60 + 1) * 60 * 1000; - await do_failed_request(controller); - await assertTelemetry({ success: 0, failed: 1 }); - - // Subtract a second hour and a little bit. - await do_successful_request(controller); - await assertTelemetry({ success: 1, failed: 1 }); -}); - -async function assertTelemetry({ success, failed }) { - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(ENGINE_ID, "success") - .testGetValue(), - success ? success : null, - `Should ${success ? "" : "not "}have incremented the successes` - ); - - for ( - let i = 1; - i <= SearchSuggestionController.MAX_OHTTP_FAILURES_BEFORE_FALLBACK; - i++ - ) { - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(ENGINE_ID, "failed" + i) - .testGetValue(), - failed >= i ? 1 : null, - `Should ${success ? "" : "not "}have incremented failed${i}` - ); - } -} diff --git a/toolkit/components/search/tests/xpcshell/test_searchSuggestionCountTelemetry.js b/toolkit/components/search/tests/xpcshell/test_searchSuggestionCountTelemetry.js @@ -12,9 +12,6 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ const { SearchSuggestionController } = ChromeUtils.importESModule( "moz-src:///toolkit/components/search/SearchSuggestionController.sys.mjs" ); -const { ObliviousHTTP } = ChromeUtils.importESModule( - "resource://gre/modules/ObliviousHTTP.sys.mjs" -); let openSearchEngine, workingAppEngine, failingAppEngine; @@ -67,47 +64,9 @@ add_setup(async function () { }); workingAppEngine = Services.search.getEngineById("workingAppEngine"); failingAppEngine = Services.search.getEngineById("failingAppEngine"); - - // Set up OHTTP as well, this currently won't be used unless the - // `SearchSuggestionController.oHTTPEngineId` matches the active engine. - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpConfigURL", - "https://example.com/config" - ); - Services.prefs.setCharPref( - "browser.urlbar.merino.ohttpRelayURL", - "https://example.com/relay" - ); - Services.prefs.setBoolPref("browser.search.suggest.ohttp.featureGate", true); - Services.prefs.setBoolPref("browser.search.suggest.ohttp.enabled", true); - - sinon.stub(ObliviousHTTP, "getOHTTPConfig").resolves({}); - sinon.stub(ObliviousHTTP, "ohttpRequest").callsFake(() => { - return { - status: 200, - json: async () => - Promise.resolve({ - suggestions: [ - { - title: "", - url: "https://merino.services.mozilla.com", - provider: "google_suggest", - is_sponsored: false, - score: 1, - custom_details: { - google_suggest: { - suggestions: ["mo", ["Mozilla", "modern", "mom"]], - }, - }, - }, - ], - }), - ok: true, - }; - }); }); -async function success_test(testOhttp) { +add_task(async function test_success() { for (let i = 0; i < 5; i++) { let controller = new SearchSuggestionController(); let result = await controller.fetch({ @@ -118,33 +77,14 @@ async function success_test(testOhttp) { Assert.equal(result.remote.length, 3); } - if (testOhttp) { - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(workingAppEngine.id, "success") - .testGetValue(), - 5, - "Successful OHTTP request counter is correctly updated" - ); - } else { - Assert.equal( - Glean.searchSuggestions.successfulRequests.workingAppEngine.testGetValue(), - 5, - "Successful HTTP request counter is correctly updated" - ); - } -} -add_task(async function test_success() { - await success_test(false); - - SearchSuggestionController.oHTTPEngineId = workingAppEngine.id; - - await success_test(true); - - SearchSuggestionController.oHTTPEngineId = "not-matching"; + Assert.equal( + Glean.searchSuggestions.successfulRequests.workingAppEngine.testGetValue(), + 5, + "Successful HTTP request counter is correctly updated" + ); }); -async function abort_test(testOhttp) { +add_task(async function test_abort() { let controller = new SearchSuggestionController(); // Don't await the result to trigger the abort handler. @@ -174,46 +114,19 @@ async function abort_test(testOhttp) { engine: workingAppEngine, }); - if (testOhttp) { - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(workingAppEngine.id, "success") - .testGetValue(), - 6, // 1 new + 5 from the previous test. - "Successful OHTTP request counter is correctly updated" - ); - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(workingAppEngine.id, "aborted") - .testGetValue(), - 4, - "Aborted OHTTP request counter is correctly updated" - ); - } else { - Assert.equal( - Glean.searchSuggestions.successfulRequests.workingAppEngine.testGetValue(), - 6, // 1 new + 5 from the previous test. - "Successful HTTP request counter is correctly updated" - ); - Assert.equal( - Glean.searchSuggestions.abortedRequests.workingAppEngine.testGetValue(), - 4, - "Aborted HTTP request counter is correctly updated" - ); - } -} - -add_task(async function test_abort() { - await abort_test(false); - - SearchSuggestionController.oHTTPEngineId = workingAppEngine.id; - - await abort_test(true); - - SearchSuggestionController.oHTTPEngineId = "not-matching"; + Assert.equal( + Glean.searchSuggestions.successfulRequests.workingAppEngine.testGetValue(), + 6, // 1 new + 5 from the previous test. + "Successful HTTP request counter is correctly updated" + ); + Assert.equal( + Glean.searchSuggestions.abortedRequests.workingAppEngine.testGetValue(), + 4, + "Aborted HTTP request counter is correctly updated" + ); }); -async function nonConfig_test(testOhttp) { +add_task(async function test_nonConfig() { let controller = new SearchSuggestionController(); let result = await controller.fetch({ searchString: "mo", @@ -222,34 +135,14 @@ async function nonConfig_test(testOhttp) { }); Assert.equal(result.remote.length, 3); - if (testOhttp) { - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get("other", "success") - .testGetValue(), - 1, - "Telemetry is recorded for non-config-engines for oHTTP" - ); - } else { - Assert.equal( - Glean.searchSuggestions.successfulRequests.openSearchEngine.testGetValue(), - null, - "No telemetry is recorded for non-config-engine" - ); - } -} - -add_task(async function test_nonConfig() { - await nonConfig_test(false); - - SearchSuggestionController.oHTTPEngineId = openSearchEngine.id; - - await nonConfig_test(true); - - SearchSuggestionController.oHTTPEngineId = "not-matching"; + Assert.equal( + Glean.searchSuggestions.successfulRequests.openSearchEngine.testGetValue(), + null, + "No telemetry is recorded for non-config-engine" + ); }); -async function error_test(testOhttp) { +add_task(async function test_error() { let controller = new SearchSuggestionController(); await controller.fetch({ searchString: "mo", @@ -267,55 +160,9 @@ async function error_test(testOhttp) { engine: failingAppEngine, }); - if (testOhttp) { - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(failingAppEngine.id, "failed1") - .testGetValue(), - 1, - "Failed1 has been updated for OHTTP request counter" - ); - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(failingAppEngine.id, "failed2") - .testGetValue(), - 1, - "Failed2 has been updated for OHTTP request counter" - ); - Assert.equal( - Glean.searchSuggestionsOhttp.requestCounter - .get(failingAppEngine.id, "failed3") - .testGetValue(), - 1, - "Failed3 has been updated for OHTTP request counter" - ); - } else { - Assert.equal( - Glean.searchSuggestions.failedRequests.failingAppEngine.testGetValue(), - 3, - "Failed HTTP request counter is correctly updated" - ); - } -} - -// Run this test last, as it changes the ObliviousHTTP.ohttpRequest stub. -add_task(async function test_error() { - await error_test(false); - - SearchSuggestionController.oHTTPEngineId = failingAppEngine.id; - - ObliviousHTTP.ohttpRequest.callsFake(() => { - return { - status: 500, - json: async () => - Promise.resolve({ - suggestions: [], - }), - ok: true, - }; - }); - - await error_test(true); - - SearchSuggestionController.oHTTPEngineId = "not-matching"; + Assert.equal( + Glean.searchSuggestions.failedRequests.failingAppEngine.testGetValue(), + 3, + "Failed HTTP request counter is correctly updated" + ); }); diff --git a/toolkit/components/search/tests/xpcshell/xpcshell.toml b/toolkit/components/search/tests/xpcshell/xpcshell.toml @@ -190,20 +190,6 @@ support-files = [ ["test_searchSuggest_extraParams.js"] -["test_searchSuggest_ohttp.js"] -# OHTTP experiment uses MerinoClient, and therefore not available elsewhere. -skip-if = [ - "os == 'android'", - "appname == 'thunderbird'", -] - -["test_searchSuggest_ohttp_fallback.js"] -# OHTTP experiment uses MerinoClient, and therefore not available elsewhere. -skip-if = [ - "os == 'android'", - "appname == 'thunderbird'", -] - ["test_searchSuggest_private.js"] ["test_searchSuggestionCountTelemetry.js"] diff --git a/tools/@types/generated/lib.gecko.glean.d.ts b/tools/@types/generated/lib.gecko.glean.d.ts @@ -6713,12 +6713,6 @@ interface GleanImpl { successfulRequests: Record<string, GleanCounter>; } - searchSuggestionsOhttp: { - enabled: GleanBoolean; - latency: Record<string, GleanTimingDistribution>; - requestCounter: GleanDualLabeledCounter; - } - legacyTelemetry: { clientId: GleanUuid; profileGroupId: GleanUuid;