Closed Bug 1446445 Opened Last year Closed Last year

[Normandy] Recipes using normandy.isFirstRun are not executed during first run

Categories

(Firefox :: Normandy Client, defect, P1, major)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + verified
firefox61 --- verified

People

(Reporter: adrian_sv, Assigned: mythmon)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

[PreRequisites:]
1. Set the app.normandy.dev_mode preference to true to run recipes immediately on startup.
2. Set the app.normandy.logging.level preference to 0 to enable more logging.
3. Set the security.content.signature.root_hash preference to DB:74:CE:58:E4:F9:D0:9E:E0:42:36:BE:6C:C5:C4:F6:6A:E7:74:7D:C0:21:42:7A:03:BC:2F:57:0C:8B:9B:90.
4. Set the preference value for app.normandy.api_url set to https://normandy.stage.mozaws.net/api/v1

[Steps:]
1. Open staging Control center and create any type of recipe with the following filter:
normandy.isFirstRun
2. Open Beta 60 or Nightly 61.

[Actual Result:]
Recipe created at step 1 is fetched but not executed.

[Expected Result:]
Filter should return true and recipe created at step 1 fetched and executed.
Forgot to add at step 2, Beta 60 and Nightly 61 should be opened with a new profile and the pre-requisites should already be defined in prefs.js, in order to be able to reproduce the issue.
[Additional Note:] app.normandy.dev_mode true or false makes no difference: recipe still doesn't execute.
Assignee: nobody → mcooper
Status: NEW → ASSIGNED
Priority: -- → P1
How's the patch moving?
Flags: needinfo?(mcooper)
The patch is pending review from Gijs. I believe it is ready to go.

Gijs, is there anything blocking this on your end I can help with? This is a major bug that we're going to need to uplift to Beta 60 to support bug 1436113. I'd much rather it land soon than have to back out the toolkit component migrations.
Flags: needinfo?(mcooper) → needinfo?(gijskruitbosch+bugs)
(In reply to Mike Cooper [:mythmon] from comment #5)
> The patch is pending review from Gijs. I believe it is ready to go.

There's no review request, so I didn't think it was...
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true

https://reviewboard.mozilla.org/r/229110/#review237578

::: toolkit/components/normandy/lib/ClientEnvironment.jsm:206
(Diff revision 1)
>        const addons = await Addons.getAll();
>        return Utils.keyBy(addons, "id");
>      });
>  
>      XPCOMUtils.defineLazyGetter(environment, "isFirstRun", () => {
> -      return Preferences.get("app.normandy.first_run");
> +      return Services.prefs.get("app.normandy.first_run", true);

Services.prefs.get doesn't exist. You want Services.prefs.getBoolPref, probably?
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true

https://reviewboard.mozilla.org/r/229110/#review237578

> Services.prefs.get doesn't exist. You want Services.prefs.getBoolPref, probably?

(Throughout this set of changes)
> There's no review request, so I didn't think it was...

Odd. Sorry about that, I guess I confused myself with review board. It seems even though I flagged you for review on review board it didn't sync to Bugzilla.
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true

https://reviewboard.mozilla.org/r/229110/#review238136

I'm a little bit concerned none of the below cause test failures (or did the tests not get run?), but r=me with these fixed, I guess...

::: toolkit/components/normandy/lib/ClientEnvironment.jsm:83
(Diff revision 2)
>      XPCOMUtils.defineLazyGetter(environment, "userId", () => {
> -      let id = Preferences.get("app.normandy.user_id", "");
> +      let id = Services.prefs.getCharPref("app.normandy.user_id", "");
>        if (!id) {
>          // generateUUID adds leading and trailing "{" and "}". strip them off.
>          id = generateUUID().toString().slice(1, -1);
> -        Preferences.set("app.normandy.user_id", id);
> +        Services.prefs.set("app.normandy.user_id", id);

setCharPref

::: toolkit/components/normandy/lib/ClientEnvironment.jsm:146
(Diff revision 2)
>        }
>        return null;
>      });
>  
>      XPCOMUtils.defineLazyGetter(environment, "syncSetup", () => {
> -      return Preferences.isSet("services.sync.username");
> +      return Services.prefs.isSet("services.sync.username");

`prefHasUserValue`, I think?
Attachment #8960352 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aeeb277faf1d
app.normandy.first_run should default to true r=Gijs
https://hg.mozilla.org/mozilla-central/rev/aeeb277faf1d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: qe-verify+
QA Contact: adrian.florinescu
Once this fix is verified on Nightly, I plan to request uplift of this to Beta 60.
Managed to reproduce the initial issue on 61.0a1 (2018-03-16) using the steps provided by Adrian. I can confirm that the bug is verified fixed on 61.0a1 (2018-04-02), using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12.6.
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true

Approval Request Comment
[Feature/Bug causing the regression]: bug 1436113
[User impact if declined]: First run studies will never run
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Done already, STR in comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It is a simple change that is easy to verify and has limited side effects
[String changes made/needed]: None
Attachment #8960352 - Flags: approval-mozilla-beta?
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true

normandy regression fix, verified in nightly, approved for 60.0b10
Attachment #8960352 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
The fix is also successfully applied on 60.0b10 (20180404171943) across platforms (Windows 10 x64, Ubuntu 16.04 x86 and macOS 10.13.3).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.