&& and || operators don't short-circuit in mozjexl

[aminomancer]

This is also tracked on bugzilla bug 1921638.

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.

[aminomancer]

We learned that the operators don't short-circuit in mozjexl. It can be tested in about:asrouter by setting browserSettings to null in the Targeting page (you can edit each field individually), then scrolling up to the JEXL evaluator and running:

true || browserSettings.foo

^ This will throw instead of returning true.

browserSettings && browserSettings.something

^ This will throw instead of returning null.

However, accessing properties of some random undefined attribute will not throw, so it seems to be handled differently:

true || nonExistentAttribute.foo

We think TomFrost/Jexl@ab2233a might be portable to mozjexl.

[aminomancer]

Blocked by bug 1778535, which is blocked by #33.