1385396 - Calling setExperimentActive forces graphics initialization

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: stack

Calling setExperimentActive forces graphics initialization due to getGfxField("ContentBackend", null) being called [1]

http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/telemetry/TelemetryEnvironment.jsm#1527

Hopefully there's some way around that, so that an experiment that starts using setExperimentActive doesn't cause the startup order to change.

We first notice this because this was causing the BrowserTabsRemoteAutostart getter to run too early and cache a wrong value.

Stack attached

Comment 1

[Georg Fritzsche [:gfritzsche]]

We should just cache the data from any early startup calls to setExperimentActive() and not initialize the global environment.
Then when the experiment global is actually initialized we can pick that cached data up.

Comment 2

[Doug Thayer [:dthayer] (he/him)]

Ran into this same problem working on Bug 1361500. Should we add some asserts in BrowserTabsRemoteAutostart to make it faster to diagnose these problems when they happen?

In any case, I'll work on this unless anyone else was already planning to.

Comment 3

[Doug Thayer [:dthayer] (he/him)]

Attachment: Bug 1385396 - Cache early setExperimentActive calls

Calling setExperimentActive too early during startup can change
the order of some initialization. setExperimentActive probably
shouldn't have this kind of effect, so simply cache early calls
to it until gGlobalEnvironment has been initialized through other
functions.

Additionally, I am speculatively including work to ensure that
setExperimentInactive and getActiveExperiments have the same
behavior, while remaining correct by working from the same cache
that setExperimentActive uses.

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

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

https://reviewboard.mozilla.org/r/164118/#review169732

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:62
(Diff revision 1)
> +    for (const [id, {branch, options}] of gActiveExperimentStartupBuffer.entries()) {
> +      gGlobalEnvironment.setExperimentActive(id, branch, options);
> +    }
> +    gActiveExperimentStartupBuffer = null;

The other initialization happens inside `function EnvironmentCache()`, lets move this there as well.

Comment 5

[Doug Thayer [:dthayer] (he/him)]

Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

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

Comment 6

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Because bug 1367129 landed in 56

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

https://reviewboard.mozilla.org/r/164118/#review170218

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:873
(Diff revisions 1 - 2)
>    }
>  
> +  for (const [id, {branch, options}] of gActiveExperimentStartupBuffer.entries()) {
> +    this.setExperimentActive(id, branch, options);
> +  }
> +  gActiveExperimentStartupBuffer = null;

This fails test_TelemetryEnvironment.js with: "gActiveExperimentStartupBuffer is null at resource://gre/modules/TelemetryEnvironment.jsm:870"

You need to reset `gActiveExperimentStartupBuffer` in `testCleanRestart()`.

Comment 8

[Doug Thayer [:dthayer] (he/him)]

Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

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

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

https://reviewboard.mozilla.org/r/164118/#review170442

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aba83d3d9a3b
Cache early setExperimentActive calls r=gfritzsche

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/aba83d3d9a3b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 13

[Doug Thayer [:dthayer] (he/him)]

Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression, but needed for Bug 1367129.
[User impact if declined]: The e10s cohort will be cached incorrectly.
[Is this code covered by automated tests?]: Partially. The out of order startup case is not, but the existing uses of the API are well covered. I manually tested the startup reordering by adding early setExperimentActive calls and observing my e10s prefs being ignored, and then applying the fix to see them working correctly.
[Has the fix been verified in Nightly?]: No. The code that prompted this change is specific to beta and release. The only way at present to verify this fix is to edit a local build, as detailed above.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The only code paths affected by this change are code paths that already cause broken e10s behavior without it.
[String changes made/needed]: None.

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

Needed for e10s metrics. Let's uplift for 56 beta 1.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ea9b41d34bde