Shield fails at startup if a preference experiment sets a previously non-existent pref

RESOLVED FIXED

Status

()

defect
--
major
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mythmon, Assigned: mythmon)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57blocking fixed, firefox58 verified)

Details

User Story

I. Reduced Steps:
1. Create new profile Open/Close FF.
2. Open profile location, edit prefs.js and add two lines:
user_pref("extensions.shield-recipe-client.startupExperimentPrefs.thisshouldnotbehere", 2);
user_pref("thisshouldnotbehere", 2);
3. Open FF.

II. Original Steps: (- not reproducible without certain type of Staging recipe enabled )
1. Create new profile, without opening FF with it.
2. Open profile location and copy/paste the prefs.js from https://bugzilla.mozilla.org/attachment.cgi?id=8926337 
3. Start Firefox.
4. Restart 2 times.

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

2 years ago
# STR

1. Set the following prefs:

extensions.shield-recipe-client.dev_mode = true
extensions.shield-recipe-client.startupExperimentPrefs.testing.does-not-exist = foo

Only the "StartupExperimentPrefs" preference is needed. dev_mode makes this easier to verify.

2. Restart Firefox, and open the browser console

Expected results:

# Shield should run as normal.

# Actual results

Shield throws an error, and stops running.

# Impact

This will prevent Shield from running until a fix is released for Shield, for anyone that runs an experiment that sets a non-existant pref. There is at least one recipe that matches this requirement targeting 57.

# The error:

The error happens on this line of code:

    studyPrefsChanged[prefName] = defaultBranch.getCharPref(prefName);

The code assumes that there exists a value for the preference, due to earlier checks. However, in the case of a non-existant experimental pref, those checks do not hold, causing an exception.

1510262743952	addons.xpi	WARN	Exception running bootstrap method startup on shield-recipe-client@mozilla.org: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/mythmon/fx/nightly/browser/features/shield-recipe-client@mozilla.org.xpi!/bootstrap.js :: initExperimentPrefs :: line 87"  data: no] Stack trace: initExperimentPrefs()@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/mythmon/fx/nightly/browser/features/shield-recipe-client@mozilla.org.xpi!/bootstrap.js:87 < startup()@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/mythmon/fx/nightly/browser/features/shield-recipe-client@mozilla.org.xpi!/bootstrap.js:147 < callBootstrapMethod()@resource://gre/modules/addons/XPIProvider.jsm:4409 < startup()@resource://gre/modules/addons/XPIProvider.jsm:2231 < callProvider()@resource://gre/modules/AddonManager.jsm:263 < _startProvider()@resource://gre/modules/AddonManager.jsm:730 < startup()@resource://gre/modules/AddonManager.jsm:897 < startup()@resource://gre/modules/AddonManager.jsm:3082 < observe()@jar:file:///home/mythmon/fx/nightly/omni.ja!/components/addonManager.js:65
Assignee

Updated

2 years ago
Summary: Shield fails at startup if a preference experiment sets an previously non-existent pref → Shield fails at startup if a preference experiment sets a previously non-existent pref
Assignee

Comment 1

2 years ago
[Tracking Requested - why for this release]:

This will impact Shield studies planned to launch with 57, as well as all other Shield operations.
FYI: we are targeting RC3 for this fix but it should be landed and tested well before GTB Sunday.
Flags: needinfo?(sledru)
Flags: needinfo?(jcristau)
Assignee

Comment 3

2 years ago
The STR in comment 0 seem to be unreliable, and depend on some random sampling and interactions with the server. Adrian and I are working to make those more reliable.
I feel very uncomfortable taking this in rc3. If we make any mistake, we miss the Tuesday date. 
I would prefer to do a dot release the week after for that.
Flags: needinfo?(sledru)
User Story: (updated)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> I feel very uncomfortable taking this in rc3. If we make any mistake, we
> miss the Tuesday date. 
> I would prefer to do a dot release the week after for that.

We're using Shield to give new users different search results composition than the default. The plan is to give 97% of new profiles (also unified) search results first followed by history. We have a few other compositions we're testing with remaining smaller populations. The pref we're targeting is 'browser.urlbar.matchBuckets' which does not exist in Firefox by default. Waiting for the dot release would delay that rollout. +javaun for visibility.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8927096 [details]
Bug 1416003 - Handle preference studies on prefs with only user-branch values

https://reviewboard.mozilla.org/r/198302/#review203682

If it makes relman more comfortable, IMO this is a pretty safe patch. We're basically throwing in fewer cases.

::: browser/extensions/shield-recipe-client/bootstrap.js:108
(Diff revision 2)
> -        default:
> +          default:
> -          // This should never happen
> +            // This should never happen
> -          log.error(`Error getting startup pref ${prefName}; unknown value type ${experimentPrefType}.`);
> +            log.error(`Error getting startup pref ${prefName}; unknown value type ${experimentPrefType}.`);
> -      }
> +        }
> +      } catch (e) {
> +        if (e.message.includes("(NS_ERROR_UNEXPECTED) [nsIPrefBranch.get")) {

Use:

```js
e.result == Cr.NS_ERROR_UNEXPECTED
```

to check this. You'll need to add `Cr` to the destructuring assignment off `Components` - it should equal `Components.results`.
Attachment #8927096 - Flags: review?(gijskruitbosch+bugs) → review+
For now, the plan is the following:
* gijs will complete the patch
* we will land that on m-c directly
* asking for a verification to sv (ni Andrei for that) as soon as the builds are ready
* take that in m-r today or tomorrow for the rc3 (gtb Sunday)
Flags: needinfo?(andrei.vaida)

Comment 11

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/80474174ca66
Handle preference studies on prefs with only user-branch values r=Gijs,a=sylvestre,ship57,etc

Comment 12

2 years ago
As noted, I fixed up the nits I made and landed this on central:

remote:   https://hg.mozilla.org/mozilla-central/rev/80474174ca663daffc37537bc811be5d99bd01a0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Assignee

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8927096 [details]
Bug 1416003 - Handle preference studies on prefs with only user-branch values

https://reviewboard.mozilla.org/r/198302/#review203682

> Use:
> 
> ```js
> e.result == Cr.NS_ERROR_UNEXPECTED
> ```
> 
> to check this. You'll need to add `Cr` to the destructuring assignment off `Components` - it should equal `Components.results`.

The old way also checked that the problem stemmed from getting a preference as well as being NS_ERROR_UNEXPECTED. The new way doesn't check this. I don't think this is a problem, since they code in the try block is doing very little except getting prefs, but I think it is something worth mentioning.
I've tested on 58.0a1 20171110100139, which contains the landed version from comment 12 and the error is handled.

However, with the original STR of this bug (see II. from user story), on the updated version of Nightly  Shield enters in a weird state, which seems to me "almost broken". (I validated the "almost broken" state by changing recipe status from disabled to published on staging Control-Center, restarting FF and checking for the update, which didn't happened).

I've tested similar STR on a manual installed build (with an initial fix) of Shield (Shield Recipe Client 77.18.g3a1687e5) that I got last night from :mythmon and I can't reproduce the "almost broken" state there.

:mythmon, :gijs, could you please take a look?
User Story: (updated)
Flags: needinfo?(mcooper)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(andrei.vaida)

Comment 15

2 years ago
(In reply to Adrian Florinescu [:AdrianSV] from comment #14)
> I've tested on 58.0a1 20171110100139, which contains the landed version from
> comment 12 and the error is handled.
> 
> However, with the original STR of this bug (see II. from user story), on the
> updated version of Nightly  Shield enters in a weird state, which seems to
> me "almost broken". (I validated the "almost broken" state by changing
> recipe status from disabled to published on staging Control-Center,
> restarting FF and checking for the update, which didn't happened).
> 
> I've tested similar STR on a manual installed build (with an initial fix) of
> Shield (Shield Recipe Client 77.18.g3a1687e5) that I got last night from
> :mythmon and I can't reproduce the "almost broken" state there.
> 
> :mythmon, :gijs, could you please take a look?

I am on a train for the next few hours, and I'm not sure about this issue off-hand. mythmon said he'll take a look. I'll try to remain in touch as wifi/cell signal permits during the next few hours. Clearing ni so I can be re-needinfo'd.

Off-hand, I don't see other places where we do similar pref checks, and ISTM that there are other reasons updates wouldn't happen... and anyway, no updates sounds better than none of the code working/running...
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Adrian Florinescu [:AdrianSV] from comment #14)
> I've tested similar STR on a manual installed build (with an initial fix) of
> Shield (Shield Recipe Client 77.18.g3a1687e5) that I got last night from
> :mythmon and I can't reproduce the "almost broken" state there.

Ignore this part, manually installing Shield is not the "similar" to having it installed, since it adds extra STR; I think I need to test/investigate more.
Flags: needinfo?(adrian.florinescu)
After additional investigation, I've found and logged bug 1416325, which is what appears to be a race condition only in the case of new profiles. For new profiles, Shield runs immediately, which IMO happens fast enough that telemetry hasn't set up yet a ping with the profileCreationDate.

In comment 14, I was testing with a recipe that had telemetry.main.environment.profile.creationDate as a filter. Once I reverted to testing with simple filter recipes, this fix looks good to me. 

From testing coverage point of view, I only managed to cover simple combinations of new/old profiles. A variable that I couldn't eliminate was that I needed to manually edit the prefs.js to point to normandy staging, set devmode on, enable logging, but I do not think that these changes present a risk related to the verification of this fix.
Flags: needinfo?(mcooper)
Flags: needinfo?(adrian.florinescu)
Hi Mike, I am told that this fix needs to be included in RC3. Could you please nominate a patch for uplift to beta57? Thanks!
Flags: needinfo?(mcooper)
Assignee

Comment 19

2 years ago
This is a patch of Gijs's commit [0]. I think I exported the patch correctly. I haven't done a patch like this before. This is what actually landed on m-c.

[0]: https://hg.mozilla.org/mozilla-central/rev/80474174ca663daffc37537bc811be5d99bd01a0
Attachment #8927096 - Attachment is obsolete: true
Attachment #8927098 - Attachment is obsolete: true
Flags: needinfo?(mcooper)
Assignee

Comment 20

2 years ago
Comment on attachment 8927610 [details] [diff] [review]
bug-1416003-take2.patch

I intent for this uplift request to mean uplift into 57.0-rc3

Approval Request Comment
[Feature/Bug causing the regression]: bug 1413643
[User impact if declined]: Potential failure of Shield for users targeted for Activity Stream studies
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, already done.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Risk is minimized.
[Why is the change risky/not risky?]: This causes code to throw in less cases.
[String changes made/needed]: None
Attachment #8927610 - Flags: approval-mozilla-beta?
Comment on attachment 8927610 [details] [diff] [review]
bug-1416003-take2.patch

as we said, we need to take it to 57.
Attachment #8927610 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Flags: needinfo?(jcristau)
Blocks: 1413565
See Also: → 1414042
Assignee

Comment 23

2 years ago
Using 57 RC3, I have verified that STR I from the user story no longer produce the problem. I also observed general Shield behavior working without issue.

Comment 24

2 years ago
Commits pushed to master at https://github.com/mozilla/normandy

https://github.com/mozilla/normandy/commit/62f23a634211c516174f2cb0aa6cabc68f8191f7
Bug 1416003 - Handle preference studies on prefs with only user-branch values

https://github.com/mozilla/normandy/commit/02356f80a73e20c18db62a14b0dc3322e4fdf25d
Merge pull request #1129 from mythmon/only-user-branch-prefs

Bug 1416003 - Handle preference studies on prefs with only user-branch values

Updated

a year ago
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.