1466490 - Add multi-process xpcshell test for scalar semantics

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P2)

Description

[Alessio Placitelli [:Dexter]]

In bug 1454606 we landed the code to preserve the scalar semantics in GeckoView, during measurements deserialization. Test coverage for that was added through gtest, which does not support content processes yet.

We should add xpcshell test coverage for making sure we're correctly preserving the scalar semantics with multiple processes on GeckoView.

Comment 1

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1466490 - Actually record non-main scalars on the main process after deserialization/recording.

Review commit: https://reviewboard.mozilla.org/r/251150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251150/

Comment 2

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1466490 - Set Telemetry preference on startup.

This follows the documentation example:

* Scalar deserialization is started
* “test” scalar is incremented by “10” by the application -> The operation [test, add, 10] is recorded into the list.
* The state of the “test” scalar is loaded off the persistence file, and the value “14” is set.
* Deserialization is finished and the pending operations are applied.
* The “test” scalar is incremented by “10”, the value is now “24”
* “test” scalar is incremented via “scalarAdd” by 1. Its value is “25”

It does that in both the parent and the content process.

Review commit: https://reviewboard.mozilla.org/r/251152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251152/

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985539 [details]
Bug 1466490 - Actually record non-main scalars on the main process after deserialization/recording.

https://reviewboard.mozilla.org/r/251150/#review257408

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review257410

::: toolkit/components/telemetry/TelemetryScalar.h:9
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef TelemetryScalar_h__
>  #define TelemetryScalar_h__
>  
> +#include "nsTArray.h"

mh, why is this needed?

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review257412

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:25
(Diff revision 1)
> + * Run a file in the content process.
> + * @param aFileName - The file to execute in the content process.
> + * @return {Promise} A promise resolved after the execution in the other process
> + *         finishes.
> + */
> +async function run_in_child(aFileName) {

Let's add all the shared functions to a new header file, "head_GeckoView.js".

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:41
(Diff revision 1)
> +
> +/**
> + * Builds a promise to wait for the GeckoView data loading to finish.
> + * @return {Promise} A promise resolved when the data loading finishes.
> + */
> +function waitGeckoViewLoadComplete() {

Ditto

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:57
(Diff revision 1)
> + * snapshot of the relevant process.
> + * @param aScalarName - The name of the scalar to look for.
> + * @param aProcessName - The name of the process to look in.
> + * @param aKeyed - Whether or not to look in keyed snapshots.
> + */
> +async function waitForSnapshotData(aScalarName, aProcessName, aKeyed) {

Ditto

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:81
(Diff revision 1)
> +  let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +  env.set("MOZ_ANDROID_DATA_DIR", profileDir.path);
> +});
> +
> +add_task(async function test_MultiprocessScalarSemantics() {
> +  Telemetry.clearScalars();

Can you add a brief comment about the purpose of this test? Maybe just copy the example from the docs

::: toolkit/components/telemetry/tests/unit/xpcshell.ini:27
(Diff revision 1)
>    system.xpi
>    restartless.xpi
>  
>  [test_GeckoView.js]
>  skip-if = os == "android" # Disabled due to crashes (see bug 1331366)
>  head =

Reference the header file here (and in the other test)

Comment 6

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1466490 - Extract common test functionality into a header.

Review commit: https://reviewboard.mozilla.org/r/251210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251210/

Comment 7

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251152/diff/1-2/

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985675 [details]
Bug 1466490 - Extract common test functionality into a header.

https://reviewboard.mozilla.org/r/251210/#review257544

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review257556

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3891caf027f
Actually record non-main scalars on the main process after deserialization/recording. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/95048dd368eb
Extract common test functionality into a header. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/5f13ed9de8a3
Add multi-process xpcshell test for scalar semantics. r=Dexter

Comment 11

[Cosmin Sabou [:CosminS]]

Backed out for ESlint failures on test_GeckoView_ScalarSemantics

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=5f13ed9de8a3c256e934fdd5fff4cdf712baadac&tochange=f189e0da7e193f204a7f962bc2b1796a3cdddcbf&selectedJob=183385659

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183385659&repo=autoland&lineNumber=271

Backout link: https://hg.mozilla.org/integration/autoland/rev/f189e0da7e193f204a7f962bc2b1796a3cdddcbf

Comment 12

[Cosmin Sabou [:CosminS]]

Also please take a look at this failure: https://treeherder.mozilla.org/logviewer.html#?job_id=183388757&repo=autoland&lineNumber=1849

Comment 13

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251152/diff/2-3/

Comment 14

[Jan-Erik Rediger [:janerik]]

Lint fixed. Looking into test failure, might be blocked by one of the other issues we encountered this week around (de)serialization.

Comment 15

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1466490 - Add multi-process xpcshell test for scalar semantics.

This follows the documentation example:

* Scalar deserialization is started
* “test” scalar is incremented by “10” by the application -> The operation [test, add, 10] is recorded into the list.
* The state of the “test” scalar is loaded off the persistence file, and the value “14” is set.
* Deserialization is finished and the pending operations are applied.
* The “test” scalar is incremented by “10”, the value is now “24”

It does that in both the parent and the content process.

Review commit: https://reviewboard.mozilla.org/r/251634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251634/

Comment 16

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251152/diff/3-4/

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review258128

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:8
(Diff revision 4)
>  */
>  "use strict";
>  
>  ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm", this);
>  ChromeUtils.import("resource://gre/modules/Services.jsm", this);
> +ChromeUtils.import("resource://gre/modules/TelemetryController.jsm", this);

We are not exporting this module on GeckoView, so we should probably not be importing this (even though these xpcshell tests, for now, run on other platforms).

The best way forward would be to move the code to a shared location (e.g. `TelemetryUtils.jsm`?) and use it in both places. Let's not block on this for this bug, but please file a new bug and leave a comment in the code mentioning its number.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:69
(Diff revision 4)
> +
> +if (runningInParent) {
> +  // Set logging preferences for all the tests.
> +  Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> +  // Telemetry archiving should be on.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ArchiveEnabled, true);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:71
(Diff revision 4)
> +  // Set logging preferences for all the tests.
> +  Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> +  // Telemetry archiving should be on.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ArchiveEnabled, true);
> +  // Telemetry xpcshell tests cannot show the infobar.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.BypassNotification, true);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:73
(Diff revision 4)
> +  // Telemetry archiving should be on.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ArchiveEnabled, true);
> +  // Telemetry xpcshell tests cannot show the infobar.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.BypassNotification, true);
> +  // FHR uploads should be enabled.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:77
(Diff revision 4)
> +  // FHR uploads should be enabled.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> +  // Many tests expect the shutdown and the new-profile to not be sent on shutdown
> +  // and will fail if receive an unexpected ping. Let's globally disable these features:
> +  // the relevant tests will enable these prefs when needed.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:78
(Diff revision 4)
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> +  // Many tests expect the shutdown and the new-profile to not be sent on shutdown
> +  // and will fail if receive an unexpected ping. Let's globally disable these features:
> +  // the relevant tests will enable these prefs when needed.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:80
(Diff revision 4)
> +  // and will fail if receive an unexpected ping. Let's globally disable these features:
> +  // the relevant tests will enable these prefs when needed.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);
> +  Services.prefs.setBoolPref("toolkit.telemetry.newProfilePing.enabled", false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FirstShutdownPingEnabled, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:82
(Diff revision 4)
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);
> +  Services.prefs.setBoolPref("toolkit.telemetry.newProfilePing.enabled", false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FirstShutdownPingEnabled, false);
> +  // Turn off Health Ping submission.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.HealthPingEnabled, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:89
(Diff revision 4)
> +  // Non-unified Telemetry (e.g. Fennec on Android) needs the preference to be set
> +  // in order to enable Telemetry.
> +  if (Services.prefs.getBoolPref(TelemetryUtils.Preferences.Unified, false)) {
> +    Services.prefs.setBoolPref(TelemetryUtils.Preferences.OverridePreRelease, true);
> +  } else {
> +    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);

I don't think we need the `Unified` check, that's Fennec specific. If we need `TelemetryEnabled` to be true, let's always set it to true.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:93
(Diff revision 4)
> +  } else {
> +    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);
> +  }
> +}
> +
> +TelemetryController.testInitLogging();

Do we really need this?

Comment 18

[Alessio Placitelli [:Dexter]]

Comment on attachment 8986243 [details]
Bug 1466490 - Add multi-process xpcshell test for scalar semantics.

https://reviewboard.mozilla.org/r/251634/#review258134

Comment 19

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251152/diff/4-5/

Comment 20

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8986243 [details]
Bug 1466490 - Add multi-process xpcshell test for scalar semantics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251634/diff/1-2/

Comment 21

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review258152

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6870eb4e9daa
Actually record non-main scalars on the main process after deserialization/recording. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/cd3ecb04a7bc
Extract common test functionality into a header. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/d282d6a15965
Set Telemetry preference on startup. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/eb0d3d9a16c6
Add multi-process xpcshell test for scalar semantics. r=Dexter

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6870eb4e9daa
https://hg.mozilla.org/mozilla-central/rev/cd3ecb04a7bc
https://hg.mozilla.org/mozilla-central/rev/d282d6a15965
https://hg.mozilla.org/mozilla-central/rev/eb0d3d9a16c6