1722304 - Setting default downloads directory to unavailable windows share makes opening settings cause several seconds of parent process mainthread jank (net_GetURLSpecFromFile causes slow mainthread IO)

Status: NEW

(Core :: Networking: File, defect, P3)

Description

[:Gijs (he/him)]

STR:

1. have a network with 2 windows machines
2. share a directory from machine 1
3. set this directory as the default downloads directory on machine 2
4. disconnect / power down machine 1
5. open the Firefox preferences/settings page on machine 2

ER:
it works fine

AR:
mega jank (I see about 5 seconds on my normally super-fast threadripper)

Stack:

NtQueryAttributesFile
GetFileAttributesW
nsLocalFile::IsDirectory(bool*)
net_GetURLSpecFromFile(nsIFile*, nsTSubstring<char>&)
XPTC__InvokebyIndex
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)
nsIFileProtocolHandler.getURLSpecFromFile
XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
Interpret
js::RunScript(JSContext*, js::RunState&)
displayDownloadDirPrefTask
displayDownloadDirPref
init

This is from this call and very sad.

We can obviously specialcase the network path case somewhere, though of course that also increases complexity of the network functions. It'd also be interesting to align the behaviour of the file: URL conversion with other browsers; the code comment in the conversion method talks about the importance of the trailing slash, but the consumer here definitely does not care about this: https://searchfox.org/mozilla-central/rev/59e797b66f5ce8a27ede0e7677688931be7aed20/netwerk/base/nsURLHelper.cpp#118-123 .

A stupid workaround we can employ in the prefs is doing the conversion ourselves... in which case we could at least ensure that we use off-mainthread IO. But that feels pretty dumb.

Also, I could have sworn I've run into this code before but I can't find any other bugreports on file...

Profile: https://share.firefox.dev/3zwvG56

Comment 1

[:Gijs (he/him)]

Looks like bug 890712 covers the root cause here. Florian, how frequent is this problem in BHR (which you reference in that bug) ?

Comment 2

[Florian Quèze [:florian]]

This bug is very rare on BHR (I think I saw it once the day you reported). Maybe the people who use network shares the most are enterprise users, with Telemetry disabled? (or just not using Nightly)

Bug 890712 is more common, but still rare on BHR (no more than 0.01% of the total reported hang time).

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I think we can avoid the IO by calling nsFileProtocolHandler::GetURLSpecFromActualFile/GetURLSpecFromDir instead, when we know/expect if it's a file or directory. I think this works well for displayDownloadDirPrefTask, although it's not a very general fix.

:Gijs what do you think?

Comment 4

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #3)

I think we can avoid the IO by calling nsFileProtocolHandler::GetURLSpecFromActualFile/GetURLSpecFromDir instead, when we know/expect if it's a file or directory. I think this works well for displayDownloadDirPrefTask, although it's not a very general fix.

:Gijs what do you think?

Hm, yeah, that fixes the prefs case and makes sense. I do wonder what other browsers do. The relative path argument for wanting to know if it's a file or dir makes sense to me - but AFAICT 0 of the JS callers care about this (or, if/where they do, they know the answer already). It's a bit harder to tell with the C++ consumers - but I'm tempted to suggest we should remove the helper function entirely, and leave only the ActualFile and Dir versions, so consumers are forced to choose (and/or do their own mainthread IO only where required.

WDYT?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Great point. I think removing nsIFileProtocolHandler.getURLSpecFromFile is definitely something we want.
I think a few of the C++ consumers can be converted - not sure if we can get rid of it completely, but we should make sure the MT I/O is not accidental.

Can you split the JS work into a separate bug and we can keep this for the C++ consumers?

Comment 6

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #5)

Great point. I think removing nsIFileProtocolHandler.getURLSpecFromFile is definitely something we want.
I think a few of the C++ consumers can be converted - not sure if we can get rid of it completely, but we should make sure the MT I/O is not accidental.

Can you split the JS work into a separate bug and we can keep this for the C++ consumers?

Yep, filed bug 1723723.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: WIP: Bug 1722304 - Avoid uses of net_GetURLSpecFromFile r=#necko

Comment 8

[:Gijs (he/him)]

Hey Valentin! What's the state of this patch/bug? :-)

Comment 9

[Valentin Gosu [:valentin] (he/him)]

This is in my backlog, but I don't expect to work on it very soon.
Feel free to assign it to someone else, or if it's blocking important work let me know and I'll try to find some cycles for this.
Thanks!