1775312 - Glean default search engine data is reporting errors when submitting an empty submission URL

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Mark Banner (:standard8)]

When submitting the default search engine data to glean, we will sometimes submit undefined as the value for the submission url.

This is because we do not report the submission URL for every engine - only specific ones that we're interested in.

The problem here is because we're using the application lifetime, we need to be able to clear the submission URL if we move from an engine where we normally submit the url to one where we don't.

Comment 1

[Mark Banner (:standard8)]

Talking with the Glean team, the best option for now is to report a sentinel value for this case, e.g. sentinel: or blank:. The added advantage is that it gives us a clear indication that we intentionally did not submit a value.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1775312 - When reporting an empty submission url to Glean, use a dummy value as it requires a non-empty string. r?mak

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1775312 - Add simple tests for user and policy engines. r?mak

Depends on D149929

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e53862a3c3d
When reporting an empty submission url to Glean, use a dummy value as it requires a non-empty string. r=mak
https://hg.mozilla.org/integration/autoland/rev/881b04553e0d
Add simple tests for user and policy engines. r=mak

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7e53862a3c3d
https://hg.mozilla.org/mozilla-central/rev/881b04553e0d

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 9282286 [details]
Bug 1775312 - When reporting an empty submission url to Glean, use a dummy value as it requires a non-empty string. r?mak

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes a telemetry reporting issue with default search engines which is raising errors and incorrectly reporting.

Note, this only needs inclusion in the next available dot release, we are not asking for the first 102.0.0esr release.

• User impact if declined: N/A. Fixes some new telemetry reporting
• Fix Landed on Version: 103.0
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial change to handle the telemetry reporting better and avoid errors being reported.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9282286 [details]
Bug 1775312 - When reporting an empty submission url to Glean, use a dummy value as it requires a non-empty string. r?mak

Approved for 102.1esr.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/2fdbc9b0564f
https://hg.mozilla.org/releases/mozilla-esr102/rev/893b8217d060

Comment 9

[Natalia Csoregi [:nataliaCs]]

Backed out from esr102 for failures on test_policyEngine.js

• backout: https://hg.mozilla.org/releases/mozilla-esr102/rev/0b2e663d2f2498c679dff628ede5e0b451e9ccdc
• push: https://treeherder.mozilla.org/jobs?repo=mozilla-esr102&revision=70254fba3bdcbe9a0e2a15bc6c67587d2f4d8384&group_state=expanded
• failure log: https://treeherder.mozilla.org/logviewer?job_id=382660466&repo=mozilla-esr102&lineNumber=2015

[task 2022-06-27T19:39:06.880Z] 19:39:06     INFO -  TEST-START | xpcshell.ini:toolkit/components/search/tests/xpcshell/test_policyEngine.js
[task 2022-06-27T19:39:08.459Z] 19:39:08  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/search/tests/xpcshell/test_policyEngine.js | xpcshell return code: 0
[task 2022-06-27T19:39:08.465Z] 19:39:08     INFO -  TEST-INFO took 1580ms
[task 2022-06-27T19:39:08.465Z] 19:39:08     INFO -  >>>>>>>
[task 2022-06-27T19:39:08.465Z] 19:39:08     INFO -  PID 4072 | DLL blocklist was unable to intercept AppInit DLLs.
[task 2022-06-27T19:39:08.465Z] 19:39:08     INFO -  PID 4072 | [Parent 4072, Main Thread] WARNING: Failed to get directory to cache.: file /builds/worker/checkouts/gecko/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:124
[task 2022-06-27T19:39:08.466Z] 19:39:08     INFO -  PID 4072 | [Parent 4072, Main Thread] WARNING: Failed to get directory to cache.: file /builds/worker/checkouts/gecko/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:124
[task 2022-06-27T19:39:08.466Z] 19:39:08     INFO -  PID 4072 | [Parent 4072, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp:2981
[task 2022-06-27T19:39:08.466Z] 19:39:08     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2022-06-27T19:39:08.467Z] 19:39:08     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2022-06-27T19:39:08.467Z] 19:39:08     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2022-06-27T19:39:08.467Z] 19:39:08     INFO -  running event loop
[task 2022-06-27T19:39:08.467Z] 19:39:08     INFO -  xpcshell.ini:toolkit/components/search/tests/xpcshell/test_policyEngine.js | Starting setup
[task 2022-06-27T19:39:08.467Z] 19:39:08     INFO -  (xpcshell/head.js) | test setup pending (2)
[task 2022-06-27T19:39:08.467Z] 19:39:08     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)

Comment 10

[Mark Banner (:standard8)]

Attachment: Add simple tests - ESR patch

Ah yes, I'd forgotten we'd changed some code there, and this only needed a tweak in tests.

Please could this version of the second patch land alongside the main patch.

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/releases/mozilla-esr102/rev/7087b7dbee4c
https://hg.mozilla.org/releases/mozilla-esr102/rev/263f78bccafc