@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.
params->IsolatedCopy()));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)`...
: public CrossThreadCopierPassThrough<base::span<uint8_t>> {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.
// GarbageCollected heap but it doesn't actually have to.@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.
CHECK(bitmap.isImmutable() || bitmap.isNull())@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)
"sk_sp<T> can be passed across threads only if T is SkRefCnt.");@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".
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
bindings/ and inspector/ lgtm
CSSSyntaxDefinition IsolatedCopy() const;Looks like you've just deleted the only call site, so this can go as well.
scoped_refptr<blink::SerializedScriptValue> message;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.
params->IsolatedCopy()));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)`...
Looks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.
// TODO(dcheng): Unclear if this is even needed.It's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).
// GarbageCollected heap but it doesn't actually have to.@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.
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.
struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>drive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?
Please nuke the entire file.
This is intentional, to avoid dealing with IWYU errors. Copied the TODO here.
Please nuke the entire file.
This one I missed though. Done now.
Daniel ChengNuke the entire file?
Intentionally kept for now but copied the TODO here as well.
Daniel Chengditto.
Good call, done.
params->IsolatedCopy()));Andrey KosyakovIf 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)`...
Looks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.
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.
// TODO(dcheng): Unclear if this is even needed.It's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).
I'm going to have to think about how to make this CL safe in light of this.
// GarbageCollected heap but it doesn't actually have to.Andrey Kosyakov@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.
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.
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.
struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>drive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?
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
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
CHECK(bitmap.isImmutable() || bitmap.isNull())@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)
New code should use `SkImage`s instead, which are intrinsically immutable. Removing sgtm.
"sk_sp<T> can be passed across threads only if T is SkRefCnt.");@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".
Agreed, I don't think it's worth keeping this machinery around just for an assert.
(it also arbitrarily excludes `SkNVRefCnt`-derived types)
// TODO(dcheng): Unclear if this is even needed.Daniel ChengIt's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).
I'm going to have to think about how to make this CL safe in light of this.
OK, the parent CL should address this. `CrossThreadSourceLocation` is now always inherently safe to move across threads.
CHECK(bitmap.isImmutable() || bitmap.isNull())Florin Malita@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)
New code should use `SkImage`s instead, which are intrinsically immutable. Removing sgtm.
Acknowledged
"sk_sp<T> can be passed across threads only if T is SkRefCnt.");Florin Malita@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".
Agreed, I don't think it's worth keeping this machinery around just for an assert.
(it also arbitrarily excludes `SkNVRefCnt`-derived types)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Looks like you've just deleted the only call site, so this can go as well.
Oops, fixed this too now.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
// TODO(dcheng): This one might have some value, though it's not entirely clearRemove this following the removal of class it was referring to?
struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>Daniel Chengdrive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?
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
If no one has objections, I'm planning on landing this tomorrow.
// TODO(dcheng): This one might have some value, though it's not entirely clearRemove this following the removal of class it was referring to?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
immutable.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.
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?)
CrossThreadBindOnce(&ModuleScriptFetcher::Client::OnFetched, it.key,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).
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:
so probably it's just fine as-is.
// std::move(f).Run(42); // Calls MyFunction(<deep copy of `obj`>, 42);This comment becomes obsolete (after this CL, CrossThreadBindOnce doesn't create a deep copy).
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
lgtm, having struggled with the copied and ODR myself I see this as a net win
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.
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.
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
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.
scoped_refptr<blink::SerializedScriptValue> message;Daniel Chengdrive-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.
Acknowledged
CrossThreadBindOnce(&ModuleScriptFetcher::Client::OnFetched, it.key,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.
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.
params->IsolatedCopy()));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)`...
Daniel ChengLooks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.
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.
Acknowledged
// std::move(f).Run(42); // Calls MyFunction(<deep copy of `obj`>, 42);This comment becomes obsolete (after this CL, CrossThreadBindOnce doesn't create a deep copy).
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
@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.
Acknowledged
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
```
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.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |