1921638 - mozjexl && and || operators don't short-circuit, causing the sticky clause to fail if there are errors in the rest of the targeting

Status: NEW

(Firefox :: Messaging System, defect, P2)

Description

[Shane Hughes [:aminomancer]]

There are more details here. We're having issues with the following targeting expression:

(browserSettings.update.channel == "release") && ((experiment.slug in activeExperiments) || (((!os.isWindows || os.windowsVersion < 10) || hasActiveEnterprisePolicies || isDefaultHandler.pdf || (defaultPDFHandler.registered && !defaultPDFHandler.knownBrowser)) && (version|versionCompare('131.!') >= 0) && (locale in ['de', 'en-CA', 'en-GB', 'en-US', 'fr', 'it'])))

Ignoring the release channel check, it basically has the pattern stickyClause || mainTargeting. So the sticky clause should be preventing evaluation of all the other stuff. But it seems not to, because the later clause defaultPDFHandler.registered && !defaultPDFHandler.knownBrowser throws a TypeError (bug 1920698). From Beth:

The sticky clause should be preventing the defaultPDFHandler clause from ever being evaluated. I'm very confused about what is going on here.

Unfortunately e don't really have the capability to answer these questions right now. Long term, we would want to build some amount of evaluation debugging into mozjexl.jsm that we can enable in cases like this. We've talked about it before in the scope of submitting telemetry about failed sub-expressions, but that is likely out of the question due to the targeting context including potentially sensitive data (and submitting information about the targeting evaluation would be category 3 or higher telemetry).

In the near term, we can add some logging to mozjexl, compile it, and pull it into a build that we can hand off to someone that can reproduce this issue. I don't know if I have time to do that work, but I can definitely help guide someone towards how to do that work. This would also hopefully be a stepping stone to building a real solution into moxjexl that can provide detailed evaluation and context information.

So we think the first step in debugging why the sticky clause isn't working here would be to work with Beth on adding logging to mozjexl.

Comment 1

[Meg Viar [:mviar]]

:dmosedale noted that we have an incomplete prototype that allows for logging the existing mozjexl interpreter's output alongside that from an updated version. This could be useful for testing. There is also a know issue with mozjexl's comparison precedence (they don't match what's expected / present in most modern languages).

Comment 2

[Shane Hughes [:aminomancer]]

Daniel had some information on a previous rollout that encountered a similar issue, where the sticky clause did not prevent unenrollment if errors were encountered in subsequent parts of the expression.

Comment 3

[Shane Hughes [:aminomancer]]

Our tentative plan:

1. land the mjs fix in github
2. land the parallel interpreters on m-c (bug 1778535)
3. test the mjs parallel interpreter on m-c
4. enable the mjs interpreter
5. land the short-circuiting patch & repeat steps 3 & 4

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Note that 1. and 2. in the previous comment are not yet in landable shape and will need some work. We'll also need to talk to the experimenter folks and get buy-in on the risk-reward tradeoff here.

Comment 5

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:mviar, could you have a look please?

For more information, please visit BugBot documentation.

Comment 6

[Meg Viar [:mviar]]

Hi Shane, any thoughts on severity for this one?