Empty experiment feature object for enabled aboutwelcome fails to render "content.screens is undefined"
Categories
(Firefox :: Messaging System, defect, P2)
Tracking
()
People
(Reporter: Mardak, Assigned: pdahiya)
References
Details
(Whiteboard: [proton-onboarding][priority:2a] [proton-uplift])
Attachments
(1 file, 1 obsolete file)
TypeError: can't access property "undefined", content.screens is undefined
AboutWelcomeDefaults.jsm:594:7
https://searchfox.org/mozilla-central/rev/0e8b28fb355afd2fcc69d34e8ed66bbabf59a59a/browser/components/newtab/aboutwelcome/lib/AboutWelcomeDefaults.jsm#594
I thought we fall back to the local default screens if there's no screen variable set in the experiment, e.g.,
https://experimenter.services.mozilla.com/nimbus/multistage-onboarding-remove-urlbar-focus
{
"id": "msw_control",
"template": "multistage"
}
But maybe something changed?
https://searchfox.org/mozilla-central/rev/0e8b28fb355afd2fcc69d34e8ed66bbabf59a59a/browser/components/newtab/aboutwelcome/AboutWelcomeChild.jsm#221,225,240,250
Updated•3 years ago
|
Reporter | ||
Comment 1•3 years ago
|
||
I think the ability to specify aboutwelcome feature without an explicit screens changed with bug 1694257 removing:
AboutWelcome.defaultProps = DEFAULT_WELCOME_CONTENT;
https://hg.mozilla.org/mozilla-central/rev/96d3adba86e6#l5.31
The specific screens
undefined related exceptions may not be a bug if the intention is that we're supposed to provide an explicit screens definition whenever aboutwelcome is enabled. If so, we can resolve this INVALID?
Assignee | ||
Comment 2•3 years ago
|
||
+1 , with 1694257 fix we moved picking default screens logic only to non-experiment path
Considering we have targeted defaults , we should have flexibility to assign default screens in code for experiment control branch essentially having below for all code paths
let defaults = await AboutWelcomeDefaults.getDefaults(featureConfig);
let screens = featureConfig.screens || defaults.screens;
Will assign and submit the fix, thanks!
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
Marking it P2 in favor of proton onboarding priority flags
Assignee | ||
Comment 4•3 years ago
|
||
Pushed by pdahiya@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5cfa6f26806f Use default screens when no screen variable set in experiment r=Mardak
Comment 6•3 years ago
|
||
bugherder |
Assignee | ||
Comment 7•3 years ago
|
||
Assignee | ||
Comment 8•3 years ago
•
|
||
Comment on attachment 9222858 [details]
Bug 1711700 - Use default screens when no screen variable set in experiment
Beta/Release Uplift Approval Request
- User impact if declined: Patch is going to help conduct nimbus holdback study and closely simulate 88 default onboarding experience in 89
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: See Test plan in https://phabricator.services.mozilla.com/D115549
- List of other uplifts needed: none
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Small patch needed for nimbus experiment when screens content is not defined for experiments. Default flow always has screen content and well tested for onboarding
- String changes made/needed: None
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 9•3 years ago
|
||
NI Pascal to help confirm if attached uplift request can make beta 89 or its too late for that. Thanks
Reporter | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 10•3 years ago
|
||
We have already shipped our last beta and the new onboarding flow was signed off by QA. I think this is a risky uplit as we are now technically entering our Release Candidate week. Romain, do we need that for 89?
Comment 11•3 years ago
|
||
I don't have context on criticality for the Nimbus experiment, Ana can you please clarify if this bug is really critical - QA was done without this patch and we're now very late on the beta cycle.
Comment 12•3 years ago
|
||
I have verified that this issue is no longer reproducible using the latest Firefox Nightly (90.0a1 - Build ID: 20210520095745) installed on Windows 10 x64, Windows 8.1 x64, and Windows 7 x64. Now, I can confirm the following:
- The old Onboarding experience is displayed on the "about:welcome" page after enrolling in the experiment.
- The RTAMO page is displayed after forcing the attribution from the "about:newtab#devtools" page.
Updated•3 years ago
|
Comment 13•3 years ago
|
||
This patch would allow us to run a holdback experiment for MR1 onboarding and the team feels that there is no high risk. If we have the opportunity I'd like to uplift.
Comment 14•3 years ago
|
||
Just discussed with Ana, this is high value/low risk and the team will ensure there is onboarding QA on the RC build.
Can we please uplift this one?
Comment 15•3 years ago
|
||
After further discussions with Pascal this is a risky one that comes too late in the process. It sounds like there are possible work-arounds and delaying the experiment is also an option so let's not uplift it.
Comment 16•3 years ago
|
||
Comment on attachment 9222858 [details]
Bug 1711700 - Use default screens when no screen variable set in experiment
Removing the uplift request as there is a fallback solution to avoid uplifting a change to our onboarding code in RC.
Reporter | ||
Updated•3 years ago
|
Comment 17•3 years ago
|
||
Based on the fact that the status for Firefox 89 was changed to wontfix
and on the verification performed in comment 12, I am removing the "qe-verify+" flag and I am marking this issue as Verified.
Updated•3 years ago
|
Description
•