1750756 - test_telemetryController.js fails on Android with GPU process enabled

Status: RESOLVED FIXED

(GeckoView Graveyard :: Sandboxing, defect)

Description

[Jamie Nicol [:jnicol]]

The test test_telemetryEnabledUnexpectedValue fails with the following warning and error message:

WARNING: Rejected attempt to change type of pref toolkit.telemetry.enabled's user value from bool to string: file /home/jamie/src/gecko/modules/libpref/Preferences.cpp:1668
...
FAIL toolkit/components/telemetry/tests/unit/test_TelemetryController.js - xpcshell return code: 0
ERROR Unexpected exception NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setStringPref]
Preferences._set@resource://gre/modules/Preferences.jsm:126:24
Preferences.set@resource://gre/modules/Preferences.jsm:115:8
test_telemetryEnabledUnexpectedValue@test_TelemetryController.js:633:17
_run_next_test/<@/data/local/tmp/test_root/xpc/head.js:1673:22
_run_next_test@/data/local/tmp/test_root/xpc/head.js:1673:38
run@/data/local/tmp/test_root/xpc/head.js:819:9
_do_main@/data/local/tmp/test_root/xpc/head.js:240:6
_execute_test@/data/local/tmp/test_root/xpc/head.js:604:5
@-e:1:1

This test only runs on android because we skip it if toolkit.telemetry.unified is true.

The test starts by calling defaultPrefBranch.deleteBranch(TelemetryUtils.Preferences.TelemetryEnabled) to delete the existing pref. It then attempts to set the telemetry enabled pref to a string value. It then asserts that telemetry is disabled because the pref is invalid (because it's a string).

However, we can see from the warning that we encountered an error when setting the pref to a string, because the pref already exists as a bool.

We can see that deleteBranch() successfully removes the pref from HashTable() here. However, in the subsequent pref_SetPref() call we find an existing pref here in gSharedMap. It doesn't matter that we deleted the pref from HashTable() as we don't even look that up if we find the pref in gSharedMap first.

If the GPU process is disabled then gSharedMap is null. This means we look up the pref in HashTable(), where it has indeed been deleted from. Meaning we cannot find an existing pref, and can therefore successfully set it to a string.

But if the GPU process is enabled then gSharedMap is non-null due to this call, which calls through to here.

It doesn't seem to me like the GPU process is doing anything wrong here - the other process host types also call SerializeToSharedMemory() - I think we just happen to be the only one launched by xpcshell tests on Android. What I'm unsure of is whether the test is relying on invalid behaviour, assuming that it can successfully delete a pref. Or whether the preference code should be allowing it to do that.

Kris and Chris, any ideas how to proceed with this? I see you are both active in Preferences.cpp and test_TelemetryController.js respectively, but please forward the needinfo on if there's someone who'd be better able to answer!

Comment 1

[Chris H-C :chutten]

(In reply to Jamie Nicol [:jnicol] from comment #0)

This test only runs on android because we skip it if toolkit.telemetry.unified is true.

Oh, well, let me stop you there. Non-unified Telemetry is not a supported codepath now that Fennec is no more. It would have been removed as part of defennecstration bug 1577863 if that work had been assigned sufficient priority.

You may remove the test.

Comment 2

[Jamie Nicol [:jnicol]]

Attachment: Bug 1750756 - Remote test_telemetryEnabledUnexpectedValue. r?chutten

This test attempts to set a pref to a value of the wrong type. Prior
to doing so it must delete the existing pref, else we get an error
about changing the pref's type. This failed when we enable the GPU
process on Android, as we are unable to delete the existing pref when
using a shared pref map.

But luckily this test only runs when toolkit.telemetry.unified=false,
which is no longer a supported codepath in this post-fennec world. So
we can simply remove the test.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9536b8469f34
Remote test_telemetryEnabledUnexpectedValue. r=chutten

Comment 4

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/9536b8469f34

Comment 5

[Chris Peterson [:cpeterson]]

Moving GPU process bugs to the new GeckoView::Sandboxing component.