1666850 - Add additional checks when using ExperimentStore during early startup

Status: VERIFIED FIXED

(Firefox :: Nimbus Desktop Client, defect, P1)

Description

[Andrei Oprea [:andreio]]

Using the ExperimentAPI in the AboutWelcomeChild uncovered some issues for the early startup scenario of the store. We try to access too early and (I suspect) the lazy getter "saves" this bad store state (undefined) for future access as well.

Comment 1

[Ed Lee :Mardak]

The problematic calling code is here and commenting out the early access from https://searchfox.org/mozilla-central/rev/3d53187b90605ccef321c9cadbba762ad06a6381/browser/actors/AboutNewTabChild.jsm#42

seems to avoid the undefined store issue that persists to the login usage introduced in https://phabricator.services.mozilla.com/D90988

Not sure if this AboutNewTabChild code introduced in bug 1659152 should be changed or it's somewhat expected that the API can fail thus try/catch used elsewhere, so the fix here is to make sure ExperimentAPI doesn't persist failure to future access.

Comment 2

[Andrei Oprea [:andreio]]

Attachment: Bug 1666850 - Add additional checks when using ExperimentStore during early startup

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e85d76a5d84
Add additional checks when using ExperimentStore during early startup r=pdahiya

Comment 4

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9e85d76a5d84

Comment 5

[Ed Lee :Mardak]

[Tracking Requested - why for this release]: Experiment behavior targeting 82 might not load correctly on first run

QA steps:

1. create a new profile
2. open firefox (nightly opens about:home and beta opens about:welcome)
3. open browser console

expect no error message looking like this for about:home

can't convert null to object SharedDataMap.jsm:122
    _notifyUpdate resource://messaging-system/lib/SharedDataMap.jsm:122
    _syncFromParent resource://messaging-system/lib/SharedDataMap.jsm:137
    SharedDataMap resource://messaging-system/lib/SharedDataMap.jsm:52
    ExperimentStore resource://messaging-system/experiments/ExperimentStore.jsm:22
    <anonymous> resource://messaging-system/experiments/ExperimentAPI.jsm:164
    get resource://gre/modules/XPCOMUtils.jsm:58
    isFeatureEnabled resource://messaging-system/experiments/ExperimentAPI.jsm:64
    handleEvent resource:///actors/AboutNewTabChild.jsm:42

or about:welcome

can't convert null to object SharedDataMap.jsm:122
    _notifyUpdate resource://messaging-system/lib/SharedDataMap.jsm:122
    _syncFromParent resource://messaging-system/lib/SharedDataMap.jsm:137
    SharedDataMap resource://messaging-system/lib/SharedDataMap.jsm:52
    ExperimentStore resource://messaging-system/experiments/ExperimentStore.jsm:21
    <anonymous> resource://messaging-system/experiments/ExperimentAPI.jsm:143
    get resource://gre/modules/XPCOMUtils.jsm:58
    getExperiment resource://messaging-system/experiments/ExperimentAPI.jsm:46
    AWGetStartupData resource:///actors/AboutWelcomeChild.jsm:234

Comment 6

[Ciprian Muresan [:cmuresan], Ecosystem QA]

I have successfully reproduced the issue on the latest Firefox Beta 82.0b3 (BuildID 20200924145114) and have verified that it is no longer reproducible on both the about:welcome and about:home pages on the latest Nightly 83.0a1 (BuildID 20200924215348) using Windows 10 x64, macOS 10.15, and Linux MX 4.19.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:andreio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

[Ed Lee :Mardak]

Comment on attachment 9177448 [details]
Bug 1666850 - Add additional checks when using ExperimentStore during early startup

Beta/Release Uplift Approval Request

• User impact if declined: Experiments for new profiles might not run correctly
• 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: Comment 5
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Code adds some checks to use default empty value to avoid accessing null
• String changes made/needed: None

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9177448 [details]
Bug 1666850 - Add additional checks when using ExperimentStore during early startup

approved for 82.0b6

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ff6ee029cc0

Comment 11

[Ciprian Muresan [:cmuresan], Ecosystem QA]

I have verified that the issue is no longer reproducible on the about:welcome or about:home pages on the latest Firefox Beta 82.0b6 (BuildID 20201001171107) using Windows 10, macOS 10.15, and Linux MX 4.19.