1708152 - [meta] Stop using std::thread because it can throw C++ exceptions

Status: NEW

(Core :: XPCOM, task, P5)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Spawned by bug 1701570 comment 12:

[The C++] exception is originating here. It's being thrown from deep inside std::thread::thread().

And according to https://en.cppreference.com/w/cpp/thread/thread/thread , the main constructor can indeed throw:

std::system_error if the thread could not be started. The exception may represent the error condition std::errc::resource_unavailable_try_again or another implementation-specific error condition.

Since Firefox is built without exception handling, exceptions from std::thread cannot be caught and will result in crashes.

So I think we should get rid of all instances of std::thread (and replace them with nsIThread or PR_Thread), and if possible find a way to prevent its reintroduction. Maybe even remove 'thread' from https://searchfox.org/mozilla-central/source/config/stl-headers.mozbuild ?

List: https://searchfox.org/mozilla-central/search?q=symbol:T_std%3A%3Athread
(Bug 1701570 is removing the one in widget/windows/RemoteBackbuffer.h.)

This bug may become a meta-bug under which sub-bugs would tackle different components.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

RemoteBackBuffer, Cubeb and the logging code in dom/media appear to be the users. I'm not sure if we actually touch the Skia version.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Code coverage says we don't use the Skia code there.

Comment 3

[Mike Hommey [:glandium]]

Note: code coverage may not be totally accurate for graphics related things.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Gerald Squelart [:gerald] (he/him) from comment #0)

get rid of all instances of std::thread (and replace them with nsIThread or PR_Thread)

About the replacement candidates, some helpful notes on which one to choose:

• From https://phabricator.services.mozilla.com/D113576#inline-631339 :

std::thread::thread(...) and PR_CreateThread are roughly equivalent -- They just spawn an OS thread and start running the passed-in function, whereas NS_NewNamedThread is a much higher-level "thread + event queue + message loop" abstraction that's meant for things like UI threads, message pumps, and task runners.

• https://phabricator.services.mozilla.com/D113576#3691465 :

if we are looking deliberately for an OS thread like in this use case then we want to use PR_CreateThread. We want to favor using NS_NewNamedThread at every opportunity (and also route everything we can into scheduler or background thread pool).
we should have a comment [where relevant] about why we use a PRThread and not an nsIThread.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

This may help with bug 1465926, so it's good to remove std::thread in any case.

But that made me think: The original issue is due to some std:: API that can throw, used in our codebase without exception handling.
Would it be possible to enable exceptions in single compilation units? This way we could have small wrappers with try-catch(...) blocks around exception-throwing external functions, which could then be used elsewhere with no risk of throwing exceptions and crashing. (If yes, and agreeable, this would be worth a separate bug.)