1355346 - CocoaFileUtils::AddOriginMetadataToFile() called at the end of file downloads can do sync IPC on the main thread

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement, P2)

Description

[(no longer active)]

STR:

Download a file.

Profile: https://perfht.ml/2o2izEA

The sendServerMessageWithTimeout() call seems to be sending a message to a different process and blocking on it.  In this profile it's pausing for 46ms.  We should do this work off the main thread, if possible.

This is coming from: <http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/toolkit/components/jsdownloads/src/DownloadPlatform.cpp#176>

Comment 1

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

Attachment: Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.

Comment 2

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

Comment on attachment 9002139 [details]
Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.

Hey spohl,

Here's a strawman that tries to get this stuff off of the main thread.

MAJOR CAVEAT: I'm pretty unfamiliar with these structures that I'm suddenly manipulating off of the main thread. I do not know, for example, if CFStrings are threadsafe (I pass pathCFStr into the lambda). I do not know if AddOriginMetadataToFile or AddQuarantineMetadataToFile are safe to be called off of the main thread.

But on the off-chance that they are, this _might_ suffice. Just be advised that I'm not sure, and I guess that's what I'm asking here with my feedback?.

Comment 3

[:Paolo Amadini]

Comment on attachment 9002139 [details]
Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.

This should also notify completion, because we only want to consider the download finished after the file is closed and ready to be used. For example, this may be launched automatically after completion.

Comment 4

[Stephen A Pohl [:spohl]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)
> Comment on attachment 9002139 [details]
> Bug 1355346 - [WIP] Call nsCocoaFileUtils::AddOriginMetadataToFile off of
> the main-thread when completing downloads.
>
> MAJOR CAVEAT: I'm pretty unfamiliar with these structures that I'm suddenly
> manipulating off of the main thread. I do not know, for example, if
> CFStrings are threadsafe (I pass pathCFStr into the lambda).

This should be fine. The CFString is an immutable object and it is only accessed by the IO thread after creation.

> I do not know
> if AddOriginMetadataToFile or AddQuarantineMetadataToFile are safe to be
> called off of the main thread.

It appears that all CF objects in question here are only accessed from one thread, so this should be fine as well.

Some background on thread safety, particularly involving Core Foundation (CF) objects:
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html#//apple_ref/doc/uid/10000057i-CH12-SW9

Comment 5

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

Attachment: Bug 1355346 - Make mozIDownloadPlatform::downloadDone return a Promise. r=paolo,spohl

Depends on D3674

Comment 6

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

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a1304f60c865c64c3e66e4588af7f15a5c2d779

Comment 7

[:Paolo Amadini]

Comment on attachment 9002595 [details]
Bug 1355346 - Make mozIDownloadPlatform::downloadDone return a Promise. r=paolo,spohl

:Paolo Amadini has approved the revision.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 9002139 [details]
Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.

Markus Stange [:mstange] has approved the revision.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 9002595 [details]
Bug 1355346 - Make mozIDownloadPlatform::downloadDone return a Promise. r=paolo,spohl

Markus Stange [:mstange] has approved the revision.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/566a5b21073a
Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/acfaacaca4cb
Make mozIDownloadPlatform::downloadDone return a Promise. r=paolo,mstange

Comment 11

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/566a5b21073a
https://hg.mozilla.org/mozilla-central/rev/acfaacaca4cb