1503809 - FaviconHelper::ObtainCachedIconFile does main thread IO and causes hangs

Status: NEW

(Core :: Widget: Win32, enhancement, P3)

Description

[Johann Hofmann [:johannh]]

FaviconHelper::ObtainCachedIconFile has been showing up in BHR for causing hangs due to its main thread IO:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/widget/windows/WinUtils.cpp#1528

Now, the file.exists call in there is probably completely unnecessary, but I'm not sure if it's possible to get rid of the GetLastModified time.

Comment 1

[Florian Quèze [:florian]]

This is a specific subset of the main thread I/O done by jumplists (bug 1529276).

Comment 2

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

I think what we can do here is that we can introduce a new method, in FaviconHelper - FaviconHelper::ObtainCachedIconFileAsync. We can then transition the new Jump List backend and this usage here (which already ignores the synchronous return value) to that new method.

The new method can then do the existence testing / stat'ing on the IO thread rather than the main thread, and call into a runnable with either the path to the cached favicon or an nsresult reporting what has gone wrong.

And then we can update nsIJumpListBuilder.obtainAndCacheFavicon to return a Promise instead and use our new backend.

Comment 3

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

Attachment: Bug 1503809 - Add an async version of FaviconHelper::ObtainCachedIconFile. r?rkraesig

Comment 4

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

Attachment: Bug 1503809 - Update WindowsJumpList.sys.mjs to use the async favicon caching utility. r?rkraesig

Depends on D203761

Comment 5

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

Attachment: Bug 1503809 - Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync.

Depends on D209152