Calling setExperimentActive forces graphics initialization

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
21 days ago
11 days 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])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

21 days ago
Created attachment 8891469 [details]
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

16 days 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

16 days 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)
(Reporter)

Comment 6

15 days ago
[Tracking Requested - why for this release]:
Because bug 1367129 landed in 56
tracking-firefox56: --- → ?
Depends on: 1367129

Comment 7

15 days 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

14 days 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

14 days 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
Last Resolved: 14 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance.
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(dothayer)
(Assignee)

Comment 13

11 days 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?
tracking-firefox56: ? → +
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+

Comment 15

11 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/ea9b41d34bde
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.