Delete CrossThreadCopier [chromium/src : main]

0 views
Skip to first unread message

Daniel Cheng (Gerrit)

unread,
Nov 20, 2025, 6:31:39 PMNov 20
to Daniel Cheng, Michael Lippautz, Jeremy Roman, Andrey Kosyakov, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
Attention needed from Andrey Kosyakov, Florin Malita, Jeremy Roman and Michael Lippautz

Daniel Cheng added 6 comments

Patchset-level comments
File-level comment, Patchset 5 (Latest):
Daniel Cheng . unresolved

@mlip...@chromium.org and @jbr...@chromium.org for the overall review.

tl;dr I really don't think the costs of `CrossThreadCopier` justify its existence anymore. It's true that Oilpan types are still thread-hostile, but the issue has always been transitive violations, which the current traits don't really do much for anyway.

I have toyed with the idea of having some marker for thread-hostile things, and then having the clang plugin derive thread-safety transitively and enforce it for things that care (like `CrossThreadBind...`)–basically, the Rust style of "compiler derives `Send` for you where it can" but I don't think figuring that out should be blocking for this cleanup either. Please let me know you feel differently.

@ca...@chromium.org, @fma...@chromium.org I left some implementation-specific questions below–PTAL at those if you can.

File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
Line 51, Patchset 5 (Latest): params->IsolatedCopy()));
Daniel Cheng . unresolved

If we don't call `IsolatedCopy()` here, it won't build.

Technically you can still do an unsafe thing here though if you call `std::move(*this)`...

File third_party/blink/renderer/modules/webcodecs/background_readback.cc
Line 68, Patchset 5 (Parent): : public CrossThreadCopierPassThrough<base::span<uint8_t>> {
Daniel Cheng . resolved

This is an example of a specialization that's not really defined in the correct place and could lead to ODR issues.

Actually even bindings spans is pretty dubious... but that's something I'm tackling separately.

File third_party/blink/renderer/platform/bindings/source_location.h
Line 36, Patchset 5 (Latest):// GarbageCollected heap but it doesn't actually have to.
Daniel Cheng . unresolved

@ca...@chromium.org, is this really load-bearing? I see this was made GCed in https://chromium-review.googlesource.com/c/chromium/src/+/6485451 but from my perspective it's only adding complexity.

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h
Line 146, Patchset 5 (Parent): CHECK(bitmap.isImmutable() || bitmap.isNull())
Daniel Cheng . unresolved

@fma...@chromium.org, do we really need to keep these runtime checks?

(If this were code outside of Blink, we just wouldn't have these checks at all)

File third_party/blink/renderer/platform/wtf/cross_thread_copier_skia.h
Line 48, Patchset 5 (Parent): "sk_sp<T> can be passed across threads only if T is SkRefCnt.");
Daniel Cheng . unresolved

@fma...@chromium.org, is this `static_assert` really load-bearing? It seems "nice to have" but I'm not sure it's "keep hundreds of lines of C++ specializations nice to have".

Open in Gerrit

Related details

Attention is currently required from:
  • Andrey Kosyakov
  • Florin Malita
  • Jeremy Roman
  • Michael Lippautz
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
Gerrit-Change-Number: 6685109
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
Gerrit-CC: Hongchan Choi <hong...@chromium.org>
Gerrit-CC: Menard, Alexis <alexis...@intel.com>
Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-CC: Xida Chen <xida...@chromium.org>
Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
Gerrit-Attention: Andrey Kosyakov <ca...@chromium.org>
Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
Gerrit-Attention: Florin Malita <fma...@chromium.org>
Gerrit-Comment-Date: Thu, 20 Nov 2025 23:31:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Andrey Kosyakov (Gerrit)

unread,
Nov 20, 2025, 7:21:42 PMNov 20
to Daniel Cheng, Michael Lippautz, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
Attention needed from Daniel Cheng, Florin Malita, Jeremy Roman and Michael Lippautz

Andrey Kosyakov voted and added 11 comments

Votes added by Andrey Kosyakov

Code-Review+1

11 comments

Patchset-level comments
Andrey Kosyakov . resolved

bindings/ and inspector/ lgtm

File third_party/blink/renderer/core/css/css_syntax_definition.h
Line 56, Patchset 5 (Latest): CSSSyntaxDefinition IsolatedCopy() const;
Andrey Kosyakov . unresolved

Looks like you've just deleted the only call site, so this can go as well.

File third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.h
File-level comment, Patchset 5 (Parent):
Andrey Kosyakov . unresolved

Please nuke the entire file.

File third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.cc
File-level comment, Patchset 5 (Parent):
Andrey Kosyakov . unresolved

Please nuke the entire file.

File third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.h
File-level comment, Patchset 5 (Parent):
Andrey Kosyakov . unresolved

Nuke the entire file?

File third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.cc
File-level comment, Patchset 5 (Parent):
Andrey Kosyakov . unresolved

ditto.

File third_party/blink/renderer/core/workers/custom_event_message.h
Line 18, Patchset 5 (Latest): scoped_refptr<blink::SerializedScriptValue> message;
Andrey Kosyakov . unresolved

drive-by: looks like this may be destroyed on a thread different from its creation thread (both before and after this change), and `~SerializedScriptValue()` is not entirely thread safe, at least when it comes to [non-production code](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=12455;drc=db569cd7847d479358bd391a18b2c28393803333). The prod code looks thread-safe though, at least at a quick glance.

File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
Line 51, Patchset 5 (Latest): params->IsolatedCopy()));
Daniel Cheng . unresolved

If we don't call `IsolatedCopy()` here, it won't build.

Technically you can still do an unsafe thing here though if you call `std::move(*this)`...

Andrey Kosyakov

Looks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.

File third_party/blink/renderer/platform/bindings/cross_thread_source_location.h
Line 51, Patchset 5 (Latest): // TODO(dcheng): Unclear if this is even needed.
Andrey Kosyakov . unresolved

It's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).

File third_party/blink/renderer/platform/bindings/source_location.h
Line 36, Patchset 5 (Latest):// GarbageCollected heap but it doesn't actually have to.
Daniel Cheng . resolved

@ca...@chromium.org, is this really load-bearing? I see this was made GCed in https://chromium-review.googlesource.com/c/chromium/src/+/6485451 but from my perspective it's only adding complexity.

Andrey Kosyakov

The original plan was to start using some v8 types in SourceLocation, but for the time being further development went a slightly different path. That said, I think it may still have some merit. A SourceLocation being sent accross threads is a very infrequent case, whereas SourceLocation (proper) is rather performance-critical and we want it to be as fast as possible. Considering it's not uncommon to discard it, we primarily care for capturing source location to be fast, and from this point of view keeping v8 types and converting lazily might help.

File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h
Line 132, Patchset 5 (Parent):struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>
Andrey Kosyakov . unresolved

drive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?

Open in Gerrit

Related details

Attention is currently required from:
  • Daniel Cheng
  • Florin Malita
  • Jeremy Roman
  • Michael Lippautz
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
      Gerrit-Attention: Florin Malita <fma...@chromium.org>
      Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
      Gerrit-Comment-Date: Fri, 21 Nov 2025 00:21:27 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Daniel Cheng (Gerrit)

      unread,
      Nov 20, 2025, 8:25:42 PMNov 20
      to Daniel Cheng, Kentaro Hara, Andrey Kosyakov, Michael Lippautz, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
      Attention needed from Andrey Kosyakov, Florin Malita, Jeremy Roman and Michael Lippautz

      Daniel Cheng added 8 comments

      File third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.h
      Andrey Kosyakov . resolved

      Please nuke the entire file.

      Daniel Cheng

      This is intentional, to avoid dealing with IWYU errors. Copied the TODO here.

      File third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.cc
      Andrey Kosyakov . resolved

      Please nuke the entire file.

      Daniel Cheng

      This one I missed though. Done now.

      File third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.h
      Andrey Kosyakov . resolved

      Nuke the entire file?

      Daniel Cheng

      Intentionally kept for now but copied the TODO here as well.

      File third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.cc
      Andrey Kosyakov . resolved

      ditto.

      Daniel Cheng

      Good call, done.

      File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
      Line 51, Patchset 5: params->IsolatedCopy()));
      Daniel Cheng . unresolved

      If we don't call `IsolatedCopy()` here, it won't build.

      Technically you can still do an unsafe thing here though if you call `std::move(*this)`...

      Andrey Kosyakov

      Looks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.

      Daniel Cheng

      Yes, sorry–to be clear, calling `std::move(*this)` would be a bug, and I am highlighting this as a thing that we would no longer be able to catch.

      File third_party/blink/renderer/platform/bindings/cross_thread_source_location.h
      Line 51, Patchset 5: // TODO(dcheng): Unclear if this is even needed.
      Andrey Kosyakov . unresolved

      It's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).

      Daniel Cheng

      I'm going to have to think about how to make this CL safe in light of this.

      File third_party/blink/renderer/platform/bindings/source_location.h
      Line 36, Patchset 5:// GarbageCollected heap but it doesn't actually have to.
      Daniel Cheng . resolved

      @ca...@chromium.org, is this really load-bearing? I see this was made GCed in https://chromium-review.googlesource.com/c/chromium/src/+/6485451 but from my perspective it's only adding complexity.

      Andrey Kosyakov

      The original plan was to start using some v8 types in SourceLocation, but for the time being further development went a slightly different path. That said, I think it may still have some merit. A SourceLocation being sent accross threads is a very infrequent case, whereas SourceLocation (proper) is rather performance-critical and we want it to be as fast as possible. Considering it's not uncommon to discard it, we primarily care for capturing source location to be fast, and from this point of view keeping v8 types and converting lazily might help.

      Daniel Cheng

      OK I'm willing to buy that argument. I just worry because it's currently easy to bind this unsafely. I've reverted the changes to this file.

      File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h
      Line 132, Patchset 5 (Parent):struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>
      Andrey Kosyakov . unresolved

      drive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?

      Daniel Cheng

      There is a static_assert in `CrossThreadBindOnce()` that theoretically should catch this.

      But it doesn't catch transitive violations, e.g. see https://chromium-review.googlesource.com/7167966

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Andrey Kosyakov
      • Florin Malita
      • Jeremy Roman
      • Michael Lippautz
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement is not satisfiedCode-Review
          • requirement is not satisfiedNo-Unresolved-Comments
          • requirement is not satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
          Gerrit-Change-Number: 6685109
          Gerrit-PatchSet: 6
          Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
          Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
          Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
          Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
          Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
          Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
          Gerrit-CC: Hongchan Choi <hong...@chromium.org>
          Gerrit-CC: Kentaro Hara <har...@chromium.org>
          Gerrit-CC: Menard, Alexis <alexis...@intel.com>
          Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
          Gerrit-CC: Nate Chapin <jap...@chromium.org>
          Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
          Gerrit-CC: Stephen Chenney <sche...@chromium.org>
          Gerrit-CC: Xida Chen <xida...@chromium.org>
          Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
          Gerrit-Attention: Andrey Kosyakov <ca...@chromium.org>
          Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
          Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
          Gerrit-Attention: Florin Malita <fma...@chromium.org>
          Gerrit-Comment-Date: Fri, 21 Nov 2025 01:25:31 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Andrey Kosyakov <ca...@chromium.org>
          Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Florin Malita (Gerrit)

          unread,
          Nov 21, 2025, 10:43:00 AMNov 21
          to Daniel Cheng, Kentaro Hara, Andrey Kosyakov, Michael Lippautz, Jeremy Roman, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
          Attention needed from Andrey Kosyakov, Daniel Cheng, Jeremy Roman and Michael Lippautz

          Florin Malita added 2 comments

          File third_party/blink/renderer/platform/graphics/skia/skia_utils.h
          Line 146, Patchset 5 (Parent): CHECK(bitmap.isImmutable() || bitmap.isNull())
          Daniel Cheng . unresolved

          @fma...@chromium.org, do we really need to keep these runtime checks?

          (If this were code outside of Blink, we just wouldn't have these checks at all)

          Florin Malita

          New code should use `SkImage`s instead, which are intrinsically immutable. Removing sgtm.

          File third_party/blink/renderer/platform/wtf/cross_thread_copier_skia.h
          Line 48, Patchset 5 (Parent): "sk_sp<T> can be passed across threads only if T is SkRefCnt.");
          Daniel Cheng . unresolved

          @fma...@chromium.org, is this `static_assert` really load-bearing? It seems "nice to have" but I'm not sure it's "keep hundreds of lines of C++ specializations nice to have".

          Florin Malita

          Agreed, I don't think it's worth keeping this machinery around just for an assert.

          (it also arbitrarily excludes `SkNVRefCnt`-derived types)

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Andrey Kosyakov
          • Daniel Cheng
          • Jeremy Roman
          • Michael Lippautz
          Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
          Gerrit-Comment-Date: Fri, 21 Nov 2025 15:42:52 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Daniel Cheng (Gerrit)

          unread,
          Nov 21, 2025, 3:18:57 PMNov 21
          to Daniel Cheng, Hiroki Nakagawa, Kentaro Hara, Andrey Kosyakov, Michael Lippautz, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
          Attention needed from Andrey Kosyakov, Jeremy Roman and Michael Lippautz

          Daniel Cheng added 3 comments

          File third_party/blink/renderer/platform/bindings/cross_thread_source_location.h
          Line 51, Patchset 5: // TODO(dcheng): Unclear if this is even needed.
          Andrey Kosyakov . resolved

          It's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).

          Daniel Cheng

          I'm going to have to think about how to make this CL safe in light of this.

          Daniel Cheng

          OK, the parent CL should address this. `CrossThreadSourceLocation` is now always inherently safe to move across threads.

          File third_party/blink/renderer/platform/graphics/skia/skia_utils.h
          Line 146, Patchset 5 (Parent): CHECK(bitmap.isImmutable() || bitmap.isNull())
          Daniel Cheng . resolved

          @fma...@chromium.org, do we really need to keep these runtime checks?

          (If this were code outside of Blink, we just wouldn't have these checks at all)

          Florin Malita

          New code should use `SkImage`s instead, which are intrinsically immutable. Removing sgtm.

          Daniel Cheng

          Acknowledged

          File third_party/blink/renderer/platform/wtf/cross_thread_copier_skia.h
          Line 48, Patchset 5 (Parent): "sk_sp<T> can be passed across threads only if T is SkRefCnt.");
          Daniel Cheng . resolved

          @fma...@chromium.org, is this `static_assert` really load-bearing? It seems "nice to have" but I'm not sure it's "keep hundreds of lines of C++ specializations nice to have".

          Florin Malita

          Agreed, I don't think it's worth keeping this machinery around just for an assert.

          (it also arbitrarily excludes `SkNVRefCnt`-derived types)

          Daniel Cheng

          Acknowledged

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Andrey Kosyakov
          • Jeremy Roman
          • Michael Lippautz
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement is not satisfiedCode-Review
          • requirement is not satisfiedNo-Unresolved-Comments
          • requirement is not satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
          Gerrit-Change-Number: 6685109
          Gerrit-PatchSet: 8
          Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
          Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
          Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
          Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
          Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
          Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
          Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
          Gerrit-CC: Hongchan Choi <hong...@chromium.org>
          Gerrit-CC: Kentaro Hara <har...@chromium.org>
          Gerrit-CC: Menard, Alexis <alexis...@intel.com>
          Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
          Gerrit-CC: Nate Chapin <jap...@chromium.org>
          Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
          Gerrit-CC: Stephen Chenney <sche...@chromium.org>
          Gerrit-CC: Xida Chen <xida...@chromium.org>
          Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
          Gerrit-Attention: Andrey Kosyakov <ca...@chromium.org>
          Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
          Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
          Gerrit-Comment-Date: Fri, 21 Nov 2025 20:18:47 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Andrey Kosyakov <ca...@chromium.org>
          Comment-In-Reply-To: Florin Malita <fma...@chromium.org>
          Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Daniel Cheng (Gerrit)

          unread,
          Nov 21, 2025, 3:25:43 PMNov 21
          to Daniel Cheng, Hiroki Nakagawa, Kentaro Hara, Andrey Kosyakov, Michael Lippautz, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
          Attention needed from Andrey Kosyakov, Jeremy Roman and Michael Lippautz

          Daniel Cheng added 1 comment

          File third_party/blink/renderer/core/css/css_syntax_definition.h
          Line 56, Patchset 5: CSSSyntaxDefinition IsolatedCopy() const;
          Andrey Kosyakov . resolved

          Looks like you've just deleted the only call site, so this can go as well.

          Daniel Cheng

          Oops, fixed this too now.

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Andrey Kosyakov
          • Jeremy Roman
          • Michael Lippautz
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement is not satisfiedCode-Review
          • requirement is not satisfiedNo-Unresolved-Comments
          • requirement is not satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
          Gerrit-Change-Number: 6685109
          Gerrit-PatchSet: 9
          Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
          Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
          Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
          Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
          Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
          Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
          Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
          Gerrit-CC: Hongchan Choi <hong...@chromium.org>
          Gerrit-CC: Kentaro Hara <har...@chromium.org>
          Gerrit-CC: Menard, Alexis <alexis...@intel.com>
          Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
          Gerrit-CC: Nate Chapin <jap...@chromium.org>
          Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
          Gerrit-CC: Stephen Chenney <sche...@chromium.org>
          Gerrit-CC: Xida Chen <xida...@chromium.org>
          Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
          Gerrit-Attention: Andrey Kosyakov <ca...@chromium.org>
          Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
          Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
          Gerrit-Comment-Date: Fri, 21 Nov 2025 20:25:32 +0000
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Andrey Kosyakov (Gerrit)

          unread,
          Nov 21, 2025, 5:01:17 PMNov 21
          to Daniel Cheng, Hiroki Nakagawa, Kentaro Hara, Michael Lippautz, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
          Attention needed from Daniel Cheng, Jeremy Roman and Michael Lippautz

          Andrey Kosyakov voted and added 3 comments

          Votes added by Andrey Kosyakov

          Code-Review+1

          3 comments

          Patchset-level comments
          File-level comment, Patchset 9 (Latest):
          Andrey Kosyakov . resolved

          re-stamping

          File third_party/blink/renderer/platform/graphics/skia/skia_utils.h
          Line 138, Patchset 9 (Latest):// TODO(dcheng): This one might have some value, though it's not entirely clear
          Andrey Kosyakov . unresolved

          Remove this following the removal of class it was referring to?

          File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h
          Line 132, Patchset 5 (Parent):struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>
          Andrey Kosyakov . resolved

          drive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?

          Daniel Cheng

          There is a static_assert in `CrossThreadBindOnce()` that theoretically should catch this.

          But it doesn't catch transitive violations, e.g. see https://chromium-review.googlesource.com/7167966

          Andrey Kosyakov

          Acknowledged

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Daniel Cheng
          • Jeremy Roman
          • Michael Lippautz
          Submit Requirements:
            • requirement satisfiedCode-Coverage
            • requirement satisfiedCode-Owners
            • requirement satisfiedCode-Review
            • requirement is not satisfiedNo-Unresolved-Comments
            • requirement satisfiedReview-Enforcement
            Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
            Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
            Gerrit-Comment-Date: Fri, 21 Nov 2025 22:00:55 +0000
            Gerrit-HasComments: Yes
            Gerrit-Has-Labels: Yes
            Comment-In-Reply-To: Andrey Kosyakov <ca...@chromium.org>
            Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
            satisfied_requirement
            unsatisfied_requirement
            open
            diffy

            Daniel Cheng (Gerrit)

            unread,
            Nov 24, 2025, 12:45:26 PMNov 24
            to Daniel Cheng, Andrey Kosyakov, Hiroki Nakagawa, Kentaro Hara, Michael Lippautz, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
            Attention needed from Jeremy Roman and Michael Lippautz

            Daniel Cheng added 2 comments

            Patchset-level comments
            File-level comment, Patchset 10 (Latest):
            Daniel Cheng . resolved

            If no one has objections, I'm planning on landing this tomorrow.

            File third_party/blink/renderer/platform/graphics/skia/skia_utils.h
            Line 138, Patchset 9:// TODO(dcheng): This one might have some value, though it's not entirely clear
            Andrey Kosyakov . resolved

            Remove this following the removal of class it was referring to?

            Daniel Cheng

            Done

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Jeremy Roman
            • Michael Lippautz
            Submit Requirements:
            • requirement satisfiedCode-Coverage
            • requirement satisfiedCode-Owners
            • requirement satisfiedCode-Review
            • requirement is not satisfiedNo-Unresolved-Comments
            • requirement satisfiedReview-Enforcement
            Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
            Gerrit-MessageType: comment
            Gerrit-Project: chromium/src
            Gerrit-Branch: main
            Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
            Gerrit-Change-Number: 6685109
            Gerrit-PatchSet: 10
            Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
            Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
            Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
            Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
            Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
            Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
            Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
            Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
            Gerrit-CC: Hongchan Choi <hong...@chromium.org>
            Gerrit-CC: Kentaro Hara <har...@chromium.org>
            Gerrit-CC: Menard, Alexis <alexis...@intel.com>
            Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
            Gerrit-CC: Nate Chapin <jap...@chromium.org>
            Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
            Gerrit-CC: Stephen Chenney <sche...@chromium.org>
            Gerrit-CC: Xida Chen <xida...@chromium.org>
            Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
            Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
            Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Comment-Date: Mon, 24 Nov 2025 17:45:11 +0000
            satisfied_requirement
            unsatisfied_requirement
            open
            diffy

            Hiroshige Hayashizaki (Gerrit)

            unread,
            Nov 24, 2025, 10:44:32 PM (14 days ago) Nov 24
            to Daniel Cheng, Andrey Kosyakov, Hiroki Nakagawa, Kentaro Hara, Michael Lippautz, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
            Attention needed from Daniel Cheng, Jeremy Roman and Michael Lippautz

            Hiroshige Hayashizaki voted and added 4 comments

            Votes added by Hiroshige Hayashizaki

            Code-Review+1

            4 comments

            Commit Message
            Line 25, Patchset 10 (Latest): immutable.
            Hiroshige Hayashizaki . unresolved

            Another notable check is forbidding `scoped_refptr<>` + (non-thread-safe) `RefCounted` (in `third_party/blink/renderer/platform/wtf/cross_thread_copier_base.h`).

            It might be worth adding this check into `CrossThreadBindOnce()` like the Oilpan-related checks mentioned below (in a separate CL) -- or maybe not.

            Line 42, Patchset 10 (Latest):
            Hiroshige Hayashizaki . unresolved

            Could you mention somewhere in the commit message that this CL removes the following non-trivial `CrossThreadCopier`s that were just used to call `WTF::String`'s `CrossThreadCopier` transitively, and thus were no longer needed (even before this CL) since `WTF::String` became thread-safe?
            (Am I understanding correctly?)

            • third_party/blink/renderer/core/css/css_syntax_definition.h
            • third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.h
            • third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.h
            File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
            Line 50, Patchset 10 (Latest): CrossThreadBindOnce(&ModuleScriptFetcher::Client::OnFetched, it.key,
            Hiroshige Hayashizaki . unresolved

            Historically, this pattern of
            `PostCrossThreadTask(CrossThreadBindOnce(params->IsolatedCopy()))`
            was unsafe, if e.g. (#) the IsolatedCopy()ed result is non-movable and contains (non-thread-safe) RefCounted, because the temporary object remains on the main thread after the task is posted, causing race conditions between the temporary object and the posted object. This sounds quite rare but actually had been a major source of crashes (around 2014-2016?), because `WTF::String` and all objects contains `WTF::String` satisfied the condition (#). This was the primary motivation to call `CrossThreadCopier` inside `CrossThreadBindOnce`, not at its call sites.

            However, now the concern is gone, because IIUC there are no current cases that satisfies the condition (#) and that were previously covered by `CrossThreadCopier` (and thus should be touched by this CL).

            • `WTF::String` is now thread-safe!
            • The `IsolatedCopy()` here is the only remaining non-boilerplate/non-CHECK-only `CrossThreadCopier`. And `ModuleScriptCreationParams` is move-only and doesn't contain non-thread-safe `RefCounted` objects, so doesn't satisfies the condition (#).

            So I feel the change of this CL itself should be thread-safe.

            The remaining concern is about the future changes -- I was a bit afraid of losing this context (that the `PostCrossThreadTask(CrossThreadBindOnce(params->IsolatedCopy()))` pattern is still theoretically unsafe), but this is now more like a theoretical concern:

            • The risk of this particular pattern is not much higher than other thread-unsafe cases.
            • The fact that now there is only one code location (here) that needs non-trivial `IsolatedCopy()` (+ we use move semantics everywhere) suggests that it's also unlikely we'll add a new such cases hitting the condition (#).

            so probably it's just fine as-is.

            File third_party/blink/renderer/platform/wtf/cross_thread_functional.h
            Line 37, Patchset 10 (Latest):// std::move(f).Run(42); // Calls MyFunction(<deep copy of `obj`>, 42);
            Hiroshige Hayashizaki . unresolved

            This comment becomes obsolete (after this CL, CrossThreadBindOnce doesn't create a deep copy).

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Daniel Cheng
            • Jeremy Roman
            • Michael Lippautz
            Submit Requirements:
            • requirement satisfiedCode-Coverage
            • requirement satisfiedCode-Owners
            • requirement satisfiedCode-Review
            • requirement is not satisfiedNo-Unresolved-Comments
            • requirement satisfiedReview-Enforcement
            Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
            Gerrit-MessageType: comment
            Gerrit-Project: chromium/src
            Gerrit-Branch: main
            Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
            Gerrit-Change-Number: 6685109
            Gerrit-PatchSet: 10
            Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
            Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
            Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
            Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
            Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
            Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
            Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
            Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
            Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
            Gerrit-CC: Hongchan Choi <hong...@chromium.org>
            Gerrit-CC: Kentaro Hara <har...@chromium.org>
            Gerrit-CC: Menard, Alexis <alexis...@intel.com>
            Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
            Gerrit-CC: Nate Chapin <jap...@chromium.org>
            Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
            Gerrit-CC: Stephen Chenney <sche...@chromium.org>
            Gerrit-CC: Xida Chen <xida...@chromium.org>
            Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
            Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
            Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
            Gerrit-Comment-Date: Tue, 25 Nov 2025 03:44:06 +0000
            Gerrit-HasComments: Yes
            Gerrit-Has-Labels: Yes
            satisfied_requirement
            unsatisfied_requirement
            open
            diffy

            Michael Lippautz (Gerrit)

            unread,
            Nov 25, 2025, 1:07:54 AM (13 days ago) Nov 25
            to Daniel Cheng, Hiroshige Hayashizaki, Andrey Kosyakov, Hiroki Nakagawa, Kentaro Hara, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
            Attention needed from Daniel Cheng and Jeremy Roman

            Michael Lippautz voted and added 1 comment

            Votes added by Michael Lippautz

            Code-Review+1

            1 comment

            Patchset-level comments
            Michael Lippautz . resolved

            lgtm, having struggled with the copied and ODR myself I see this as a net win

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Daniel Cheng
            • Jeremy Roman
            Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
            Gerrit-Comment-Date: Tue, 25 Nov 2025 06:07:29 +0000
            Gerrit-HasComments: Yes
            Gerrit-Has-Labels: Yes
            satisfied_requirement
            unsatisfied_requirement
            open
            diffy

            Daniel Cheng (Gerrit)

            unread,
            Nov 26, 2025, 3:20:04 AM (12 days ago) Nov 26
            to Daniel Cheng, Michael Lippautz, Hiroshige Hayashizaki, Andrey Kosyakov, Hiroki Nakagawa, Kentaro Hara, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
            Attention needed from Jeremy Roman

            Daniel Cheng added 6 comments

            Commit Message
            Line 25, Patchset 10: immutable.
            Hiroshige Hayashizaki . resolved

            Another notable check is forbidding `scoped_refptr<>` + (non-thread-safe) `RefCounted` (in `third_party/blink/renderer/platform/wtf/cross_thread_copier_base.h`).

            It might be worth adding this check into `CrossThreadBindOnce()` like the Oilpan-related checks mentioned below (in a separate CL) -- or maybe not.

            Daniel Cheng

            Fair enough. I will mention this in the CL description. I do think there is additional work we could do to meaningfully improve things in the future and I will file a bug for that as well.

            Line 42, Patchset 10:
            Hiroshige Hayashizaki . resolved

            Could you mention somewhere in the commit message that this CL removes the following non-trivial `CrossThreadCopier`s that were just used to call `WTF::String`'s `CrossThreadCopier` transitively, and thus were no longer needed (even before this CL) since `WTF::String` became thread-safe?
            (Am I understanding correctly?)

            • third_party/blink/renderer/core/css/css_syntax_definition.h
            • third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.h
            • third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.h
            Daniel Cheng

            There were quite a few, e.g. many of the `CrossThreadCopierPassthrough`s were really just copies. I will mention it generically but hopefully in a way that makes it clear this is not exhaustive.

            File third_party/blink/renderer/core/workers/custom_event_message.h
            Line 18, Patchset 5: scoped_refptr<blink::SerializedScriptValue> message;
            Andrey Kosyakov . resolved

            drive-by: looks like this may be destroyed on a thread different from its creation thread (both before and after this change), and `~SerializedScriptValue()` is not entirely thread safe, at least when it comes to [non-production code](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=12455;drc=db569cd7847d479358bd391a18b2c28393803333). The prod code looks thread-safe though, at least at a quick glance.

            Daniel Cheng

            Acknowledged

            File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
            Line 50, Patchset 10: CrossThreadBindOnce(&ModuleScriptFetcher::Client::OnFetched, it.key,
            Hiroshige Hayashizaki . resolved

            Historically, this pattern of
            `PostCrossThreadTask(CrossThreadBindOnce(params->IsolatedCopy()))`
            was unsafe, if e.g. (#) the IsolatedCopy()ed result is non-movable and contains (non-thread-safe) RefCounted, because the temporary object remains on the main thread after the task is posted, causing race conditions between the temporary object and the posted object. This sounds quite rare but actually had been a major source of crashes (around 2014-2016?), because `WTF::String` and all objects contains `WTF::String` satisfied the condition (#). This was the primary motivation to call `CrossThreadCopier` inside `CrossThreadBindOnce`, not at its call sites.

            However, now the concern is gone, because IIUC there are no current cases that satisfies the condition (#) and that were previously covered by `CrossThreadCopier` (and thus should be touched by this CL).

            • `WTF::String` is now thread-safe!
            • The `IsolatedCopy()` here is the only remaining non-boilerplate/non-CHECK-only `CrossThreadCopier`. And `ModuleScriptCreationParams` is move-only and doesn't contain non-thread-safe `RefCounted` objects, so doesn't satisfies the condition (#).

            So I feel the change of this CL itself should be thread-safe.

            The remaining concern is about the future changes -- I was a bit afraid of losing this context (that the `PostCrossThreadTask(CrossThreadBindOnce(params->IsolatedCopy()))` pattern is still theoretically unsafe), but this is now more like a theoretical concern:

            • The risk of this particular pattern is not much higher than other thread-unsafe cases.
            • The fact that now there is only one code location (here) that needs non-trivial `IsolatedCopy()` (+ we use move semantics everywhere) suggests that it's also unlikely we'll add a new such cases hitting the condition (#).

            so probably it's just fine as-is.

            Daniel Cheng

            My personal feeling is `ModuleScriptCreationParams` should not contain `Persistent` at all; it should actually be garbage-collected. And we should have a separate `CrossThreadModuleScriptCreationParams` that is always thread-safe. But I will experiment with this in a followup.

            Line 51, Patchset 5: params->IsolatedCopy()));
            Daniel Cheng . resolved

            If we don't call `IsolatedCopy()` here, it won't build.

            Technically you can still do an unsafe thing here though if you call `std::move(*this)`...

            Andrey Kosyakov

            Looks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.

            Daniel Cheng

            Yes, sorry–to be clear, calling `std::move(*this)` would be a bug, and I am highlighting this as a thing that we would no longer be able to catch.

            Daniel Cheng

            Acknowledged

            File third_party/blink/renderer/platform/wtf/cross_thread_functional.h
            Line 37, Patchset 10:// std::move(f).Run(42); // Calls MyFunction(<deep copy of `obj`>, 42);
            Hiroshige Hayashizaki . resolved

            This comment becomes obsolete (after this CL, CrossThreadBindOnce doesn't create a deep copy).

            Daniel Cheng

            Done

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Jeremy Roman
            Submit Requirements:
            • requirement satisfiedCode-Coverage
            • requirement satisfiedCode-Owners
            • requirement satisfiedCode-Review
            • requirement is not satisfiedNo-Unresolved-Comments
            • requirement satisfiedReview-Enforcement
            Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
            Gerrit-MessageType: comment
            Gerrit-Project: chromium/src
            Gerrit-Branch: main
            Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
            Gerrit-Change-Number: 6685109
            Gerrit-PatchSet: 13
            Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
            Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
            Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
            Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
            Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
            Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
            Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
            Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
            Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
            Gerrit-CC: Hongchan Choi <hong...@chromium.org>
            Gerrit-CC: Kentaro Hara <har...@chromium.org>
            Gerrit-CC: Menard, Alexis <alexis...@intel.com>
            Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
            Gerrit-CC: Nate Chapin <jap...@chromium.org>
            Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
            Gerrit-CC: Stephen Chenney <sche...@chromium.org>
            Gerrit-CC: Xida Chen <xida...@chromium.org>
            Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
            Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Comment-Date: Wed, 26 Nov 2025 08:19:50 +0000
            Gerrit-HasComments: Yes
            Gerrit-Has-Labels: No
            Comment-In-Reply-To: Hiroshige Hayashizaki <hiro...@chromium.org>
            satisfied_requirement
            unsatisfied_requirement
            open
            diffy

            Daniel Cheng (Gerrit)

            unread,
            Nov 26, 2025, 3:20:18 AM (12 days ago) Nov 26
            to Daniel Cheng, Michael Lippautz, Hiroshige Hayashizaki, Andrey Kosyakov, Hiroki Nakagawa, Kentaro Hara, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
            Attention needed from Jeremy Roman

            Daniel Cheng added 1 comment

            Patchset-level comments
            File-level comment, Patchset 5:
            Daniel Cheng . resolved

            @mlip...@chromium.org and @jbr...@chromium.org for the overall review.

            tl;dr I really don't think the costs of `CrossThreadCopier` justify its existence anymore. It's true that Oilpan types are still thread-hostile, but the issue has always been transitive violations, which the current traits don't really do much for anyway.

            I have toyed with the idea of having some marker for thread-hostile things, and then having the clang plugin derive thread-safety transitively and enforce it for things that care (like `CrossThreadBind...`)–basically, the Rust style of "compiler derives `Send` for you where it can" but I don't think figuring that out should be blocking for this cleanup either. Please let me know you feel differently.

            @ca...@chromium.org, @fma...@chromium.org I left some implementation-specific questions below–PTAL at those if you can.

            Daniel Cheng

            Acknowledged

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Jeremy Roman
            Submit Requirements:
              • requirement satisfiedCode-Coverage
              • requirement satisfiedCode-Owners
              • requirement satisfiedCode-Review
              • requirement satisfiedReview-Enforcement
              Gerrit-Comment-Date: Wed, 26 Nov 2025 08:20:00 +0000
              Gerrit-HasComments: Yes
              Gerrit-Has-Labels: No
              Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
              satisfied_requirement
              open
              diffy

              Daniel Cheng (Gerrit)

              unread,
              Nov 26, 2025, 12:10:09 PM (12 days ago) Nov 26
              to Daniel Cheng, Michael Lippautz, Hiroshige Hayashizaki, Andrey Kosyakov, Hiroki Nakagawa, Kentaro Hara, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com
              Attention needed from Jeremy Roman

              Daniel Cheng voted Commit-Queue+2

              Commit-Queue+2
              Open in Gerrit

              Related details

              Attention is currently required from:
              • Jeremy Roman
              Submit Requirements:
              • requirement satisfiedCode-Coverage
              • requirement satisfiedCode-Owners
              • requirement satisfiedCode-Review
              • requirement satisfiedReview-Enforcement
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: comment
              Gerrit-Project: chromium/src
              Gerrit-Branch: main
              Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
              Gerrit-Change-Number: 6685109
              Gerrit-PatchSet: 14
              Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
              Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
              Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
              Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
              Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
              Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
              Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
              Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
              Gerrit-CC: Hongchan Choi <hong...@chromium.org>
              Gerrit-CC: Kentaro Hara <har...@chromium.org>
              Gerrit-CC: Menard, Alexis <alexis...@intel.com>
              Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
              Gerrit-CC: Nate Chapin <jap...@chromium.org>
              Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
              Gerrit-CC: Stephen Chenney <sche...@chromium.org>
              Gerrit-CC: Xida Chen <xida...@chromium.org>
              Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
              Gerrit-Attention: Jeremy Roman <jbr...@chromium.org>
              Gerrit-Comment-Date: Wed, 26 Nov 2025 17:09:54 +0000
              Gerrit-HasComments: No
              Gerrit-Has-Labels: Yes
              satisfied_requirement
              open
              diffy

              Chromium LUCI CQ (Gerrit)

              unread,
              Nov 26, 2025, 12:40:28 PM (12 days ago) Nov 26
              to Daniel Cheng, Michael Lippautz, Hiroshige Hayashizaki, Andrey Kosyakov, Hiroki Nakagawa, Kentaro Hara, Jeremy Roman, Florin Malita, srirama chandra sekhar, Xida Chen, devtools...@chromium.org, Hongchan Choi, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Hirokazu Honda, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, blink-re...@chromium.org, eric.c...@apple.com, ipc-securi...@chromium.org, blink-revi...@chromium.org, erickun...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, devtools-re...@chromium.org, dom+...@chromium.org, drott+bl...@chromium.org, emircan+watch...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mcasas+med...@chromium.org, oilpan-rev...@chromium.org, shimazu...@chromium.org, tommyw+w...@chromium.org, video-networking...@google.com

              Chromium LUCI CQ submitted the change with unreviewed changes

              Unreviewed changes

              10 is the latest approved patch-set.
              The change was submitted with unreviewed changes in the following files:

              ```
              The name of the file: third_party/blink/renderer/platform/wtf/cross_thread_functional.h
              Insertions: 8, Deletions: 3.

              The diff is too large to show. Please review the diff.
              ```

              Change information

              Commit message:
              Delete CrossThreadCopier

              This was previously needed because Blink strings were thread-hostile.
              Blink provided `CrossThreadBindOnce()` and `CrossThreadBindRepeating()`
              wrappers around the //base equivalents for creating callbacks;
              internally, these helpers used `CrossThreadCopier` to ensure that the
              callback had uniqely-owned copy of thread-hostile arguments, so the
              ownership could be safely transferred to another thread.

              Since https://crrev.com/1057879, Blink strings are thread-safe, and with
              few exceptions noted below, the `CrossThreadCopier` specializations are
              almost 100% boilerplate.

              - `scoped_refptr<T>` contains a `static_assert()` that `T` derives from
              `base::RefCountedThreadSafe`. This is definitely nice to have, and the
              `static_assert()` itself has been moved into the implementation of
              `CrossThreadBindOnce()` and `CrossThreadBindRepeating()`–with the
              caveat that it will not catch transitive violations of this rule.
              - `ModuleScriptCreationParams` contains Oilpan `Persistent<T>` and is
              not safe to directly copy across threads. However, the type itself is
              not copyable–and the only way to create a copy to pass across threads
              is to call `IsolatedCopy()`. Note: `ModuleScriptCreationParams` itself
              should probably be garbage collected, with a separate, distinct type
              for passing across threads, similar to `CrossThreadSourceLocation`.
              - The specialization for `SkBitmap` asserts the bitmap is null or
              immutable; while this is nice, modern code should be using `SkImage`,
              which is immutable.
              - The specialization for `sk_sp` asserts that the type uses a specific
              type of Skia refcounting, but this is overly strict.

              While the `SkBitmap` property is nice to check, requiring hundreds of
              lines of boilerplate specializations for that does not seem like a good
              tradeoff. `CrossThreadCopier` protects against some misuse of Oilpan
              objects across thread boundaries; however, `CrossThreadBindOnce()` and
              `CrossThreadBindRepeating()` already `static_assert()` that no
              thread-hostile Oilpan types are directly bound–and neither approach is
              good at catching transitive violations.

              In addition, `CrossThreadCopier` invites potential ODR violations–many
              `CrossThreadCopier` specializations are defined in random .cc files,
              with no real coordination.

              Overall, the burdens imposed by the `CrossThreadCopier` abstraction does
              not really justify its benefit, so just remove it–which also improves
              binary size by a decent amount :) This also removes a number of
              CrossThreadCopier specializations (e.g. for ContentSecurityPolicyPtr,
              ResourceTimingInfoPtr, and ServerTimingInfoPtr) which were
              manually-written copy constructors.
              Bug: 460743390
              Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
              Reviewed-by: Michael Lippautz <mlip...@chromium.org>
              Commit-Queue: Daniel Cheng <dch...@chromium.org>
              Reviewed-by: Hiroshige Hayashizaki <hiro...@chromium.org>
              Reviewed-by: Andrey Kosyakov <ca...@chromium.org>
              Cr-Commit-Position: refs/heads/main@{#1550583}
              Files:
              • M third_party/blink/public/mojom/timing/resource_timing.mojom
              • M third_party/blink/renderer/bindings/core/v8/script_decoder.cc
              • M third_party/blink/renderer/controller/javascript_call_stack_collector.cc
              • M third_party/blink/renderer/controller/performance_manager/renderer_resource_coordinator_impl.cc
              • M third_party/blink/renderer/controller/performance_manager/v8_worker_memory_reporter.cc
              • M third_party/blink/renderer/core/css/css_syntax_definition.cc
              • M third_party/blink/renderer/core/css/css_syntax_definition.h
              • M third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
              • M third_party/blink/renderer/core/inspector/devtools_agent.cc
              • M third_party/blink/renderer/core/inspector/thread_debugger_common_impl.h
              • M third_party/blink/renderer/core/loader/build.gni
              • D third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.cc
              • M third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.h
              • M third_party/blink/renderer/core/loader/modulescript/module_script_creation_params.h
              • M third_party/blink/renderer/core/loader/resource/font_resource.cc
              • M third_party/blink/renderer/core/messaging/blink_transferable_message.h
              • M third_party/blink/renderer/core/typed_arrays/array_buffer/array_buffer_contents.h
              • M third_party/blink/renderer/core/workers/build.gni
              • D third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.cc
              • M third_party/blink/renderer/core/workers/cross_thread_global_scope_creation_params_copier.h
              • M third_party/blink/renderer/core/workers/custom_event_message.h
              • M third_party/blink/renderer/core/workers/worker_backing_thread_startup_data.h
              • M third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
              • M third_party/blink/renderer/modules/breakout_box/media_stream_audio_track_underlying_sink_test.cc
              • M third_party/blink/renderer/modules/imagecapture/image_capture_frame_grabber.cc
              • M third_party/blink/renderer/modules/mediarecorder/audio_track_recorder.cc
              • M third_party/blink/renderer/modules/mediarecorder/video_track_recorder.cc
              • M third_party/blink/renderer/modules/mediasource/cross_thread_media_source_attachment.cc
              • M third_party/blink/renderer/modules/mediastream/track_audio_renderer.cc
              • M third_party/blink/renderer/modules/mediastream/web_media_player_ms.cc
              • M third_party/blink/renderer/modules/peerconnection/BUILD.gn
              • D third_party/blink/renderer/modules/peerconnection/adapters/web_rtc_cross_thread_copier.cc
              • M third_party/blink/renderer/modules/peerconnection/adapters/web_rtc_cross_thread_copier.h
              • M third_party/blink/renderer/modules/peerconnection/peer_connection_dependency_factory.cc
              • M third_party/blink/renderer/modules/peerconnection/peer_connection_tracker.cc
              • M third_party/blink/renderer/modules/peerconnection/rtc_data_channel.cc
              • M third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
              • M third_party/blink/renderer/modules/peerconnection/rtc_rtp_sender_impl.cc
              • M third_party/blink/renderer/modules/peerconnection/webrtc_set_description_observer.h
              • M third_party/blink/renderer/modules/webaudio/audio_worklet_thread_test.cc
              • M third_party/blink/renderer/modules/webcodecs/audio_decoder_broker.cc
              • M third_party/blink/renderer/modules/webcodecs/background_readback.cc
              • M third_party/blink/renderer/modules/webcodecs/video_decoder_broker.cc
              • M third_party/blink/renderer/modules/webcodecs/video_encoder.cc
              • M third_party/blink/renderer/modules/webcodecs/video_frame.cc
              • M third_party/blink/renderer/modules/webrtc/webrtc_audio_renderer.cc
              • M third_party/blink/renderer/platform/bindings/cross_thread_copier.h
              • M third_party/blink/renderer/platform/bindings/cross_thread_source_location.h
              • M third_party/blink/renderer/platform/bindings/source_location_copier.h
              • M third_party/blink/renderer/platform/fonts/font.h
              • M third_party/blink/renderer/platform/graphics/paint/paint_record.h
              • M third_party/blink/renderer/platform/graphics/resource_id_traits.h
              • M third_party/blink/renderer/platform/graphics/skia/skia_utils.h
              • M third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h
              • M third_party/blink/renderer/platform/heap/cross_thread_persistent.h
              • M third_party/blink/renderer/platform/heap/test/cross_thread_handle_test.cc
              • M third_party/blink/renderer/platform/loader/fetch/url_loader/background_url_loader.cc
              • M third_party/blink/renderer/platform/loader/fetch/url_loader/background_url_loader_unittest.cc
              • M third_party/blink/renderer/platform/loader/fetch/url_loader/resource_request_sender.cc
              • M third_party/blink/renderer/platform/media/web_media_player_impl.cc
              • M third_party/blink/renderer/platform/network/network_state_notifier.h
              • M third_party/blink/renderer/platform/peerconnection/rtc_scoped_refptr_cross_thread_copier.h
              • M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc
              • M third_party/blink/renderer/platform/peerconnection/rtc_video_encoder.cc
              • M third_party/blink/renderer/platform/peerconnection/webrtc_audio_sink.cc
              • M third_party/blink/renderer/platform/peerconnection/webrtc_audio_sink.h
              • M third_party/blink/renderer/platform/weborigin/kurl.h
              • M third_party/blink/renderer/platform/webrtc/convert_to_webrtc_video_frame_buffer.cc
              • M third_party/blink/renderer/platform/wtf/BUILD.gn
              • D third_party/blink/renderer/platform/wtf/cross_thread_copier.cc
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_base.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_gfx.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_gpu.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_media.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_mojo.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_public.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_skia.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_std.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_copier_url.h
              • M third_party/blink/renderer/platform/wtf/cross_thread_functional.h
              • M third_party/blink/renderer/platform/wtf/functional.h
              Change size: XL
              Delta: 82 files changed, 37 insertions(+), 1799 deletions(-)
              Branch: refs/heads/main
              Submit Requirements:
              • requirement satisfiedCode-Review: +1 by Hiroshige Hayashizaki, +1 by Andrey Kosyakov, +1 by Michael Lippautz
              Open in Gerrit
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: merged
              Gerrit-Project: chromium/src
              Gerrit-Branch: main
              Gerrit-Change-Id: Id598e48b3a1b9f2f1e28f5458b94ee5995a8e48d
              Gerrit-Change-Number: 6685109
              Gerrit-PatchSet: 15
              Gerrit-Owner: Daniel Cheng <dch...@chromium.org>
              Gerrit-Reviewer: Andrey Kosyakov <ca...@chromium.org>
              Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
              Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
              Gerrit-Reviewer: Florin Malita <fma...@chromium.org>
              Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
              Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              open
              diffy
              satisfied_requirement
              Reply all
              Reply to author
              Forward
              0 new messages