Funnelcake 138 / 139 Pinning to Win10 taskbar A/B testing

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: RT, Assigned: nthomas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Context: We're investigating the impact that placing a Firefox shortcut on the Windows 10 taskbar would have on engagement and retention. The hypothesis being that users find it more convenient to open Firefox from the taskbar in a context where pinning shortcuts to the taskbar is not something most users know how to do.

This bug is for the creation of the test and control funnelcake builds to support the test.

Start date: TBC
Channel: Release 63
Feature document: https://docs.google.com/document/d/1zhj1GWtZoUTCsAlJ2i0hdgxkAFdpK4lT6QNygRtIgCI/edit
Addressable population: New Windows 10 users on 64 bit OS with en-US locale (test and control cohorts)
Blocks: 1493595
We don't have the ability to do custom installers for funnelcakes...
We sketched out a plan to make this possible. The control would come from the regular release automation. The taskbar-pinning variation would need some custom work - mhowell would write a installer patch and we'd push that to a branch in the mozilla-release repo, yielding a customized en-US installer. RelEng would then manually repack that to include the distribution directory, sign, stage etc. We limited to en-US to avoid repeating over ~100 locales, each with it's own strings in the installer. Hopefully we could gather cohorts fast enough to not need to rebuild for point releases.

Having taken a look around the installer code I'm wondering if there's another way, ni? mhowell for his thoughts. I see the full installer reads distribution\distribution.ini and distribution\setup.ini (if they exist) to look for parameters. Could we add a new parameter to control pinning ? Then land that support in 63 on beta/release so all installers support the pinning functionality, but only enable that in variation where we want it. The scope of the risk expands a little but there's still a few 63 betas left. The patch could be very similar to the first case, I think, just conditional on reading the parameter. The upside of this approach is it makes generating the funnelcake builds very simple, we can use the existing automation for releases. Could do select all locales this too.
Flags: needinfo?(mhowell)
Sure, I think we could safely use setup.ini that way. setup.ini doesn't do a whole lot, so I feel better about messing with it than with distribution.ini. And I can easily gate off the block I'm adding based on that parameter to keep the risk down. So yeah, if that's better for you guys, let's go for it.
Flags: needinfo?(mhowell)
Do we need to revisit that plan now that beta14 has been built, and the merge from beta to release will happen next Monday ?
I hope you're not asking me, because I would put that question to you. ;)

I was waiting for confirmation that we were going with your .ini file suggestion and about what to name the setting in there. I have a patch that's ready except for that mechanism.
Oh sorry, I didn't realize you were waiting on me for an ack for using setup.ini. That sounds good, and I don't have any preference on the naming of the setting.

Pascal, would you accept an installer change in the RC build of 63 ?  I realize that's very optimistic a this point. If not we can still use the patch on a branch of mozilla-release.
Flags: needinfo?(pascalc)
Matt, can we get your patch? That would give relman more data to take a decision especially since we build RC today, thanks.
Flags: needinfo?(pascalc) → needinfo?(mhowell)
Posted patch Patch for funnelcake testing (obsolete) — Splinter Review
Here you go. This patch checks for the presence of an option in setup.ini called Win10TaskbarPin under the a section called Testing; the value can be "1" or "true" or whatever, doesn't matter.
Assignee: nobody → mhowell
Flags: needinfo?(mhowell)
Nick, I think this is a bit risky and would require QA to be involved, so I'd prefer that you use the patch on a branch of mozilla-release since you have this option.
Flags: needinfo?(nthomas)
OK, I'll figure out a detailed plan for the branch option.
Flags: needinfo?(nthomas)
(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #10)
> OK, I'll figure out a detailed plan for the branch option.

Hi Nick, what's the latest on this? Does this require a PI request to get QA involved?
The plan is to adjust our partner repack automation so that it can substitute the setup.exe inside the full installer with a custom version, which we'll create using Matt's patch. I need to do some development work and testing to get that ready, which I'm going to do this week. No firm ETA yet.

While we often don't involve QE with funnelcake tests I think it would be useful this time to make sure the pinning works as expected.
Assignee: mhowell → nthomas
Sounds good, I just raised the PI request.
Thanks for letting us know when this could potentially ship once you have visibility.
Summary: Funnelcake Pinning to Win10 taskbar A/B testing → Funnelcake 138 / 139 Pinning to Win10 taskbar A/B testing
138 for the control, vanilla Firefox win64 en-US with the minimal funnelcake tagging
139 for the test, modified setup.exe win64 en-US that does taskbar pinning & funnelcake tag

I'll need to generate the modified setup.exe (with release signing) to write the funnelcake configs. ETA a day or so for builds to test.
Comment on attachment 9017191 [details] [diff] [review]
Patch for funnelcake testing

Traditionally funnelcake patches haven't needed review, but this one is a bit involved, so I've been asked to request it.
Attachment #9017191 - Flags: review?(agashlin)
(In reply to Matt Howell (he/him) [:mhowell] from comment #16)
> Traditionally funnelcake patches haven't needed review, but this one is a
> bit involved, so I've been asked to request it.

Since we're going to need a fairly large number of users it seemed like a good precuation to take.

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #14)
> Created attachment 9023194 [details] [review]
> GitHub Pull Request - support for replacing setup.exe

Now landed.
Not quite complete yet.
Posted patch InvokeShellVerb patch (obsolete) — Splinter Review
These are the changes I made to InvokeShellVerb.cpp to get the DLL included in the main patch. I didn't think to include this before because we don't actually have that source file in our tree and I didn't expect that particular patch to be getting reviewed.
Comment on attachment 9017191 [details] [diff] [review]
Patch for funnelcake testing

Review of attachment 9017191 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/windows/nsis/shared.nsh
@@ +1198,5 @@
> +              ClearErrors
> +              ReadINIStr $R9 "$EXEDIR\core\distribution\setup.ini" \
> +                "Testing" "Win10TaskbarPin"
> +              ${IfNot} ${Errors}
> +                ReadRegStr $R8 HKCU "Software\Classes\*\shell\taskbarpin" "ExplorerCommandHandler"

I'm not quite comfortable with how the temporary registry value is handled here. If the key existed but didn't have ExplorerCommandHandler (or the value was ""), the DeleteRegKey below will delete the whole key. There's also some weirdness if the value happened to be a DWORD.

If it's unlikely that there will be something to backup anyway, maybe we should just bail out if the key exists? Or could we use a random name instead of "taskbarpin"?
Comment on attachment 9023714 [details] [diff] [review]
InvokeShellVerb patch

Review of attachment 9023714 [details] [diff] [review]:
-----------------------------------------------------------------

::: InvokeShellVerb.cpp
@@ +77,4 @@
>  #endif
> +
> +      // If our szResID parameter isn't a number, then it's just
> +      // the name of a verb that we should invoke directly.

Now that the number-ness is optional, it might make sense to use the strtol family to check that the whole string is parsed as an integer. Or maybe it would be better to use something to mark szResId an unambiguously not an integer, like a leading /?

@@ +88,5 @@
> +                         sizeof(wszVerbName)/sizeof(wszVerbName[0])); 
> +        FreeLibrary(hInst);
> +        if (!rv)
> +        {
> +          pushstring(OUT_ERR_INVALID_RESID);

A general question: Do the calling scripts account for the value pushed here and elsewhere? I don't see anything popping a result.
Comment on attachment 9017191 [details] [diff] [review]
Patch for funnelcake testing

Review of attachment 9017191 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/windows/nsis/shared.nsh
@@ +1198,5 @@
> +              ClearErrors
> +              ReadINIStr $R9 "$EXEDIR\core\distribution\setup.ini" \
> +                "Testing" "Win10TaskbarPin"
> +              ${IfNot} ${Errors}
> +                ReadRegStr $R8 HKCU "Software\Classes\*\shell\taskbarpin" "ExplorerCommandHandler"

I was never fully comfortable with it either. I really like your random name idea; I'll try it with something that includes the path hash, so we can be very sure it's going to be unique.
(In reply to Adam Gashlin [:agashlin] from comment #21)
> Now that the number-ness is optional, it might make sense to use the strtol
> family to check that the whole string is parsed as an integer. Or maybe it
> would be better to use something to mark szResId an unambiguously not an
> integer, like a leading /?

strtol is a good idea, that should take care of it without the need for anything like a leading /.

> A general question: Do the calling scripts account for the value pushed here
> and elsewhere? I don't see anything popping a result.

Ha ha, nope, we never pop that value anywhere. I kind of feel like that's a separate bug, but I could fix it here if you want.
> strtol is a good idea, that should take care of it without the need for anything like a leading /.
A benefit of the / is it would allow strings that are numbers.
> Ha ha, nope, we never pop that value anywhere. I kind of feel like that's a separate bug, but I could fix it here if you want.

Probably not worth messing with it in this bug.
(In reply to Adam Gashlin [:agashlin] from comment #24)
> > strtol is a good idea, that should take care of it without the need for anything like a leading /.
> A benefit of the / is it would allow strings that are numbers.

I don't think we need to worry about that.
Attachment #9017191 - Attachment is obsolete: true
Attachment #9017191 - Flags: review?(agashlin)
Attachment #9023806 - Flags: review?(agashlin)
Comment on attachment 9023806 [details] [diff] [review]
Patch for funnelcake testing, revision 1

Thanks!
Attachment #9023806 - Flags: review?(agashlin) → review+
Comment on attachment 9023806 [details] [diff] [review]
Patch for funnelcake testing, revision 1

Thanks for reviewing. Pushed as https://hg.mozilla.org/releases/mozilla-release/rev/b054ab8cb4da2d2970135b0b8066a0c0a51ed79f on FUNNELCAKE_138_139_BRANCH.
Depends on: 1506648
(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #30)
> Thanks for reviewing. Pushed as
> https://hg.mozilla.org/releases/mozilla-release/rev/
> b054ab8cb4da2d2970135b0b8066a0c0a51ed79f on FUNNELCAKE_138_139_BRANCH.

The Ns build produced a release-signed setup.exe, which is at
https://queue.taskcluster.net/v1/task/BiLFx2vFTCW4vo89-PAivw/runs/1/artifacts/public/build/setup.exe
I've added bouncer products firefox-stub-f138 and firefox-stub-f139 in the usual way. We're ready to ship from a Release Engineering point of view.
(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #33)
> I've added bouncer products firefox-stub-f138 and firefox-stub-f139 in the
> usual way. We're ready to ship from a Release Engineering point of view.

Sorry I'm not too familiar with the bouncer, does your comment above cover the needs of bug 1493595 or do we still need someone to help configure the roll-out from moz.org?
Flags: needinfo?(nthomas)
Bouncer is what moz.org will point to in the download button, specifically
  https://download.mozilla.org/?product=firefox-stub-f138&os=win64&lang=en-US
  https://download.mozilla.org/?product=firefox-stub-f139&os=win64&lang=en-US
Flags: needinfo?(nthomas)
.... so you'll still need someone from bedrock or similar to configure traffic cop in bug 1493595. 

I've updated bouncer to point to 63.0.3 (build1), which was close to shipping at the time of writing this.
We have finished testing the Funnelcake builds (Control and Test).

QA’s recommendation: GREEN - SHIP IT

Reasoning: 
- We haven’t found any issues during testing.

Testing Summary:
- Test Suite: https://testrail.stage.mozaws.net/index.php?/plans/view/13720

Tested Platforms:
- Windows 10 x64 v1803;
- Windows 10 x64 v1709;
- Windows 10 x64 v1703;

Tested funnelcake versions:
- Funnelcake 138;
- Funnelcake 139;
I've looked for changes in browser/installer between 63.0 and 64.0, since we might need to build a new setup.exe if there is. There's bug 675428, but that will be backed out in bug 1512496 before 64 ships. Bug 1490575, 1477669, 1489812 don't look to be in the installer code, and bug 1488857 is the stub installer. Also checked toolkit/mozapps/installer/ and there's nothing in the windows subdirectory.

--> No need to create setup.exe again, but will need to update bouncer once we settle on a final build for 64.0. Setting ni as a reminder.
Flags: needinfo?(nthomas)
Bouncer links are now updated to 64.0 build3.
Flags: needinfo?(nthomas)
Hi Nick, we ran into issues when exposing the Funnelcakes through traffic cop (bug 1493595) and it seems we cannot reliably target 64 bit OS users to serve them the 64 bit Funnelcake.
We met with the moz.org team and the proposal is to serve 32 bit funnelcakes to Win10 users regardless of their CPU architecture to avoid providing a 64 bit build to 32 bit OS users.
This will although require building new 32 bit funnelcakes and run them through QA again:
- 138 for the control, vanilla Firefox win32 en-US with the minimal funnelcake tagging
- 139 for the test, modified setup.exe win32 en-US that does taskbar pinning & funnelcake tag 

Can you please help us understand if re-building these funnelcakes is a large piece of work and what the timeline could be so we align QA and bouncer configuration activities accordingly?
Flags: needinfo?(nthomas)
Hi Romain, I could create 32 bit replacement builds within a day. 

Given the compromise in giving many users on a 64 bit OS a 32 bit Firefox, perhaps it's worth investigating custom stub installers ? It might not take long to create funnelcake stubs which install 64-funnelcake or 32-funnelcake (one each for 138 and 139), or even a mixed-stub which does 64-funnelcake or 32-vanilla. The latter would probably be quickest, so long as this doesn't muddy the metrics waters ?
Flags: needinfo?(nthomas)
(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #41)
> Hi Romain, I could create 32 bit replacement builds within a day. 
> 
> Given the compromise in giving many users on a 64 bit OS a 32 bit Firefox,
> perhaps it's worth investigating custom stub installers ? It might not take
> long to create funnelcake stubs which install 64-funnelcake or 32-funnelcake
> (one each for 138 and 139), or even a mixed-stub which does 64-funnelcake or
> 32-vanilla. The latter would probably be quickest, so long as this doesn't
> muddy the metrics waters ?

Agreed we need a solution long term and this is being looked at on bug 1498689 (I need to follow that up with Matt). This will require extra QA on the stub side and Matt's time to get it done which feels like it won't fit our timelines on this project though.
For the purpose of this experiment delivering 32 bit to all users regardless of their OS arch won't make the test invalid from an analysis standpoint and I assume that eligible users would get migrated to 64 bit at their next update?
For simplicity sake I feel we should complete this experiment using a 32 bit funnelcake and we'll be prioritizing bug 1498689 on the installer side since we absolutely need a long term solution for funnelcakes and partner builds (partner builds feel like the real driver there since funnelcakes get used very occasionally now AFAIK).
Are you OK with this approach?
Flags: needinfo?(nthomas)

(In reply to Romain Testard [:RT] from comment #42)

Agreed we need a solution long term and this is being looked at on bug
1498689 (I need to follow that up with Matt). This will require extra QA on
the stub side and Matt's time to get it done which feels like it won't fit
our timelines on this project though.
For the purpose of this experiment delivering 32 bit to all users regardless
of their OS arch won't make the test invalid from an analysis standpoint and
I assume that eligible users would get migrated to 64 bit at their next
update?

We actually don't have any 32 to 64 bit migration paths in place right now, at least for anyone using newer releases than Firefox 56.0. It's very likely we could add a new migration specific to these funnelcake users, so long as we include the time cost of that when considering alternatives. The QA load was quite considerable prior to the last migration, so that's why I'm looking at ways to avoid it and give users 64 bit if we can.

The custom stub installers I'm suggesting would be much less work than implementing bug 1498689. I'd patch the stub code to change where it downloads the full installer from. Roughly the flow would be

  • www.mozilla.org download buttons point to (bouncer products) firefox-stub, firefox-stub-f138, firefox-stub-f139 (via traffic cop, all genuine stub installers, no changes required to bedrock etc)
  • each stub detects the OS when it runs and downloads the full installer via bouncer
    • firefox-latest-stub: product=firefox-latest, and os=win64 or os=win (32 bit) (no changes)
    • firefox-stub-f138: product=firefox-latest-f138 & os=win64; or product=firefox-latest & os=win
    • firefox-stub-f139: product=firefox-latest-f139 & os=win64; or product=firefox-latest & os=win

ie, funnelcake stubs install the (existing) funnelcake 64 bit build where appropriate, otherwise the 'vanilla' 32 bit build. We don't need a 32 bit funnelcake this way. The implementation is fairly simple - pushing stub patches into a relbranch to generate signed stub installers (similar to what we did earlier for setup.exe), move the files around, and set up bouncer. This is all en-US only, no change there. We've done something similar to this in the past but QA would absolutely be a good idea.

There is a bit more work in custom stub installers than switching to 32 bit funnelcake, but not that much: 2 days vs 1 if I'm conservative, probably quicker. It doesn't make any difference which we choose if 64.0.2 or 65.0 comes along before the cohort is big enough. Custom stubs with vanilla 32 bit is my recommendation, what do you think ?

agibson, do the bedrock and bouncer details look correct to you ?

Flags: needinfo?(nthomas) → needinfo?(agibson)

Matt, this should implement what I describe in comment #43 for 138 - turn on the funnelcake machinery in the stub, but download the vanilla 32 bit build (which avoids some extra work to create both 32 and 64 full installers with custom setup.exe). I've left the ping alone so we get separate metrics from the custom stubs. Similarly URLManualDownloadAppend because the query arg it adds doesn't seem do anything on www.mozilla.org.

There'd be an almost identical version for funnelcake 139.

Attachment #9034947 - Flags: feedback?(mhowell)

If I'm following along correctly here and the funnelcake builds would effectively be custom stub installers which would correctly choose between 32/64bit depending on the OS architecture, then yes this could be a good short term solution. As long as the product details we pass along to bouncer don't need to change (they look the same to me here), then this should be an easy thing for bedrock to accomodate.

Flags: needinfo?(agibson)
Comment on attachment 9034947 [details] [diff] [review]
[gecko] custom stub patch for 138

Review of attachment 9034947 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, with this patch we would get the behavior under test and the stub ping would report a funnelcake number, but we would only ever install a funnelcake build for 64-bit and not even try to find one for 32-bit.

The one thought I have is, the 32-bit build won't know that it was installed as part of a funnelcake, so we would need some other way to identify it to get the metrics from it that we're after; having attribution parameters set on the stub with this patch should be a good way to take care of that, I think.
Attachment #9034947 - Flags: feedback?(mhowell) → feedback+

If 32-bit is not part of the test, is it important to track metrics ? We'll certainly see a drop in pings from the vanilla stub for 32-bit if 18% of downloads are given funnelcake stubs, but that'll be there for 64 too.

Comment on attachment 9034947 [details] [diff] [review]
[gecko] custom stub patch for 138

Pushed this to a new relbranch (FUNNELCAKE_138_139_STUB_BRANCH) at mozilla-release@62c1a96d83a0.
It should yield a signed stub installer here.

Bumped FunnelcakeVersion to 139 at mozilla-release@bd1c59ce5919. It should yield a signed stub installer here. It's going to need a little more help to produce the stub (due to the way I cancelled unwanted tasks). Will need to rerun these taskIds in sequence: cJQCxEWwRyCZqRyc4oa_yg, Vurr0i7GRD6l1HF5llJ8iA, and F1YuFvswTcKfj4VW-hEIog.

I was testing the custom stub installers and found they worked for 64-bit Windows, but the trailing $ on

  +    InetBgDL::Get "${URLStubDownload32}$" \

caused 32-bit to fail. This is an updated patch, carrying over the f+.

138 pushed at mozilla-release@107c640a2548. TaskIds dO6kiRfMSui-uUi3mDCixQ, ciIxSF5AS-G6Ji7N7w87FA, PH-_ISI-TlWEqyUy2TYU_g, byxtvHRhRgy2KJR3rxw_JA to run.

139 pushed at mozilla-release@97d181973f94. TaskIds PamycqJXTlmK40t-2w0ZdQ, Zqwpyr-HS363aiMKCydWKQ, Qv8eMWBrRbiQiUtzmWSVvA, ZBHr51LXQzauoFSMYKmg-g to run.

Attachment #9034947 - Attachment is obsolete: true
Attachment #9035534 - Flags: feedback+

I've verified the stub for 138 installs vanilla win32 correctly now. I'll finish up my remaining tasks first thing tomorrow.

The stubs have been moved to

https://archive.mozilla.org/pub/firefox/candidates/64.0.2-candidates/build1/partner-repacks/funnelcake/funnelcake138/v1/win64/en-US/Firefox%20Installer.exe
https://archive.mozilla.org/pub/firefox/candidates/64.0.2-candidates/build1/partner-repacks/funnelcake/funnelcake139/v1/win64/en-US/Firefox%20Installer.exe

and bouncer products firefox-stub-f139 and -f139 to point to those. There are also firefox-latest-f138 and -f139 products to serve the full installer.

I've tested win32 and win64 installs work in VMs, starting from already downloaded stub installers, so I think everything is OK from my point of view. The next step is probably to make sure end-to-end install works from staging www.m.o or demo site, with traffic cop enabled.

See Also: → 1519275

Nick and Cosmin, can you please confirm if this requires another round of Softvision QA?

Flags: needinfo?(nthomas)
Flags: needinfo?(cosmin.muntean)

Considering the fact that we have new builds, I think it would be the best to perform another round of testing to make sure that everything works as expected. Since we already have the test cases created, it will take about 2 days to test this.
If everyone thinks that we should perform another round of testing, please send a new PI request for it.

Flags: needinfo?(cosmin.muntean)

Nick and Matt, you guys can probably best advise if another round of QA is needed. Let me know and I can raise the PI request if needed.

Flags: needinfo?(mhowell)

I don't think we would get much out of running another full round of QA for the changes that have been made; nothing in the code for the feature has changed, it's really just about how the builds are packaged/delivered.

Flags: needinfo?(mhowell)

question regarding the custom stubinstaller: will these be sending install pings like the regular stubinstaller does? if so, will they be sent to a different location?

They will be sending normal stub pings to the normal location. Those pings will contain the funnelcake ID so they can be identified/filtered.

fantastic!

I agree with Matt - the full installers are known good at this point, we should concentrate on the delivery to make sure we've resolved the issues there.

Flags: needinfo?(nthomas)

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #59)

I agree with Matt - the full installers are known good at this point, we should concentrate on the delivery to make sure we've resolved the issues there.

Thanks. I won't raise a PI request.

I've done some testing (see bug 1493595 comment #46) and the builds and distribution is working properly.

Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED

https://github.com/mozilla-partners/funnelcake/pull/76 to cleanup the repack config, now that bug 1493595 comment #64 says we have enough people.

You need to log in before you can comment on or make changes to this bug.