Just stumbled upon this. Why does process launching (and in particular Windows) require thread-affinity? I was going to move this to a SequencedTaskRunner too when I stumbled upon https://chromium-review.googlesource.com/c/chromium/src/+/941264

TL;DR: It does on Android.

Or maybe not -- I might be missing something. I thought I recalled reading that we launch processes from Java on Android via a dedicated Java thread, and my assumption is that we would abstract this behind a SingleThreadTaskRunner.

Right but the code looks like this :

#if defined(OS_ANDROID)
    // Android specializes Launcher thread so it is accessible in java.
    // Note Android never does clean shutdown, so shutdown use-after-free
    // concerns are not a problem in practice.
    // This process launcher thread will use the Java-side process-launching
    // thread, instead of creating its own separate thread on C++ side. Note
    // that means this thread will not be joined on shutdown, and may cause
    // use-after-free if anything tries to access objects deleted by
    // AtExitManager, such as non-leaky LazyInstance.
    static base::NoDestructor<scoped_refptr<base::SingleThreadTaskRunner>>
        launcher_task_runner(
            android::LauncherThread::GetMessageLoop()->task_runner());
  #else   // defined(OS_ANDROID)
    constexpr base::TaskTraits task_traits = {
        base::MayBlock(), base::TaskPriority::USER_BLOCKING,
        base::TaskShutdownBehavior::BLOCK_SHUTDOWN};
    // TODO(wez): Investigates whether we could use SequencedTaskRunner on
    // platforms other than Windows. http://crbug.com/820200.
    static base::NoDestructor<scoped_refptr<base::SingleThreadTaskRunner>>
        launcher_task_runner(base::CreateSingleThreadTaskRunnerWithTraits(
            task_traits, base::SingleThreadTaskRunnerThreadMode::DEDICATED));
  #endif  // defined(OS_ANDROID)


i.e. the TODO is in non-Android code (I know it has to be a specific thread on Android)

Ah OK. I don't know why thread-affinity is required on Windows either.

+hanxi who added the TODO

hanxi pinged me earlier about this; there are a couple of dependencies we need to address:

[0. On Android we re-use the Java-side Launcher thread, which is a dedicated thread. This works fine, AFAIK, in hanxi's current patch, so it's not really an issue to resolve. :)]
1. On Windows we are limited by broker_services.cc, which is currently coded to require that all calls are made on the same thread, to protect against concurrent access to globals.  I expect there is some subtlety that will make relaxing that to a Sequence check, or introducing locking, tricky.
2. hanxi mentions that some code (e.g. DiscardableSharedMemoryManager) is currently expecting to be run on a Thread with a MessageLoop, and registers a DestructionObserver to clean up resources when that MessageLoop is torn-down.  Do we have an equivalent mechanism to handle necessary teardown work on TaskScheduler shutdown?

+jschuh to confirm whether or not we might be able to relax the single-thread requirement of the sandbox broker_services.cc for Windows.

1. Sequences are thread-safe, just not thread-affine (i.e. access to globals is fine unless said globals are in TLS)
2. Destruction observers are typically used to bind objects to the lifetime of the MessageLoop. SequenceLocalStorageSlot can now be used for this (I'm working on a CL to make all these mappings more obvious)

Other SingleThread -> Sequence mappings can be found at https://chromium.googlesource.com/chromium/src/+/lkcr/docs/task_scheduler_migration.md#Relevant-single_thread-sequence-mappings (the problem is usually that components are overly strict about requiring SingleThread types but aren't otherwise thread-affine)

Re #9: (1) Yup, I think everyone understands that a Sequence is sufficient to ensure against concurrent access to globals, but this is sandbox code, so there may be limitations on the dependencies that code can take which would make checking the calling Sequence tricky, for example. Or it may be straightforward, and the code just needs updating. Not sure yet. :)

(2) OK, so it sounds like the DiscardableSharedMemoryManager, at least, needs to be migrated to SequenceLocalStorageSlot rather than DestructionObserver.

Thanks Wez for the summary on comment#7! That exactly what I want to say:)

Re#11: (2) The issue of DiscardableSharedMemoryManager isn't related to this bug, but thanks for bringing it up. I found it when I was working on another investigation whether we can run ServiceManager on a single-thread task runner. Currently it is running on BrowserThread::IO. Do we really benefit from keeping IO as a dedicated thread?

For my question in #12: is there any concern that we reuse the same single-thread taskrunner for BrowserThread::IO? For example, use the existing single-thread taskrunner to initialize proxies[BrowserThread::IO].

I don't think the Windows sandbox needs a single thread/affinity for process launching. I can think of reasons why it may need to be sequential (e.g. old style handle inheritance), but that's preserved by the proposed change.

I'm also adding in some smarter people to double check my recollection here.

jschuch: Is there any restriction on the things we can do in the sandbox
code-paths, though?  Can this code depend on everything in //base, (which
is where the SequenceChecker, lives) for example?

Ah, I see. I'm no longer an active owner, and don't really have an opinion on that. I suggest you ask the CC'd Windows sandbox owners if they think it's important to minimize any base dependencies, or if that ship has in fact sailed and we no longer care.

forshaw, wfh: Any thoughts on this?

I think in general we're not too concerned with taking a dependencies on //base as long as this is the browser process side (which sounds likely) rather than anything inside a sandbox. 

As for thread affinity yes there's no technical reason from a Windows perspective that you can't create processes on different threads, however it would be a fairly big change to convert the sandbox code itself to play nice being potentially called concurrently. Considering malicious renderers could in theory exercise the process creation routes of the sandbox code if they're possibly dispatched on multiple threads there could be a lurking sandbox escape in there so at least for now we shouldn't be calling browser sandbox code on multiple threads.

@ #18 : This isn't about making concurrent calls but rather sequenced calls (which may be on different threads but sequenced and therefore thread-safe -- i.e. SequencedTaskRunner instead of SingleThreadTaskRunner). Is that still a problem?

Sorry I misunderstood. I don't believe that the sandbox should have anything which requires thread affinity. Looking at the implementation of BrokerServicesBase::SpawnTarget we're taking a lock over process creation so I can't see any reason it can't be used on different threads sequentially.