Calling setExperimentActive forces graphics initialization

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Felipe, Assigned: dthayer)

Tracking

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56+ fixed, firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments)

Posted file 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
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.
Priority: -- → P1
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Assignee

Comment 2

2 years ago
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.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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 hidden (mozreview-request)
[Tracking Requested - why for this release]:
Because bug 1367129 landed in 56
Depends on: 1367129

Comment 7

2 years ago
mozreview-review
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()`.
Attachment #8893119 - Flags: review?(gfritzsche)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

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

Thanks!
Attachment #8893119 - Flags: review?(gfritzsche) → review+

Comment 10

2 years ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aba83d3d9a3b
Cache early setExperimentActive calls r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/aba83d3d9a3b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance.
Flags: needinfo?(dothayer)
Assignee

Comment 13

2 years ago
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.
Flags: needinfo?(dothayer)
Attachment #8893119 - Flags: approval-mozilla-beta?
Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

Needed for e10s metrics. Let's uplift for 56 beta 1.
Attachment #8893119 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.