1220198 - Telemetry experiments become appDisabled if the checkCompatibility pref is set to false

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Comment 1

[Dave Townsend [:mossop]]

Is this a fix we're going to need to uplift anywhere?

Comment 2

[Vladan Djeric (:vladan)]

Yes, we'll need it on Beta 43 (current Beta)

Comment 3

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1220198: Never appDisable experiments if they don't have the right app compatibility information available. r?rhelmer

Bug 1220198: Never appDisable experiments if they don't have the right app compatibility information available. r?rhelmer

Comment 4

[Dave Townsend [:mossop]]

This patch should be safe for uplifts. I'm going to work on a more detailed patch in bug 1220911 to try to make experiments behave more similarly to other add-ons.

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8682238 [details]
MozReview Request: Bug 1220198: Never appDisable experiments if they don't have the right app compatibility information available. r?rhelmer

https://reviewboard.mozilla.org/r/24011/#review21437

I was going to complain about the redundancy but bug 1220911 is better than the one-off suggestion I was going to make.

Comment 6

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/7a126c47da3e

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a126c47da3e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7a126c47da3e

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8682238 [details]
MozReview Request: Bug 1220198: Never appDisable experiments if they don't have the right app compatibility information available. r?rhelmer

Approval Request Comment
[Feature/regressing bug #]: Telemetry experiments
[User impact if declined]: Possible shutdown hang for users who have "extensions.checkCompatibility" set to false, and receive a telemetry experiment.
There are various experiments in the pipeline now (including one to test e10s on beta), and it would be great to avoid this problem for these users.
[Describe test coverage new/current, TreeHerder]: Patch adds a test
[Risks and why]: The patch remove one of the conditions necessary that leads to the problem, and is a much simpler fix than the fix to the code causing the hang, which is being dealt with in bug 1218266.
The risk would be in the case where the Experiments system installs an experiment targeted to another application (say, Thunderbird), but that's unlikely to happen.
[String/UUID change made/needed]: none

Comment 10

[Dave Townsend [:mossop]]

(In reply to :Felipe Gomes from comment #9)
> [Risks and why]: The patch remove one of the conditions necessary that leads
> to the problem, and is a much simpler fix than the fix to the code causing
> the hang, which is being dealt with in bug 1218266.
> The risk would be in the case where the Experiments system installs an
> experiment targeted to another application (say, Thunderbird), but that's
> unlikely to happen.

And that's also a risk with the current code anyway so really there is no change in risk here.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8682238 [details]
MozReview Request: Bug 1220198: Never appDisable experiments if they don't have the right app compatibility information available. r?rhelmer

Aiming this uplift at beta 2 so that we avoid a crash with the e10s experiment. This includes a new test. Yay tests.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09ed4ec6ff76

Comment 13

[Carsten Book [:Tomcat]]

laned on beta as https://hg.mozilla.org/releases/mozilla-beta/rev/b87f9e2d6355

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/09ed4ec6ff76