1390681 - SubtleCrypto thread pool has bad behavior, keeps starting and stopping threads

Status: RESOLVED INACTIVE

(Core :: Security, enhancement, P3)

Description

[Markus Stange [:mstange]]

Ever since I've installed the Bitwarden extension, startup has become really janky for me. This seems to be because Bitwarden uses window.crypto.subtle.sign, e.g. here: https://github.com/bitwarden/browser/blob/6ac29cf528f74b1e18213443e2a1b6cd453b2df1/src/services/cryptoService.js#L729
And SubtleCrypto dispatches crypto tasks to a thread pool, which seems to keep launching new threads.

I'm attaching a MOZ_LOG=nsThreadPool:5 log of my startup, filtered to only contain lines from the SubtleCrypto thread pool (0x11a9f17a0). This log is over 200000 lines long, and the string "shutdown async" (from http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/xpcom/threads/nsThreadPool.cpp#137 ) occurs 14323 times in it.

Comment 1

[Markus Stange [:mstange]]

Attachment: MOZ_LOG=nsThreadPool:5 log, zipped

Comment 2

[Markus Stange [:mstange]]

Attachment: first 1000 lines of log, as plain text

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1390681 - Use NS_DISPATCH_AT_END when dispatching crypto tasks to the thread pool, so that we don't launch new threads if all threads are busy.

Review commit: https://reviewboard.mozilla.org/r/168884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168884/

Comment 4

[Markus Stange [:mstange]]

Using NS_DISPATCH_AT_END seems to help quite a bit. It's still janking because there's so much JS running on the parent process, but the big pauses are gone, as far as I can tell.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8897624 [details]
Bug 1390681 - Use NS_DISPATCH_AT_END when dispatching crypto tasks to the thread pool, so that we don't launch new threads if all threads are busy.

https://reviewboard.mozilla.org/r/168884/#review174496

Looks reasonable.  This is what is used in SharedThreadPool as well.

::: dom/crypto/WebCryptoThreadPool.cpp:79
(Diff revision 1)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      pool.swap(mPool);
>    }
>  
> -  return mPool->Dispatch(aRunnable, NS_DISPATCH_NORMAL);
> +  return mPool->Dispatch(aRunnable, NS_DISPATCH_AT_END);

Can you add a comment about why NS_DISPATCH_AT_END is used here?  I don't think many people are aware of this flag.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8897624 [details]
Bug 1390681 - Use NS_DISPATCH_AT_END when dispatching crypto tasks to the thread pool, so that we don't launch new threads if all threads are busy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168884/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98f4de7949a0
Use NS_DISPATCH_AT_END when dispatching crypto tasks to the thread pool, so that we don't launch new threads if all threads are busy. r=bkelly

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=123669626&repo=autoland on at least android.

https://hg.mozilla.org/integration/autoland/rev/34376824c7dedb76807116372f6b6c6c2ae52c02

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

You also had mochitest-chrome failures like https://treeherder.mozilla.org/logviewer.html#?job_id=123669918&repo=autoland on linux.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And crashtest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=123669919&repo=autoland

Comment 11

[Markus Stange [:mstange]]

Huh, ok. Sorry about that.

I'm abandoning this bug for now; if somebody else wants to debug what's going on here, feel free to pick it up.

Comment 12

[Nathan Froyd [:froydnj]]

Notice that the documentation for DISPATCH_AT_END says that you need to be using that flag when you're running an event that's already running on the event target.  I suspect that condition doesn't hold here.

What is probably worth doing instead is constraining the thread pool to run a few threads all the time, so that it doesn't try to constantly start and shutdown threads.   You'd call SetThreadLimit/SetIdleThreadLimit and pass them the same number of threads.

Comment 13

[Honza Bambas (:mayhemer)]

If this is still using nsIThreadPool then you can look at the changes in bug 1535361. We could also globally change the default to be 5 instead of 1. I only wanted to be careful in the bug ;)

Comment 14

[Rob Wu [:robwu]]

WebCryptoThreadPool got removed in bug 1583646, and the tasks are dispatched to a singleton background thread instead.

I assume that this issue has been fixed, but if not, please re-open or file a new bug.