1620576 - UrlbarProviderSearchTips.jsm triggers main thread I/O to updates.xml to fetch the updateCount

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Florian Quèze [:florian]]

The lastBrowserUpdateDate function in UrlbarProviderSearchTips.jsm is async so it looks like it was meant to not block the main thread, but calling updateManager.updateCount at https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/browser/components/urlbar/UrlbarProviderSearchTips.jsm#464 triggers main thread I/O to updates.xml. See this profile: https://perfht.ml/2vIUtI3

This code is from bug 1606909 and just got enabled in bug 1617029.

Comment 1

[James Teow [:jteow]]

It'd be good to first investigate why we did await lastBrowserUpdateDate() in the first place, and then decide if we can still keep showing a search tip if the _updatesCache is null / unavailable.

Comment 2

[James Teow [:jteow]]

Reading the discussion in Bug 1595328, the purpose of gathering when a user last updated was to avoid inundating user with help messages when they restarted their browser, per Verdi's comment:

The intent is to not annoy people or add to all of the other things we're asking people to look at, do, or consider.

It is either 24 hours since the browser last applied the update (the browser might not have been restarted) or when the user profile was first used (if not available, then its the created date).

We allow the enabling of the search tips provider if at least one of the search tips hasn't reached its maximum threshold of views. And checking if search tips should appear on a page occurs via onLocationChange, which could be when a user first opens the browser and views a New Tab.

I wonder if one compromise we can make is to query the update service if the updates are cached (e.g. a new method called hasUpdatesCached, and if it doesn't, we assume we shouldn't show a search tip, but in a callback passed to idleDispatchToMainThread we'll call getUpdateCount, and then if search tips provider is queried again (via another onLocationEvent), the cache might be updated.

I think the main drawback of that approach is a user will likely not see the Onboarding Search Tip without having triggered onLocationChange, so they probably won't see it immediately if they re-open the browser.

Thoughts :adw? This feels more like it needs UX input and should be taken out of the dragon slayer month.

Comment 3

[Daisuke Akatsuka (:daisuke)]

I also tried to think about this.
If calling UpdatePing.handleUpdateSuccess() in BrowserContentHandler means "this session was started with a new update", I wonder if we can add something flag to UpdateManager etc here? In UrlbarProviderSearchTips, if the flag is true, refer to Services.startup.getStartupInfo().start, if this does not pass 24 hours, will not show the tips. Then, it seems that we can avoid accessing update.xml and file access via TelemetryArchive.promiseArchivedPingList(). There may be issues, so I'll try to make a prototype once.

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1620576: Refer to the start time of the session with the update applied to suppress browser tips.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9a2ef43e030
Refer to the start time of the session with the update applied to suppress browser tips. r=adw,jteow,Gijs

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a9a2ef43e030