1911404 - Prevent defaultBrowserCheck trigger on startup after Profile Refresh

Status: NEW

(Firefox :: Messaging System, defect, P1)

Description

[Shane Hughes [:aminomancer]]

Attachment: An example of a spotlight triggering on the startup after Profile Refresh

Startup messages are currently triggering on the first startup after Profile Refresh, which is problematic because the process is not considered "finished" at this point. It seems that the browser (see SessionMigration.sys.mjs) writes about:welcomeback to the new profile's session store so that, after launching it, about:welcomeback will be restored as the active tab, and the flow should pick up from there. So, showing a spotlight modal on top of this page is a confusing experience.

I think this can be avoided by writing a flag to the session store state on migration (SessionMigration.sys.mjs), and then checking for that flag before we fire the defaultBrowserCheck trigger, to avoid firing it for users who are restoring after a profile refresh or other similar actions. It may also be possible to use some existing state, like browser.sessionstore.resume_session_once or SessionStore.willAutoRestore - but I don't know if those continue to return true even on the startup after restore. I spotted them over here which briefly made me optimistic that we wouldn't need to add any new code, but these may not survive the profile migration or may be cleared before our trigger.

ni?mconley - I figured you'd be able to help us with the question above and review our patch if we end up needing to add some persistent state that we can check on the following startup. Thank you!

Comment 1

[Meg Viar [:mviar]]

FYI there are a couple WIP patches related to profile logic that touch the logic you linked to above. This patch from Bug 1901540 prevents about:welcome from showing after profile refresh on first startup (just waiting to add tests before landing in 131 nightly). There's also a simple WIP child of that patch to prevent the reset profile prompt from showing when returning from the refresh flow.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Thanks for tagging me on this, Shane.

I don't think there's a really great way of determining if the current session is the first session after a refresh. Those items you identified (browser.sessionstore.resume_session_once and SessionStore.willAutoRestore) are also true under other circumstances (for example, when performing a browser restart).

The environment variables (like MOZ_RESET_PROFILE_MIGRATE_SESSION) that we set for kicking off the profile refresh are, I believe, also cleared by the time your trigger might consider firing.

The only variable that I think knows for sure during runtime is this static boolean on nsAppRunner: https://searchfox.org/mozilla-central/rev/1d4c27f9f166ce6e967fb0e8c8d6e0795dbbd12e/toolkit/xre/nsAppRunner.cpp#2918

Hey Mossop, how would you feel about exposing that boolean through nsIXULRuntime, so that the OMC crew can detect this case with something like Services.appinfo.firstSessionAfterRefresh?

Comment 3

[Meg Viar [:mviar]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)

Thanks for tagging me on this, Shane.

I don't think there's a really great way of determining if the current session is the first session after a refresh. Those items you identified (browser.sessionstore.resume_session_once and SessionStore.willAutoRestore) are also true under other circumstances (for example, when performing a browser restart).

The environment variables (like MOZ_RESET_PROFILE_MIGRATE_SESSION) that we set for kicking off the profile refresh are, I believe, also cleared by the time your trigger might consider firing.

The only variable that I think knows for sure during runtime is this static boolean on nsAppRunner: https://searchfox.org/mozilla-central/rev/1d4c27f9f166ce6e967fb0e8c8d6e0795dbbd12e/toolkit/xre/nsAppRunner.cpp#2918

Hey Mossop, how would you feel about exposing that boolean through nsIXULRuntime, so that the OMC crew can detect this case with something like Services.appinfo.firstSessionAfterRefresh?

Having access to the boolean from nsAppRunner would be useful as we're also exploring differentiating users' onboarding experience based on whether they're truly new users or users who just refreshed their profile (see Bug 1901540).

In the current patch in Bug 1898609 I'm defining a method in the migrator code to configure the homepage experience for users post-refresh with conditional logic for those that refresh in app (via about:support, etc) and those that refresh via the stub installer and thus see about:welcome. If exposing the nsAppRunner bool is inadvisable, we could potentially set a pref in that configureHomepage method that captures these details for message targeting purposes.

Comment 4

[Dave Townsend [:mossop]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)

Hey Mossop, how would you feel about exposing that boolean through nsIXULRuntime, so that the OMC crew can detect this case with something like Services.appinfo.firstSessionAfterRefresh?

I'm not super keen on putting it in nsIXULRuntime, maybe nsIAppStartup is a better choice? I think we may also want to do something more generic that blocks all the "firstrun" activities as we'll want to do it in a number of cases, like when creating new profiles. So maybe create an nsIAppStartup.isFirstRun. Whether a profile was created during startup might be a reasonable proxy for this and should solve both the profile refresh and new profiles feature cases, though that would also mean the mach run case would not go through first run stuff, I'm not sure if that is a bad thing though.

Comment 5

[Shane Hughes [:aminomancer]]

(In reply to Dave Townsend [:mossop] from comment #4)

I'm not super keen on putting it in nsIXULRuntime, maybe nsIAppStartup is a better choice?

Thanks for looking into this Dave. Something like nsIAppStartup.firstSessionAfterRefresh should work for our "existing user" messaging purposes (e.g. the screenshot at the top).

I think we may also want to do something more generic that blocks all the "firstrun" activities as we'll want to do it in a number of cases, like when creating new profiles. So maybe create an nsIAppStartup.isFirstRun. Whether a profile was created during startup might be a reasonable proxy for this and should solve both the profile refresh and new profiles feature cases, though that would also mean the mach run case would not go through first run stuff, I'm not sure if that is a bad thing though.

It seems like OMC has 2 related issues which may need independent solutions: 1) We don't want to show on-startup messaging to existing users if the launch is just picking up after a profile refresh, because the messaging will overlap with the post-refresh page and confuse the user; and 2) we want to avoid showing first run onboarding to users who aren't really new users.

The first issue can be resolved by a firstSessionAfterRefresh boolean or something like that. This type of messaging does not necessarily care if it's the first run or not, but ideally, we should usually explicitly exclude first run users from this type of message. So if our first run targeting is set up so that it's false if a profile was created, while that improves the accuracy, it doesn't help us avoid showing these messages to non-first-run users, since we're targeting for a false value anyway.

At first glance it seems like it would be perfect for new user/first run messaging, which has the opposite targeting. But the mach run issue you mentioned seems problematic. We test most of our messages, and especially the new user experience, with mach run, so I'm guessing it would make development more difficult if the !didCreate condition also excluded mach run in addition to profile refresh. I'll defer to Meg on that though.

Comment 6

[Dave Townsend [:mossop]]

(In reply to Shane Hughes [:aminomancer] from comment #5)

(In reply to Dave Townsend [:mossop] from comment #4)

I'm not super keen on putting it in nsIXULRuntime, maybe nsIAppStartup is a better choice?

Thanks for looking into this Dave. Something like nsIAppStartup.firstSessionAfterRefresh should work for our "existing user" messaging purposes (e.g. the screenshot at the top).

I think we may also want to do something more generic that blocks all the "firstrun" activities as we'll want to do it in a number of cases, like when creating new profiles. So maybe create an nsIAppStartup.isFirstRun. Whether a profile was created during startup might be a reasonable proxy for this and should solve both the profile refresh and new profiles feature cases, though that would also mean the mach run case would not go through first run stuff, I'm not sure if that is a bad thing though.

It seems like OMC has 2 related issues which may need independent solutions: 1) We don't want to show on-startup messaging to existing users if the launch is just picking up after a profile refresh, because the messaging will overlap with the post-refresh page and confuse the user; and 2) we want to avoid showing first run onboarding to users who aren't really new users.

Ah ok I think I misread this as an issue about firstRun messaging but the first case is actually startup messaging. In which case you probably want to use the existing nsIAppStartup.wasRestarted flag. That may already be set in the session after profile refresh but if not we can fix that.

The first issue can be resolved by a firstSessionAfterRefresh boolean or something like that. This type of messaging does not necessarily care if it's the first run or not, but ideally, we should usually explicitly exclude first run users from this type of message. So if our first run targeting is set up so that it's false if a profile was created, while that improves the accuracy, it doesn't help us avoid showing these messages to non-first-run users, since we're targeting for a false value anyway.

At first glance it seems like it would be perfect for new user/first run messaging, which has the opposite targeting. But the mach run issue you mentioned seems problematic. We test most of our messages, and especially the new user experience, with mach run, so I'm guessing it would make development more difficult if the !didCreate condition also excluded mach run in addition to profile refresh. I'll defer to Meg on that though.

I think the mach run issue is trivially solvable by being able to override the default with some environment variable. Either mach can set that by default or provide a command line argument to set it.

Comment 7

[Shane Hughes [:aminomancer]]

(In reply to Dave Townsend [:mossop] from comment #6)

Ah ok I think I misread this as an issue about firstRun messaging but the first case is actually startup messaging. In which case you probably want to use the existing nsIAppStartup.wasRestarted flag. That may already be set in the session after profile refresh but if not we can fix that.

I don't think that will work either, since we do want to message users on restart, relatively often. For example, after updating Firefox. The only case where we don't want the user to see these messages is right after a profile refresh, because there will already be another special page shown in that case (the "Success!" page in the screenshot). That's a type of restart, but most restarts are unrelated to profile refresh and don't have any similar problems. And I imagine for a lot of users, non-restart startups are relatively rare, so conditioning our messaging on !wasRestarted would probably have a significant impact on our total reach.

Comment 8

[Meg Viar [:mviar]]

@aminomancer do you recall your steps to replicate this bug?

Comment 9

[Shane Hughes [:aminomancer]]

It was actually David Rubino who encountered the bug in the screenshot, so I'm not sure if there were any special steps beyond just following the profile refresh sequence. David, can you remember what you did before seeing this?

Comment 10

[[e-mail]]

From the Slack thread: https://mozilla.slack.com/archives/C90HG2UQH/p1722616028062969

Hello... not sure if it's a bug, but it feels like a bug. I've done it twice now.

1. Uninstalled and reinstalled Firefox release channel
2. Said yes to an offer to "refresh my profile" (what does that actually do?)
3. Upon refreshing, I immediately see some experimenter based spotlight modal

This second time, when I screenshotted it, it's the waving fox. But the first time it was a spotlight from our device migration campaign earlier this year, which is what made me do a double-take and try again.

Comment 11

[Meg Viar [:mviar]]

Thanks, David. There are some caveats to seeing the profile refresh checkbox in the first place.