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

Status: RESOLVED FIXED

(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. r?Mossop

Depends on D209152

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78b1a5e7ec1e
Add an async version of FaviconHelper::ObtainCachedIconFile. r=rkraesig,win-reviewers
https://hg.mozilla.org/integration/autoland/rev/ce0ff7d06ed4
Update WindowsJumpList.sys.mjs to use the async favicon caching utility. r=rkraesig
https://hg.mozilla.org/integration/autoland/rev/4d04db1983e3
Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync. r=mossop
https://hg.mozilla.org/integration/autoland/rev/22bf63387696
apply code formatting via Lando

Comment 7

[Norisz Fay [:noriszfay]]

Backed out for causing xpcshell failures on test_JumpListBuilder_obtainAndCacheFaviconAsync.js

Backout link

Push with failures

Failure log

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/947f25cfac26
Add an async version of FaviconHelper::ObtainCachedIconFile. r=rkraesig,win-reviewers
https://hg.mozilla.org/integration/autoland/rev/38b6ce46cca2
Update WindowsJumpList.sys.mjs to use the async favicon caching utility. r=rkraesig
https://hg.mozilla.org/integration/autoland/rev/e9c1549806bf
Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync. r=mossop
https://hg.mozilla.org/integration/autoland/rev/c1a66ab56f9d
apply code formatting via Lando

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/947f25cfac26
https://hg.mozilla.org/mozilla-central/rev/38b6ce46cca2
https://hg.mozilla.org/mozilla-central/rev/e9c1549806bf
https://hg.mozilla.org/mozilla-central/rev/c1a66ab56f9d