1835742 - needsUpdate targeting attribute throws while offline, breaking ASRouter initialization

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Shane Hughes [:aminomancer]]

The needsUpdate attribute intentionally throws if the update check fails.

I think this is fine for message targeting evaluation, but it causes a problem for this method, which basically resolves the whole targeting environment on every setState call if devtools are enabled. The exception causes the Promise.all in that method to reject as well. This method is only intended for use with the ASRouter devtools, but in order to keep the state rendered in the devtools up to date, we update it every time the internal ASRouter state changes. So when ASRouter.setState is called, it invokes this getTargetingParameters method, and never resolves.

And because ASRouter's own init method calls setState before loading providers, message groups, messages, etc., an unhandled exception in targeting attribute resolution can stop ASRouter from ever loading any messages. Most targeting attributes don't throw under any circumstances, but we don't do a lot of testing offline, naturally. In this case, being offline causes the update service's check to fail, which causes the attribute getter to throw.

I took another approach to resolving this in bug 1833913, to change getTargetingParameters so that it handles exceptions in targeting getters. This doesn't affect JEXL evaluation, just the evaluation that happens when we inject targeting environment state into ASRouter devtools.

I'm not aware of any other kinds of evaluators. But it still seems worth considering if we want targeting attributes to intentionally throw under any circumstances. Would it be preferable for it to just return false or undefined or some other default value? Aside from the risk of causing failures in ASRouter and related code, I don't think that changes anything.

In JEXL, these unhandled exceptions result in an undefined result. But you can see what happens in practice if you go offline and test this in console:

var { QueryCache, ASRouterTargeting } = ChromeUtils.import(
  "resource://activity-stream/lib/ASRouterTargeting.jsm"
);
var { ASRouter } = ChromeUtils.import(
  "resource://activity-stream/lib/ASRouter.jsm"
);
QueryCache.expireAll();
var result1 = await ASRouter.evaluateExpression({
  expression: "!needsUpdate",
  context: ASRouterTargeting.Environment,
});
console.log('result1 :>> ', result1.evaluationStatus.result);
// result1 :>> undefined
var result2 = await ASRouter.evaluateExpression({
  expression: "!needsUpdate",
  context: ASRouterTargeting.Environment,
});
console.log('result2 :>> ', result2.evaluationStatus.result);
// result2 :>> true

Basically, the first time the targeting attribute is used, its value hasn't been cached, so it uses the update service, which fails. So it throws, and you can see the error logged in the console. You can also see what it's doing if you set breakpoints inside the targeting getter.

On the first call, it makes it here, throws, and ultimately the targeting expression evaluation result is undefined.

On the second call, it hasn't cached a value, but it has recorded the _lastUpdated time from the first call. So now it returns early, before it can throw. The return value is null since that's the default value.

So in practice, this means that while offline, the value of needsUpdate is (almost) always falsy. The only time this matters is on the very first time the targeting attribute is used — on ASRouter initialization. So we may as well just return false instead of throwing. It reduces the risk of something going wrong, and it shouldn't change the results of any real message's targeting evaluation.

Comment 1

[Shane Hughes [:aminomancer]]

Ultimately, the change I'm proposing is pretty simple:

diff --git a/browser/components/newtab/lib/ASRouterTargeting.jsm b/browser/components/newtab/lib/ASRouterTargeting.jsm
index 54f05c1cb8ef3..8d9fa4af73338 100644
--- a/browser/components/newtab/lib/ASRouterTargeting.jsm
+++ b/browser/components/newtab/lib/ASRouterTargeting.jsm
@@ -254,7 +254,7 @@ function CheckBrowserNeedsUpdate(
       );
       let result = await check.result;
       if (!result.succeeded) {
-        throw result.request;
+        return false;
       }
       checker._value = !!result.updates.length;
       return checker._value;

Edit: could also do this without losing the console message

-        throw result.request;
+        lazy.ASRouterPreferences.console.error(
+          "CheckBrowserNeedsUpdate failed :>> ",
+          result.request
+        );
+        return false;

Comment 2

[Shane Hughes [:aminomancer]]

Attachment: Bug 1835742 - Fix ASRouter initialization with network disabled and devtools enabled. r=#omc-reviewers

Change the needsUpdate targeting attribute so it no longer throws if an
update check failed, e.g. due to being offline. Also change how the
targeting environment is evaluated for generating devtools state, so
that encountering an exception in a targeting attribute does not cause
ASRouter initialization to fail.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:aminomancer, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/521a8641a60a
Fix ASRouter initialization with network disabled and devtools enabled. r=omc-reviewers,emcminn

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/521a8641a60a