Shared memory programming policy in Blink

[Kentaro Hara]

Hi

Recently there were multiple discussions about introducing a shared memory programming pattern in Blink (i.e., mutex locks and atomics). I'm personally generally in favor of not allowing a shared memory programming pattern but I think it's better to reach consensus and describe the guideline at blink/renderer/README.md.

With that motivation in mind, I tried to write a draft here:

========

### Threading model

When you need to use threads in Blink, cross-thread communication should be done with a message passing model (i.e., call cross_thread_task_runner->postTask() with cloned input parameters). A shared memory model (i.e., using mutex locks or atomics) is strongly discouraged. The rationale is that mutex locks and atomics are really error-prone, and even if it appears to be manageable initially, it gets out of control easily. Historically, shared memory programming patterns in Blink have been one of the major sources of use-after-free security bugs and stability issues (e.g., WebAudio, memory access via CrossThreadPersistent). Remember that, unlike V8, Blink does not have a strict API boundary and is touched by many developers, and thus it's more important to adopt a less error-prone programming pattern.

Introducing a few mutex locks or atomics in contained classes (e.g., shared counters) is fine. However, when you need to introduce a non-trivial number of mutex locks and atomics, the architecture needs to be designed and reviewed carefully. Please get approval from platform-architecture-dev@.

========

This is an initial draft -- any disagreement / suggestions are welcome! If you have major comments, please speak up in this thread. If you want to tweak the text, please suggest edits in this doc.

Thanks!

--

Kentaro Hara, Tokyo

[dan...@chromium.org]

I think this is absolutely right. First, it matches Chrome in general. Second, it's very hard to do this without serious bugs in C++.

<puts on language hat> I also think you could qualify all of this with "in C++". In some possible future with Rust, we could write threaded software without message passing and without worrying about such bugs.</hat>

[Jeremy Roman]

While Rust does address the memory safety side, I think the other complications, like correctness issues or resistance to refactoring due to a variety of possible orderings, do apply regardless of language to nontrivial cases.

 

Thanks!

--

Kentaro Hara, Tokyo

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHtyhaR0St3wQuztac6WG9Q-BQSZpUzAi1_u6WO9GmOXt-yXjw%40mail.gmail.com.

[dan...@chromium.org]

On Thu, Sep 22, 2022 at 5:35 PM Jeremy Roman <jbr...@chromium.org> wrote:

On Thu, Sep 22, 2022 at 9:23 AM <dan...@chromium.org> wrote:

On Thu, Sep 22, 2022 at 4:47 AM Kentaro Hara <har...@chromium.org> wrote:

Hi

Recently there were multiple discussions about introducing a shared memory programming pattern in Blink (i.e., mutex locks and atomics). I'm personally generally in favor of not allowing a shared memory programming pattern but I think it's better to reach consensus and describe the guideline at blink/renderer/README.md.

With that motivation in mind, I tried to write a draft here:

========

### Threading model

When you need to use threads in Blink, cross-thread communication should be done with a message passing model (i.e., call cross_thread_task_runner->postTask() with cloned input parameters). A shared memory model (i.e., using mutex locks or atomics) is strongly discouraged. The rationale is that mutex locks and atomics are really error-prone, and even if it appears to be manageable initially, it gets out of control easily. Historically, shared memory programming patterns in Blink have been one of the major sources of use-after-free security bugs and stability issues (e.g., WebAudio, memory access via CrossThreadPersistent). Remember that, unlike V8, Blink does not have a strict API boundary and is touched by many developers, and thus it's more important to adopt a less error-prone programming pattern.

Introducing a few mutex locks or atomics in contained classes (e.g., shared counters) is fine. However, when you need to introduce a non-trivial number of mutex locks and atomics, the architecture needs to be designed and reviewed carefully. Please get approval from platform-architecture-dev@.

========

This is an initial draft -- any disagreement / suggestions are welcome! If you have major comments, please speak up in this thread. If you want to tweak the text, please suggest edits in this doc.

I think this is absolutely right. First, it matches Chrome in general. Second, it's very hard to do this without serious bugs in C++.

<puts on language hat> I also think you could qualify all of this with "in C++". In some possible future with Rust, we could write threaded software without message passing and without worrying about such bugs.</hat>

While Rust does address the memory safety side, I think the other complications, like correctness issues or resistance to refactoring due to a variety of possible orderings, do apply regardless of language to nontrivial cases.

For sure. Given they exist, do you think those should be part of the message being presented by haraken?

[Kentaro Hara]

In the meantime, Blink's coding guideline assumes C++ and I don't think there is any agreed-on plan to introduce Rust to Blink. Maybe can we revisit the implication of Rust when Blink decides to adopt it? :)

--

Kentaro Hara, Tokyo

[dan...@chromium.org]

On Fri, Sep 23, 2022 at 10:31 AM Kentaro Hara <har...@chromium.org> wrote:

In the meantime, Blink's coding guideline assumes C++ and I don't think there is any agreed-on plan to introduce Rust to Blink. Maybe can we revisit the implication of Rust when Blink decides to adopt it? :)

Absolutely. I didn't mean to derail this, just to point out if these are restrictions that are getting in the way there are other possible ways to address it in the future.

[Hiroki Nakagawa]

The draft looks good to me from the perspective of web workers that heavily use Blink threads.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHtyhaQgmeqzifna0N810mH6_vp%3DU%2BqSwuTPJhdJrBrrgfgT2g%40mail.gmail.com.

[Michael Lippautz]

From a GC perspective the guideline sgtm.

I want to put out there that I think we can generally come up with better building blocks than CrossThreadPersistent that are easier to use and provide the same or better performance though. Going through a discussion on p-a-d@ sgtm if this is needed.

[Kentaro Hara]

It looks like there's no objection to the guideline. Uploaded a CL here. If you want to tweak the text, please comment on the CL. I'll land it in a few days :)

--

Kentaro Hara, Tokyo