From 08df665966abdf237c6f3428015b66cb3f905a95 Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Fri, 24 Jul 2020 15:24:41 +0000 Subject: [PATCH] Bug 1644743 - Consolidate Targeting evaluation across Normandy, Experiment Manager and ASRouter r=k88hudson,mythmon,janerik Differential Revision: https://phabricator.services.mozilla.com/D79967 --- .../docs/v2-system-addon/data_events.md | 20 ++ browser/components/newtab/lib/ASRouter.jsm | 10 +- .../newtab/lib/ASRouterTargeting.jsm | 80 +++---- .../browser/browser_asrouter_targeting.js | 74 +------ .../test/unit/asrouter/ASRouter.test.js | 22 +- .../unit/asrouter/ASRouterTargeting.test.js | 78 ++++--- toolkit/components/messaging-system/jar.mn | 1 + .../lib/RemoteSettingsExperimentLoader.jsm | 21 +- toolkit/components/messaging-system/moz.build | 1 + .../messaging-system/targeting/Targeting.jsm | 203 +++++++++++++++++ .../targeting/test/unit/head.js | 5 + .../targeting/test/unit/test_targeting.js | 206 ++++++++++++++++++ .../targeting/test/unit/xpcshell.ini | 6 + .../components/normandy/lib/RecipeRunner.jsm | 4 +- toolkit/components/telemetry/Events.yaml | 20 ++ 15 files changed, 582 insertions(+), 169 deletions(-) create mode 100644 toolkit/components/messaging-system/targeting/Targeting.jsm create mode 100644 toolkit/components/messaging-system/targeting/test/unit/head.js create mode 100644 toolkit/components/messaging-system/targeting/test/unit/test_targeting.js create mode 100644 toolkit/components/messaging-system/targeting/test/unit/xpcshell.ini diff --git a/browser/components/newtab/docs/v2-system-addon/data_events.md b/browser/components/newtab/docs/v2-system-addon/data_events.md index 5ad04889db6a..100b07d85084 100644 --- a/browser/components/newtab/docs/v2-system-addon/data_events.md +++ b/browser/components/newtab/docs/v2-system-addon/data_events.md @@ -1482,3 +1482,23 @@ Unlike other Activity Stream pings, this is a Firefox Events telemetry event, an } } ``` + +### Experiment attribute errors + +This records whether issues were encountered with any of the targeting attributes used in the experiment enrollment or message targeting. +Two different types of events are sent: `attribute_error` and `attribute_timeout` along with the attribute that caused it. + +```js +[ + "messaging_experiments", + "targeting", + "attribute_error", // event + "foo" // attribute +], +[ + "messaging_experiments", + "targeting", + "attribute_timeout", // event + "bar" // attribute +] +``` diff --git a/browser/components/newtab/lib/ASRouter.jsm b/browser/components/newtab/lib/ASRouter.jsm index c19cfd0f189f..aa4b79baa1b2 100644 --- a/browser/components/newtab/lib/ASRouter.jsm +++ b/browser/components/newtab/lib/ASRouter.jsm @@ -34,6 +34,7 @@ XPCOMUtils.defineLazyModuleGetters(this, { ExperimentAPI: "resource://messaging-system/experiments/ExperimentAPI.jsm", SpecialMessageActions: "resource://messaging-system/lib/SpecialMessageActions.jsm", + TargetingContext: "resource://messaging-system/targeting/Targeting.jsm", }); XPCOMUtils.defineLazyServiceGetters(this, { BrowserHandler: ["@mozilla.org/browser/clh;1", "nsIBrowserHandler"], @@ -581,12 +582,13 @@ class _ASRouter { // Notify all tabs of messages that have become invalid after pref change const invalidMessages = []; const context = this._getMessagesContext(); + const targetingContext = new TargetingContext(context); for (const msg of this.state.messages.filter(this.isUnblockedMessage)) { if (!msg.targeting) { continue; } - const isMatch = await ASRouterTargeting.isMatch(msg.targeting, context); + const isMatch = await targetingContext.evalWithDefault(msg.targeting); if (!isMatch) { invalidMessages.push(msg.id); } @@ -1105,7 +1107,7 @@ class _ASRouter { }); } - _handleTargetingError(type, error, message) { + _handleTargetingError(error, message) { Cu.reportError(error); if (this.dispatchToAS) { this.dispatchToAS( @@ -1113,7 +1115,6 @@ class _ASRouter { message_id: message.id, action: "asrouter_undesired_event", event: "TARGETING_EXPRESSION_ERROR", - event_context: type, }) ); } @@ -1147,10 +1148,11 @@ class _ASRouter { async evaluateExpression(target, { expression, context }) { const channel = target || this.messageChannel; + const targetingContext = new TargetingContext(context); let evaluationStatus; try { evaluationStatus = { - result: await ASRouterTargeting.isMatch(expression, context), + result: await targetingContext.evalWithDefault(expression), success: true, }; } catch (e) { diff --git a/browser/components/newtab/lib/ASRouterTargeting.jsm b/browser/components/newtab/lib/ASRouterTargeting.jsm index 5e07883f8224..e1c20c819ba7 100644 --- a/browser/components/newtab/lib/ASRouterTargeting.jsm +++ b/browser/components/newtab/lib/ASRouterTargeting.jsm @@ -21,8 +21,7 @@ XPCOMUtils.defineLazyModuleGetters(this, { TelemetryEnvironment: "resource://gre/modules/TelemetryEnvironment.jsm", AppConstants: "resource://gre/modules/AppConstants.jsm", AttributionCode: "resource:///modules/AttributionCode.jsm", - FilterExpressions: - "resource://gre/modules/components-utils/FilterExpressions.jsm", + TargetingContext: "resource://messaging-system/targeting/Targeting.jsm", fxAccounts: "resource://gre/modules/FxAccounts.jsm", Region: "resource://gre/modules/Region.jsm", }); @@ -101,7 +100,6 @@ XPCOMUtils.defineLazyServiceGetter( ); const FXA_USERNAME_PREF = "services.sync.username"; -const MOZ_JEXL_FILEPATH = "mozjexl"; const { activityStreamProvider: asProvider } = NewTabUtils; @@ -566,37 +564,6 @@ const TargetingGetters = { this.ASRouterTargeting = { Environment: TargetingGetters, - ERROR_TYPES: { - MALFORMED_EXPRESSION: "MALFORMED_EXPRESSION", - ATTRIBUTE_ERROR: "JEXL_ATTRIBUTE_GETTER_ERROR", - OTHER_ERROR: "OTHER_ERROR", - }, - - // Combines the getter properties of two objects without evaluating them - combineContexts(contextA = {}, contextB = {}, onError) { - return { - get: (obj, prop) => { - try { - return contextA[prop] || contextB[prop]; - } catch (error) { - onError(this.ERROR_TYPES.ATTRIBUTE_ERROR, error, prop); - } - - return null; - }, - }; - }, - - isMatch(filterExpression, customContext, onError) { - return FilterExpressions.eval( - filterExpression, - new Proxy( - {}, - this.combineContexts(customContext, this.Environment, onError) - ) - ); - }, - isTriggerMatch(trigger = {}, candidateMessageTrigger = {}) { if (trigger.id !== candidateMessageTrigger.id) { return false; @@ -650,12 +617,12 @@ this.ASRouterTargeting = { * checkMessageTargeting - Checks is a message's targeting parameters are satisfied * * @param {*} message An AS router message - * @param {obj} context A FilterExpression context + * @param {obj} targetingContext a TargetingContext instance complete with eval environment * @param {func} onError A function to handle errors (takes two params; error, message) * @param {boolean} shouldCache Should the JEXL evaluations be cached and reused. * @returns */ - async checkMessageTargeting(message, context, onError, shouldCache) { + async checkMessageTargeting(message, targetingContext, onError, shouldCache) { // If no targeting is specified, if (!message.targeting) { return true; @@ -668,7 +635,7 @@ this.ASRouterTargeting = { return result.value; } } - result = await this.isMatch(message.targeting, context, onError); + result = await targetingContext.evalWithDefault(message.targeting); if (shouldCache) { jexlEvaluationCache.set(message.targeting, { timestamp: Date.now(), @@ -676,19 +643,22 @@ this.ASRouterTargeting = { }); } } catch (error) { - Cu.reportError(error); if (onError) { - const type = error.fileName.includes(MOZ_JEXL_FILEPATH) - ? this.ERROR_TYPES.MALFORMED_EXPRESSION - : this.ERROR_TYPES.OTHER_ERROR; - onError(type, error, message); + onError(error, message); } + Cu.reportError(error); result = false; } return result; }, - _isMessageMatch(message, trigger, context, onError, shouldCache = false) { + _isMessageMatch( + message, + trigger, + targetingContext, + onError, + shouldCache = false + ) { return ( message && (trigger @@ -696,7 +666,12 @@ this.ASRouterTargeting = { : !message.trigger) && // If a trigger expression was passed to this function, the message should match it. // Otherwise, we should choose a message with no trigger property (i.e. a message that can show up at any time) - this.checkMessageTargeting(message, context, onError, shouldCache) + this.checkMessageTargeting( + message, + targetingContext, + onError, + shouldCache + ) ); }, @@ -715,25 +690,28 @@ this.ASRouterTargeting = { */ async findMatchingMessage({ messages, - trigger, - context, + trigger = {}, + context = {}, onError, ordered = false, shouldCache = false, returnAll = false, }) { const sortedMessages = getSortedMessages(messages, { ordered }); - const combinedContext = new Proxy( - {}, - this.combineContexts(trigger && trigger.context, context, onError) - ); const matching = returnAll ? [] : null; + const targetingContext = new TargetingContext( + TargetingContext.combineContexts( + context, + this.Environment, + trigger.context || {} + ) + ); const isMatch = candidate => this._isMessageMatch( candidate, trigger, - combinedContext, + targetingContext, onError, shouldCache ); diff --git a/browser/components/newtab/test/browser/browser_asrouter_targeting.js b/browser/components/newtab/test/browser/browser_asrouter_targeting.js index bd3bc0d3723b..c4f837517a53 100644 --- a/browser/components/newtab/test/browser/browser_asrouter_targeting.js +++ b/browser/components/newtab/test/browser/browser_asrouter_targeting.js @@ -48,33 +48,6 @@ ChromeUtils.defineModuleGetter( "resource://gre/modules/Region.jsm" ); -// ASRouterTargeting.isMatch -add_task(async function should_do_correct_targeting() { - is( - await ASRouterTargeting.isMatch("FOO", { FOO: true }), - true, - "should return true for a matching value" - ); - is( - await ASRouterTargeting.isMatch("!FOO", { FOO: true }), - false, - "should return false for a non-matching value" - ); -}); - -add_task(async function should_handle_async_getters() { - const context = { - get FOO() { - return Promise.resolve(true); - }, - }; - is( - await ASRouterTargeting.isMatch("FOO", context), - true, - "should return true for a matching async value" - ); -}); - // ASRouterTargeting.findMatchingMessage add_task(async function find_matching_message() { const messages = [ @@ -107,38 +80,10 @@ add_task(async function return_nothing_for_no_matching_message() { ); }); -add_task(async function check_syntax_error_handling() { - let result; - function onError(...args) { - result = args; - } - - const messages = [{ id: "foo", targeting: "foo === 0" }]; - const match = await ASRouterTargeting.findMatchingMessage({ - messages, - onError, - }); - - is( - match, - undefined, - "should return nothing since no valid matching message exists" - ); - // Note that in order for the following test to pass, we are expecting a particular filepath for mozjexl. - // If the location of this file has changed, the MOZ_JEXL_FILEPATH constant should be updated om ASRouterTargeting.jsm - is( - result[0], - ASRouterTargeting.ERROR_TYPES.MALFORMED_EXPRESSION, - "should recognize the error as coming from mozjexl and call onError with the MALFORMED_EXPRESSION error type" - ); - ok(result[1].message, "should call onError with the error from mozjexl"); - is(result[2], messages[0], "should call onError with the invalid message"); -}); - add_task(async function check_other_error_handling() { - let result; + let called = false; function onError(...args) { - result = args; + called = true; } const messages = [{ id: "foo", targeting: "foo" }]; @@ -158,19 +103,8 @@ add_task(async function check_other_error_handling() { undefined, "should return nothing since no valid matching message exists" ); - // Note that in order for the following test to pass, we are expecting a particular filepath for mozjexl. - // If the location of this file has changed, the MOZ_JEXL_FILEPATH constant should be updated om ASRouterTargeting.jsm - is( - result[0], - ASRouterTargeting.ERROR_TYPES.ATTRIBUTE_ERROR, - "should not recognize the error as being an attribute error." - ); - is( - result[1].message, - "test error", - "should call onError with the error thrown in the context" - ); - is(result[2], "foo", "should call onError with the invalid attribute"); + + Assert.ok(called, "Attribute error caught"); }); // ASRouterTargeting.Environment diff --git a/browser/components/newtab/test/unit/asrouter/ASRouter.test.js b/browser/components/newtab/test/unit/asrouter/ASRouter.test.js index c85e4368946a..10bd92a8a987 100644 --- a/browser/components/newtab/test/unit/asrouter/ASRouter.test.js +++ b/browser/components/newtab/test/unit/asrouter/ASRouter.test.js @@ -71,6 +71,7 @@ describe("ASRouter", () => { let FakeToolbarPanelHub; let FakeMomentsPageHub; let personalizedCfrScores; + let fakeTargetingContext; function createFakeStorage() { const getStub = sandbox.stub(); @@ -168,6 +169,10 @@ describe("ASRouter", () => { uninit: sandbox.stub(), executeAction: sandbox.stub(), }; + fakeTargetingContext = { + combineContexts: sandbox.stub(), + evalWithDefault: sandbox.stub().resolves(), + }; globals.set({ // Testing framework doesn't know how to `defineLazyModuleGetter` so we're // importing these modules into the global scope ourselves. @@ -210,6 +215,15 @@ describe("ASRouter", () => { SpecialMessageActions: { handleAction: sandbox.stub(), }, + TargetingContext: class { + static combineContexts(...args) { + return fakeTargetingContext.combineContexts.apply(sandbox, args); + } + + evalWithDefault(expr) { + return fakeTargetingContext.evalWithDefault(expr); + } + }, }); await createRouterAndInit(); }); @@ -472,7 +486,7 @@ describe("ASRouter", () => { messages: [messageTargeted, messageNotTargeted], providers: [{ id: "snippets" }], }); - sandbox.stub(ASRouterTargeting, "isMatch").resolves(false); + fakeTargetingContext.evalWithDefault.resolves(false); await Router.onPrefChange("services.sync.username"); @@ -552,7 +566,7 @@ describe("ASRouter", () => { beforeEach(async () => { stub = sandbox.stub(); stub.resolves("foo"); - sandbox.stub(ASRouterTargeting, "isMatch").callsFake(stub); + fakeTargetingContext.evalWithDefault.callsFake(stub); }); afterEach(() => { sandbox.restore(); @@ -3488,9 +3502,7 @@ describe("ASRouter", () => { sandbox.stub(Router, "loadMessagesFromAllProviders"); }); it("should dispatch an event when a targeting expression throws an error", async () => { - sandbox - .stub(global.FilterExpressions, "eval") - .returns(Promise.reject(new Error("fake error"))); + fakeTargetingContext.evalWithDefault.rejects("unit test error"); await Router.setState({ messages: [ { diff --git a/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js b/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js index 095f7d036cf4..72964693c4c0 100644 --- a/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js +++ b/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js @@ -17,6 +17,7 @@ describe("#CachedTargetingGetter", () => { let clock; let frecentStub; let topsitesCache; + let globals; beforeEach(() => { sandbox = sinon.createSandbox(); clock = sinon.useFakeTimers(); @@ -26,11 +27,25 @@ describe("#CachedTargetingGetter", () => { ); sandbox.stub(global.Cu, "reportError"); topsitesCache = new CachedTargetingGetter("getTopFrecentSites"); + globals = new GlobalOverrider(); + globals.set( + "TargetingContext", + class { + static combineContexts(...args) { + return sinon.stub(); + } + + evalWithDefault(expr) { + return sinon.stub(); + } + } + ); }); afterEach(() => { sandbox.restore(); clock.restore(); + globals.restore(); }); it("should only make a request every 6 hours", async () => { @@ -188,26 +203,6 @@ describe("#CachedTargetingGetter", () => { assert.equal(arg_m3.id, m2.id); }); }); - describe("combineContexts", () => { - it("should combine the properties of the two objects", () => { - const joined = ASRouterTargeting.combineContexts( - { - get foo() { - return "foo"; - }, - }, - { - get bar() { - return "bar"; - }, - } - ); - const proxy = new Proxy({}, joined); - - assert.equal(proxy.foo, "foo"); - assert.equal(proxy.bar, "bar"); - }); - }); }); describe("#CacheListAttachedOAuthClients", () => { const twoHours = 2 * 60 * 60 * 1000; @@ -262,34 +257,55 @@ describe("ASRouterTargeting", () => { let evalStub; let sandbox; let clock; + let globals; + let fakeTargetingContext; beforeEach(() => { sandbox = sinon.createSandbox(); - evalStub = sandbox.stub(global.FilterExpressions, "eval"); sandbox.replace(ASRouterTargeting, "Environment", {}); clock = sinon.useFakeTimers(); + fakeTargetingContext = { + combineContexts: sandbox.stub(), + evalWithDefault: sandbox.stub().resolves(), + }; + globals = new GlobalOverrider(); + globals.set( + "TargetingContext", + class { + static combineContexts(...args) { + return fakeTargetingContext.combineContexts.apply(sandbox, args); + } + + evalWithDefault(expr) { + return fakeTargetingContext.evalWithDefault(expr); + } + } + ); + evalStub = fakeTargetingContext.evalWithDefault; }); afterEach(() => { clock.restore(); sandbox.restore(); + globals.restore(); }); it("should cache evaluation result", async () => { evalStub.resolves(true); + let targetingContext = new global.TargetingContext(); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl1" }, - {}, + targetingContext, sandbox.stub(), true ); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl2" }, - {}, + targetingContext, sandbox.stub(), true ); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl1" }, - {}, + targetingContext, sandbox.stub(), true ); @@ -298,22 +314,23 @@ describe("ASRouterTargeting", () => { }); it("should not cache evaluation result", async () => { evalStub.resolves(true); + let targetingContext = new global.TargetingContext(); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl" }, - {}, + targetingContext, sandbox.stub(), false ); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl" }, - {}, + targetingContext, sandbox.stub(), false ); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl" }, - {}, + targetingContext, sandbox.stub(), false ); @@ -322,23 +339,24 @@ describe("ASRouterTargeting", () => { }); it("should expire cache entries", async () => { evalStub.resolves(true); + let targetingContext = new global.TargetingContext(); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl" }, - {}, + targetingContext, sandbox.stub(), true ); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl" }, - {}, + targetingContext, sandbox.stub(), true ); clock.tick(5 * 60 * 1000 + 1); await ASRouterTargeting.checkMessageTargeting( { targeting: "jexl" }, - {}, + targetingContext, sandbox.stub(), true ); diff --git a/toolkit/components/messaging-system/jar.mn b/toolkit/components/messaging-system/jar.mn index a1bb60ab486b..3ed3c08d19d3 100644 --- a/toolkit/components/messaging-system/jar.mn +++ b/toolkit/components/messaging-system/jar.mn @@ -6,3 +6,4 @@ toolkit.jar: % resource messaging-system %res/messaging-system/ res/messaging-system/experiments/ (./experiments/*) res/messaging-system/lib/ (./lib/*) + res/messaging-system/targeting/Targeting.jsm (./targeting/Targeting.jsm) diff --git a/toolkit/components/messaging-system/lib/RemoteSettingsExperimentLoader.jsm b/toolkit/components/messaging-system/lib/RemoteSettingsExperimentLoader.jsm index 5693ce0ef00b..324c04449625 100644 --- a/toolkit/components/messaging-system/lib/RemoteSettingsExperimentLoader.jsm +++ b/toolkit/components/messaging-system/lib/RemoteSettingsExperimentLoader.jsm @@ -20,6 +20,7 @@ const { XPCOMUtils } = ChromeUtils.import( XPCOMUtils.defineLazyModuleGetters(this, { ASRouterTargeting: "resource://activity-stream/lib/ASRouterTargeting.jsm", + TargetingContext: "resource://messaging-system/targeting/Targeting.jsm", ExperimentManager: "resource://messaging-system/experiments/ExperimentManager.jsm", RemoteSettings: "resource://services-settings/remote-settings.js", @@ -106,20 +107,24 @@ class _RemoteSettingsExperimentLoader { * @returns {Promise} Should we process the recipe? */ async checkTargeting(recipe, customContext = {}) { + const context = TargetingContext.combineContexts( + customContext, + ASRouterTargeting.Environment + ); const { targeting } = recipe; if (!targeting) { log.debug("No targeting for recipe, so it matches automatically"); return true; } log.debug("Testing targeting expression:", targeting); - const result = await ASRouterTargeting.isMatch( - targeting, - customContext, - err => { - log.debug("Targeting failed because of an error"); - Cu.reportError(err); - } - ); + const targetingContext = new TargetingContext(context); + let result = false; + try { + result = await targetingContext.evalWithDefault(targeting); + } catch (e) { + log.debug("Targeting failed because of an error"); + Cu.reportError(e); + } return Boolean(result); } diff --git a/toolkit/components/messaging-system/moz.build b/toolkit/components/messaging-system/moz.build index 3d13cc3aba77..8b01da688f6d 100644 --- a/toolkit/components/messaging-system/moz.build +++ b/toolkit/components/messaging-system/moz.build @@ -15,6 +15,7 @@ BROWSER_CHROME_MANIFESTS += [ SPHINX_TREES['docs'] = 'schemas' XPCSHELL_TESTS_MANIFESTS += ['test/unit/xpcshell.ini'] +XPCSHELL_TESTS_MANIFESTS += ['targeting/test/unit/xpcshell.ini'] TESTING_JS_MODULES += [ 'schemas/SpecialMessageActionSchemas/SpecialMessageActionSchemas.json', diff --git a/toolkit/components/messaging-system/targeting/Targeting.jsm b/toolkit/components/messaging-system/targeting/Targeting.jsm new file mode 100644 index 000000000000..cdee1a33b17c --- /dev/null +++ b/toolkit/components/messaging-system/targeting/Targeting.jsm @@ -0,0 +1,203 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +"use strict"; + +const { XPCOMUtils } = ChromeUtils.import( + "resource://gre/modules/XPCOMUtils.jsm" +); +XPCOMUtils.defineLazyModuleGetters(this, { + Services: "resource://gre/modules/Services.jsm", + clearTimeout: "resource://gre/modules/Timer.jsm", + setTimeout: "resource://gre/modules/Timer.jsm", + ASRouterTargeting: "resource://activity-stream/lib/ASRouterTargeting.jsm", + FilterExpressions: + "resource://gre/modules/components-utils/FilterExpressions.jsm", + ClientEnvironment: "resource://normandy/lib/ClientEnvironment.jsm", +}); + +var EXPORTED_SYMBOLS = ["TargetingContext"]; + +const TARGETING_EVENT_CATEGORY = "messaging_experiments"; +const TARGETING_EVENT_METHOD = "targeting"; +const DEFAULT_TIMEOUT = 3000; +const ERROR_TYPES = { + ATTRIBUTE_ERROR: "attribute_error", + TIMEOUT: "attribute_timeout", +}; + +const TargetingEnvironment = { + get locale() { + return ASRouterTargeting.Environment.locale; + }, + + get localeLanguageCode() { + return ASRouterTargeting.Environment.localeLanguageCode; + }, + + get region() { + return ASRouterTargeting.Environment.region; + }, + + get userId() { + return ClientEnvironment.userId; + }, +}; + +class TargetingContext { + constructor(customContext) { + if (customContext) { + this.ctx = new Proxy(customContext, { + get: (customCtx, prop) => { + if (prop in TargetingEnvironment) { + return TargetingEnvironment[prop]; + } + return customCtx[prop]; + }, + }); + } else { + this.ctx = TargetingEnvironment; + } + + // Enable event recording + Services.telemetry.setEventRecordingEnabled(TARGETING_EVENT_CATEGORY, true); + } + + _sendUndesiredEvent(eventData) { + Services.telemetry.recordEvent( + TARGETING_EVENT_CATEGORY, + TARGETING_EVENT_METHOD, + eventData.event, + eventData.value + ); + } + + /** + * Wrap each property of context[key] with a Proxy that captures errors and + * timeouts + * + * @param {Object. | TargetingGetters} context + * @param {string} key Namespace value found in `context` param + * @returns {TargetingGetters} Wrapped context where getter report errors and timeouts + */ + createContextWithTimeout(context, key = null) { + const timeoutDuration = key ? context[key].timeout : context.timeout; + const logUndesiredEvent = (event, key, prop) => { + const value = key ? `${key}.${prop}` : prop; + this._sendUndesiredEvent({ event, value }); + Cu.reportError(`${event}: ${value}`); + }; + + return new Proxy(context, { + get(target, prop) { + // eslint-disable-next-line no-async-promise-executor + return new Promise(async (resolve, reject) => { + // Create timeout cb to record attribute resolution taking too long. + let timeout = setTimeout(() => { + logUndesiredEvent(ERROR_TYPES.TIMEOUT, key, prop); + reject( + new Error( + `${prop} targeting getter timed out after ${timeoutDuration || + DEFAULT_TIMEOUT}ms` + ) + ); + }, timeoutDuration || DEFAULT_TIMEOUT); + + try { + resolve(await (key ? target[key][prop] : target[prop])); + } catch (error) { + logUndesiredEvent(ERROR_TYPES.ATTRIBUTE_ERROR, key, prop); + reject(error); + Cu.reportError(error); + } finally { + clearTimeout(timeout); + } + }); + }, + }); + } + + /** + * Merge all evaluation contexts and wrap the getters with timeouts + * + * @param {Object.[]} contexts + * @returns {Object.} Object that follows the pattern of `namespace: getters` + */ + mergeEvaluationContexts(contexts) { + let context = {}; + for (let c of contexts) { + for (let envNamespace of Object.keys(c)) { + // Take the provided context apart, replace it with a proxy + context[envNamespace] = this.createContextWithTimeout(c, envNamespace); + } + } + + return context; + } + + /** + * Merge multiple TargetingGetters objects without accidentally evaluating + * + * @param {TargetingGetters[]} ...contexts + * @returns {Proxy} + */ + static combineContexts(...contexts) { + return new Proxy( + {}, + { + get(target, prop) { + for (let context of contexts) { + if (prop in context) { + return context[prop]; + } + } + + return null; + }, + } + ); + } + + /** + * Evaluate JEXL expressions with default `TargetingEnvironment` and custom + * provided targeting contexts + * + * @example + * eval( + * "ctx.locale == 'en-US' && customCtx.foo == 42", + * { customCtx: { foo: 42 } } + * ); // true + * + * @param {string} expression JEXL expression + * @param {Object.[]} ...contexts Additional custom context + * objects where the keys act as namespaces for the different getters + * + * @returns {promise} Evaluation result + */ + eval(expression, ...contexts) { + return FilterExpressions.eval( + expression, + this.mergeEvaluationContexts([{ ctx: this.ctx }, ...contexts]) + ); + } + + /** + * Evaluate JEXL expressions with default provided targeting context + * + * @example + * new TargetingContext({ bar: 42 }); + * evalWithDefault( + * "bar == 42", + * ); // true + * + * @param {string} expression JEXL expression + * @returns {promise} Evaluation result + */ + evalWithDefault(expression) { + return FilterExpressions.eval( + expression, + this.createContextWithTimeout(this.ctx) + ); + } +} diff --git a/toolkit/components/messaging-system/targeting/test/unit/head.js b/toolkit/components/messaging-system/targeting/test/unit/head.js new file mode 100644 index 000000000000..085386dc49e1 --- /dev/null +++ b/toolkit/components/messaging-system/targeting/test/unit/head.js @@ -0,0 +1,5 @@ +"use strict"; +// Globals + +const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); +const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm"); diff --git a/toolkit/components/messaging-system/targeting/test/unit/test_targeting.js b/toolkit/components/messaging-system/targeting/test/unit/test_targeting.js new file mode 100644 index 000000000000..aa27ebef7ab5 --- /dev/null +++ b/toolkit/components/messaging-system/targeting/test/unit/test_targeting.js @@ -0,0 +1,206 @@ +const { TargetingContext } = ChromeUtils.import( + "resource://messaging-system/targeting/Targeting.jsm" +); +const { TelemetryTestUtils } = ChromeUtils.import( + "resource://testing-common/TelemetryTestUtils.jsm" +); + +add_task(async function instance_with_default() { + let targeting = new TargetingContext(); + + let res = await targeting.eval( + `ctx.locale == '${Services.locale.appLocaleAsBCP47}'` + ); + + Assert.ok(res, "Has local context"); +}); + +add_task(async function instance_with_context() { + let targeting = new TargetingContext({ bar: 42 }); + + let res = await targeting.eval("ctx.bar == 42"); + + Assert.ok(res, "Merge provided context with default"); +}); + +add_task(async function eval_1_context() { + let targeting = new TargetingContext(); + + let res = await targeting.eval("custom1.bar == 42", { custom1: { bar: 42 } }); + + Assert.ok(res, "Eval uses provided context"); +}); + +add_task(async function eval_2_context() { + let targeting = new TargetingContext(); + + let res = await targeting.eval("custom1.bar == 42 && custom2.foo == 42", { + custom1: { bar: 42 }, + custom2: { foo: 42 }, + }); + + Assert.ok(res, "Eval uses provided context"); +}); + +add_task(async function eval_multiple_context() { + let targeting = new TargetingContext(); + + let res = await targeting.eval( + "custom1.bar == 42 && custom2.foo == 42 && custom3.baz == 42", + { custom1: { bar: 42 }, custom2: { foo: 42 } }, + { custom3: { baz: 42 } } + ); + + Assert.ok(res, "Eval uses provided context"); +}); + +add_task(async function eval_multiple_context_precedence() { + let targeting = new TargetingContext(); + + let res = await targeting.eval( + "custom1.bar == 42 && custom2.foo == 42", + { custom1: { bar: 24 }, custom2: { foo: 24 } }, + { custom1: { bar: 42 }, custom2: { foo: 42 } } + ); + + Assert.ok(res, "Last provided context overrides previously defined ones."); +}); + +add_task(async function eval_evalWithDefault() { + let targeting = new TargetingContext({ foo: 42 }); + + let res = await targeting.evalWithDefault("foo == 42"); + + Assert.ok(res, "Eval uses provided context"); +}); + +add_task(async function log_targeting_error_events() { + let ctx = { + get foo() { + throw new Error("unit test"); + }, + }; + let targeting = new TargetingContext(ctx); + let stub = sinon.stub(targeting, "_sendUndesiredEvent"); + + await Assert.rejects( + targeting.evalWithDefault("foo == 42", ctx), + /unit test/, + "Getter should throw" + ); + + Assert.equal(stub.callCount, 1, "Error event was logged"); + let { + args: [{ event, value }], + } = stub.firstCall; + Assert.equal(event, "attribute_error", "Correct error message"); + Assert.equal(value, "foo", "Correct attribute name"); +}); + +add_task(async function eval_evalWithDefault_precedence() { + let targeting = new TargetingContext({ region: "space" }); + let res = await targeting.evalWithDefault("region != 'space'"); + + Assert.ok(res, "Custom context does not override TargetingEnvironment"); +}); + +add_task(async function eval_evalWithDefault_combineContexts() { + let combinedCtxs = TargetingContext.combineContexts({ foo: 1 }, { foo: 2 }); + let targeting = new TargetingContext(combinedCtxs); + let res = await targeting.evalWithDefault("foo == 1"); + + Assert.ok(res, "First match is returned for combineContexts"); +}); + +add_task(async function log_targeting_error_events_in_namespace() { + let ctx = { + get foo() { + throw new Error("unit test"); + }, + }; + let targeting = new TargetingContext(ctx); + let stub = sinon.stub(targeting, "_sendUndesiredEvent"); + let catchStub = sinon.stub(); + + try { + await targeting.eval("ctx.foo == 42"); + } catch (e) { + catchStub(); + } + + Assert.equal(stub.callCount, 1, "Error event was logged"); + let { + args: [{ event, value }], + } = stub.firstCall; + Assert.equal(event, "attribute_error", "Correct error message"); + Assert.equal(value, "ctx.foo", "Correct attribute name"); + Assert.ok(catchStub.calledOnce, "eval throws errors"); +}); + +add_task(async function log_timeout_errors() { + let ctx = { + timeout: 1, + get foo() { + return new Promise(() => {}); + }, + }; + + let targeting = new TargetingContext(ctx); + let stub = sinon.stub(targeting, "_sendUndesiredEvent"); + let catchStub = sinon.stub(); + + try { + await targeting.eval("ctx.foo"); + } catch (e) { + catchStub(); + } + + Assert.equal(catchStub.callCount, 1, "Timeout error throws"); + Assert.equal(stub.callCount, 1, "Timeout event was logged"); + let { + args: [{ event, value }], + } = stub.firstCall; + Assert.equal(event, "attribute_timeout", "Correct error message"); + Assert.equal(value, "ctx.foo", "Correct attribute name"); +}); + +add_task(async function test_telemetry_event_timeout() { + Services.telemetry.clearEvents(); + let ctx = { + timeout: 1, + get foo() { + return new Promise(() => {}); + }, + }; + let expectedEvents = [ + ["messaging_experiments", "targeting", "attribute_timeout", "ctx.foo"], + ]; + let targeting = new TargetingContext(ctx); + + try { + await targeting.eval("ctx.foo"); + } catch (e) {} + + TelemetryTestUtils.assertEvents(expectedEvents); + Services.telemetry.clearEvents(); +}); + +add_task(async function test_telemetry_event_error() { + Services.telemetry.clearEvents(); + let ctx = { + get bar() { + throw new Error("unit test"); + }, + }; + let expectedEvents = [ + ["messaging_experiments", "targeting", "attribute_error", "ctx.bar"], + ]; + let targeting = new TargetingContext(ctx); + + try { + await targeting.eval("ctx.bar"); + } catch (e) {} + + TelemetryTestUtils.assertEvents(expectedEvents); + Services.telemetry.clearEvents(); +}); diff --git a/toolkit/components/messaging-system/targeting/test/unit/xpcshell.ini b/toolkit/components/messaging-system/targeting/test/unit/xpcshell.ini new file mode 100644 index 000000000000..3653c7f54950 --- /dev/null +++ b/toolkit/components/messaging-system/targeting/test/unit/xpcshell.ini @@ -0,0 +1,6 @@ +[DEFAULT] +head = head.js +tags = messaging-system +firefox-appdir = browser + +[test_targeting.js] diff --git a/toolkit/components/normandy/lib/RecipeRunner.jsm b/toolkit/components/normandy/lib/RecipeRunner.jsm index ceea037abf6d..d0215d43dead 100644 --- a/toolkit/components/normandy/lib/RecipeRunner.jsm +++ b/toolkit/components/normandy/lib/RecipeRunner.jsm @@ -24,6 +24,7 @@ XPCOMUtils.defineLazyModuleGetters(this, { Storage: "resource://normandy/lib/Storage.jsm", FilterExpressions: "resource://gre/modules/components-utils/FilterExpressions.jsm", + TargetingContext: "resource://messaging-system/targeting/Targeting.jsm", NormandyApi: "resource://normandy/lib/NormandyApi.jsm", ClientEnvironment: "resource://normandy/lib/ClientEnvironment.jsm", CleanupManager: "resource://normandy/lib/CleanupManager.jsm", @@ -539,8 +540,9 @@ var RecipeRunner = { } const context = this.getFilterContext(recipe); + const targetingContext = new TargetingContext(); try { - if (await FilterExpressions.eval(recipe.filter_expression, context)) { + if (await targetingContext.eval(recipe.filter_expression, context)) { yield BaseAction.suitability.FILTER_MATCH; } else { yield BaseAction.suitability.FILTER_MISMATCH; diff --git a/toolkit/components/telemetry/Events.yaml b/toolkit/components/telemetry/Events.yaml index 17b5a33cc3f2..0f74a2fce28a 100644 --- a/toolkit/components/telemetry/Events.yaml +++ b/toolkit/components/telemetry/Events.yaml @@ -922,6 +922,26 @@ messaging_experiments: branches: > A semicolon separated string for all the qualified branch ID(s). e.g. "control;variant_01;treatment_02". + targeting: + objects: + - attribute_error + - attribute_timeout + methods: + - targeting + release_channel_collection: opt-out + products: + - firefox + record_in_processes: + - main + description: > + Record generic JEXL errors that result from issues with experiment or + message targeting expressions. The value field contains the namespace and + attribute name that caused the error. + bug_numbers: + - 1644743 + notification_emails: + - ujet@mozilla.com + expiry_version: never # This category contains event entries used for Telemetry tests. # They will not be sent out with any pings.