Open Bug 1620576 Opened 5 years ago Updated 1 year ago

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

Categories

(Firefox :: Address Bar, defect, P3)

defect
Points:
3

Tracking

()

ASSIGNED
Tracking Status
firefox74 --- disabled
firefox75 --- fix-optional
firefox76 --- fix-optional

People

(Reporter: florian, Assigned: daisuke, NeedInfo)

References

(Regression)

Details

(Keywords: perf, regression, Whiteboard: [fxperf:p3][snt-scrubbed][search-performance])

Attachments

(1 file)

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.

Blocks: 1606904
No longer regressed by: 1617029
Has Regression Range: --- → yes
Points: --- → 3
Priority: -- → P3
Whiteboard: [fxperf] → [fxperf:p3]
Severity: normal → S3

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.

Whiteboard: [fxperf:p3] → [fxperf:p3][snt-scrubbed][search-performance]

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.

Flags: needinfo?(adw)

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.

Assignee: nobody → daisuke
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: