Closed Bug 1711700 Opened 3 years ago Closed 3 years ago

Empty experiment feature object for enabled aboutwelcome fails to render "content.screens is undefined"

Categories

(Firefox :: Messaging System, defect, P2)

defect

Tracking

()

VERIFIED FIXED
90 Branch
Iteration:
90.3 - May 17 - May 30
Tracking Status
firefox89 --- wontfix
firefox90 --- verified

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

Flags: needinfo?(pdahiya)

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?

See Also: → 1694257

+1 , with 1694257 fix we moved picking default screens logic only to non-experiment path

https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutwelcome/AboutWelcomeChild.jsm#240

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!

Flags: needinfo?(pdahiya)
Assignee: nobody → pdahiya
Iteration: --- → 90.3 - May 17 - May 30
Priority: -- → P1

Marking it P2 in favor of proton onboarding priority flags

Priority: P1 → P2
See Also: → 1709718
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

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
Attachment #9222858 - Flags: approval-mozilla-beta?
Attachment #9222713 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Attachment #9222713 - Flags: approval-mozilla-beta?

NI Pascal to help confirm if attached uplift request can make beta 89 or its too late for that. Thanks

Flags: needinfo?(pascalc)
Whiteboard: [proton-onboarding] → [proton-onboarding][priority:2a]
QA Whiteboard: [qa-triaged]

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?

Flags: needinfo?(pascalc) → needinfo?(rtestard)

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.

Flags: needinfo?(rtestard) → needinfo?(amedinac)

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.
Whiteboard: [proton-onboarding][priority:2a] → [proton-onboarding][priority:2a] [proton-uplift]

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.

Flags: needinfo?(amedinac)

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?

Blocks: 1709718

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 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.

Attachment #9222858 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
See Also: → 1698153
No longer blocks: 1709718

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1708308
Attachment #9222858 - Attachment is obsolete: true
Blocks: 1713269
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: