1401249 - getStringPref in clicktoplay-rollout addon returns null but was expected to return undefined

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

+++ This bug was initially created as a clone of Bug #1397379 +++

In bug 1357517, Preferences.get was replaced by getStringPref.  In bug 1397379, we added an `undefined` parameter to it as the default return value, to avoid it throwing an error.

That worked in preventing the error from being thrown, but turns out that the value returned is actually `null` instead of `undefined` (as documented in bug 1357517 comment 63), but the switch statement below only checks for `undefined`.


[Tracking Requested - why for this release]:
Breaks the Flash Click-to-Play rollout for fresh profiles. Note: this is *not* a regression from bug 1390705. It is from bug 1357517, which should have been fixed by 1397379, but the fix was incomplete.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1401249 - Fix getStringPref usage in clicktoplay-rollout.

getStringPref(.., undefined) actually returns null instead of undefined.

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

Comment 2

[:Gijs (he/him)]

Comment on attachment 8909869 [details]
Bug 1401249 - Fix getStringPref usage in clicktoplay-rollout.

https://reviewboard.mozilla.org/r/181366/#review186590

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d94508124715
Fix getStringPref usage in clicktoplay-rollout. r=Gijs

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8909869 [details]
Bug 1401249 - Fix getStringPref usage in clicktoplay-rollout.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1357517
[User impact if declined]: Flash will fail to go Click-to-Play for *new* profiles.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not applicable, as it depends on the update channel being "release"
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Mihai discovered this on bug 1390705. Once we have a new RC with this fix in place, he should retest it.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just fixes the flash clicktoplay rollout, doesn't affect any other areas of the code.
[String changes made/needed]: none

Comment 5

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

Comment on attachment 8909869 [details]
Bug 1401249 - Fix getStringPref usage in clicktoplay-rollout.

Fix for click to play system addon, so that new users will get this option. 
This should be in the 56 RC2 later on this week.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-release/rev/2f265cbbebc6e5797cb5ab39eaa1be2c70f2fb29

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 8

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to verify this issue on Firefox 57.0a1 (2017-09-19) (64-bit), under Windows 10x64.
Click-to-Play staged rollout is set to 1.4 by default and Shockwave Flash is set to 'Ask to Activate' by default. 
I'm not marking this issue Verified Fix until I can confirm the fix also on Firefox 56.

Comment 9

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to verify this issue also on Firefox 56.0-build3, under Windows 10x64, macOS 10.12 and under Ubuntu 16.04x64. Shockwave Flash is set by default to 'Ask to Activate' and Click-to-Play staged rollout is set to 1.4.