1527712 - nsFileChannel (called e.g. by fetch) shouldn't do main thread I/O for local files

Status: RESOLVED FIXED

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

Description

Near the end of startup (I think when loading activity stream), there are 21 stat calls for png files in the thumbnails folder of the profile. Apparently we do 3 stat calls per local file we are fetching.

static bool SandboxFetchPromise(struct JSContext *, unsigned int, union JS::Value *) [xul.dll]
mozilla::dom::FetchRequest(nsIGlobalObject *,mozilla::dom::RequestOrUSVString const &,mozilla::dom::RequestInit const &,mozilla::dom::CallerType,mozilla::ErrorResult &) [xul.dll]
nsresult mozilla::dom::FetchDriver::Fetch(class mozilla::dom::AbortSignalImpl *, class mozilla::dom::FetchDriverObserver *) [xul.dll]
nsresult mozilla::dom::FetchDriver::HttpFetch(const class nsTSubstring<char> & const) [xul.dll]
nsBaseChannel::AsyncOpen(nsIStreamListener *) [xul.dll]
nsresult nsFileChannel::OpenContentStream(bool, class nsIInputStream * *, class nsIChannel * *) [xul.dll]
nsresult nsLocalFile::GetFileSize(__int64 *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

nsThread::ProcessNextEvent(bool,bool *) [xul.dll]
nsresult mozilla::dom::`anonymous namespace'::BeginConsumeBodyRunnable<mozilla::dom::Response>::Run() [xul.dll]
nsresult mozilla::dom::FetchBodyConsumer<mozilla::dom::EmptyBody>::GetBodyLocalFile(class nsIFile * *) [xul.dll]
nsLocalFile::Exists(bool *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

This is at https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/dom/fetch/FetchConsumer.cpp#505 and looks like a regression from bug 1482752.

nsThread::ProcessNextEvent(bool,bool *) [xul.dll]
nsresult mozilla::dom::`anonymous namespace'::BeginConsumeBodyRunnable<mozilla::dom::Response>::Run() [xul.dll]
mozilla::dom::FileCreatorHelper::CreateFile(nsIGlobalObject *,nsIFile *,mozilla::dom::ChromeFilePropertyBag const &,bool,mozilla::ErrorResult &) [xul.dll]
static struct already_AddRefed<mozilla::dom::File> mozilla::dom::FileCreatorHelper::CreateFileInternal(class nsPIDOMWindowInner *, class nsIFile *, const struct mozilla::dom::ChromeFilePropertyBag & const, bool, class mozilla::ErrorResult & const) [xul.dll]
static nsresult mozilla::dom::FileCreatorHelper::CreateBlobImpl(class nsIFile *, const class nsTSubstring<char16_t> & const, const class nsTSubstring<char16_t> & const, bool, __int64, bool, bool, class mozilla::dom::BlobImpl * *) [xul.dll]
nsresult mozilla::dom::MultipartBlobImpl::InitializeChromeFile(class nsIFile *, const class nsTSubstring<char16_t> & const, const class nsTSubstring<char16_t> & const, bool, __int64, bool) [xul.dll]
nsLocalFile::Exists(bool *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

This is at https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/dom/file/MultipartBlobImpl.cpp#331

All this information is from this profile: https://perfht.ml/2DQYNoG

Comment 1

[Honza Bambas (:mayhemer)]

Good findings. This is known, I think, and not easy to act on, I'm afraid. Moving to Net:File as there would probably be the fix.

Tentatively P1, as I'd like to at least start discussion on how to improve the situation here.

Comment 2

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

Could we make nsBaseChannel::BeginPumpingData() be called asynchronously on another thread? And if it fails we fail the channel?

Comment 3

[:Gijs (he/him)]

Note that the top stack here:

nsBaseChannel::AsyncOpen(nsIStreamListener *) [xul.dll]
nsresult nsFileChannel::OpenContentStream(bool, class nsIInputStream * *, class nsIChannel * *) [xul.dll]
nsresult nsLocalFile::GetFileSize(__int64 *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

affects XHRs, too.

Comment 4

[Honza Bambas (:mayhemer)]

We definitely also stat the file from nsFileChannel::Init, I'm surprised you didn't catch that as well. I think we need the symlink resolved uri prior calling asyncopen to be able to do proper security checking. anyway, this is hard and sensitive to fix while we create channel and do secchecks on the main thread, in one loop. as it's not listed in the stacks in comment 0 or later on, I will dedicate this bug to an attempt to get rid of at least those stats we do from inside asyncOpen. That should be doable relatively easily.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: Bug 1527712 - Move I/O in nsFileChannel::OpenContentStream to a background thread, r=kershaw,dragana

Comment 6

[Honza Bambas (:mayhemer)]

Seems to cause a permafailure:
https://bugzilla.mozilla.org/show_bug.cgi?id=1471737, browser/components/preferences/in-content/tests/browser_subdialogs.js

Comment 7

[Honza Bambas (:mayhemer)]

I believe the defect is in the browser_subdialogs.js test itself. I double checked that all channel finish loading and also tested to do the background IO loop blocking call of asyncOpen while also doing a sync resolution of mContentLength as before just in case someone wants the content length be known before OnStartRequest call.

The test is apparently time sensitive.

Comment 8

[:Gijs (he/him)]

Please let me know if I can help in some way to get that test to behave better.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to :Gijs (he/him) from comment #8)

Please let me know if I can help in some way to get that test to behave better.

I already ni?'ed few people that have changed the test recently, maybe I'll get some answer soon. I also raised the priority to reflect this bug.

I think I should do some talos pushes too... also, stream transport service pool can create (and probably immediately destroy) ~400 threads during that single local test run; something I'm going to look into now as it doesn't seem OK to me at all.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to :Gijs (he/him) from comment #8)

Please let me know if I can help in some way to get that test to behave better.

If you want, just grab bug 1471737. I added few comments with some initial pointers.

Comment 11

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c645a58e7734d006eb6573c0d0ba686e5933ca2d
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=e1cd1db23f8ba1382ec96d929404a2ddf60562c3

Comment 12

[Honza Bambas (:mayhemer)]

The talos compare shows some weird regression only on win7-32, which is probably not something caused by this patch.

Let's land it.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2657a29a6d76
Move I/O in nsFileChannel::OpenContentStream to a background thread, r=kershaw,dragana

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/2657a29a6d76

Comment 15

[Chris Peterson [:cpeterson]]

67=wontfix because there's no need to uplift to 67 Beta

Comment 16

[Honza Bambas (:mayhemer)]

Backed out in bug 1538535 for possible cause of bug 1538098.

Comment 17

[Honza Bambas (:mayhemer)]

We can reland, as this has been proven to NOT cause bug 1538098 (https://bugzilla.mozilla.org/show_bug.cgi?id=1538535#c3).

I'd rather wait a bit until m-c stabilize again.

Comment 18

[Honza Bambas (:mayhemer)]

Problem identified elsewhere.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d121ec2b3d8a
Move I/O in nsFileChannel::OpenContentStream to a background thread, r=kershaw,dragana

Comment 20

[Raul Gurzau (:RaulG)]

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