Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal [chromium/src : main]
Omer Katz (Gerrit)
Attention is currently required from: Kentaro Hara, Michael Lippautz.
Omer Katz would like Kentaro Hara and Michael Lippautz to review this change.
Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal
Bug: chromium:1246423
Change-Id: I8d7897546e3bc66122b5799575e09a3172a5b84b
---
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.h
M third_party/blink/renderer/platform/heap/weak_cell.h
3 files changed, 44 insertions(+), 21 deletions(-)
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Kentaro Hara, Michael Lippautz.
1 comment:
Patchset:
ptal
Using WeakPtrFactory with GCed types is generally unsafe and should be replaced with the GC-safe WeakCellFactory.
+mlippautz for weak_cell.h
This use case should ideally use CrossThreadHandle, but CrossThreadHandle is missing some traits to allow it to be used with `AsyncCall` and `Then`.
I prefer to land this fix with CrossThreadPersistent for now to unblock plugin checks and replace it later with CrossThreadHandle.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Michael Lippautz, Omer Katz.
Patch set 1:Code-Review +1
2 comments:
Patchset:
LGTM
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.h:
Patch Set #1, Line 168:
base::WeakPtrFactory<ImageDecoderExternal> decode_weak_factory_{this};Why was this working? Was it a bug?
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Kentaro Hara, Michael Lippautz, Omer Katz.
5 comments:
Patchset:
Please update the comments before submitting. I haven't checked closely, but presumably WeakCell now enforces all the constraints this class was abiding by.
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.h:
Patch Set #1, Line 168:
base::WeakPtrFactory<ImageDecoderExternal> decode_weak_factory_{this};Why was this working? Was it a bug?
See all the comments inside of the class (which also need updating). The code is very careful to HasPendingActivity() remains true while outstanding tasks are pending.
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc:
Patch Set #1, Line 286:
// See OnContextDestroyed(); WeakPtrs must be invalidated ahead of GC.Fix or remove comment now?
Patch Set #1, Line 475:
// WeakPtrs need special consideration when used with a garbage collectedFix/remove comment.
Patch Set #1, Line 486:
// WARNING: All pending WeakPtr bindings must be tracked here. I.e., allFix / remove comment.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Michael Lippautz, Omer Katz.
Patch set 1:-Code-Review
The change is no longer submittable: Code-Owners and Code-Review are unsatisfied now.
1 comment:
Patchset:
Would you help me understand why we need to explicitly invalidate the weak pointers? Why is it not okay to wait for a GC to clear the weak pointers?
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Omer Katz.
2 comments:
Patchset:
This is not fixing an immediate issue, is it? Can we add the traits right away since you are already looking at this code?
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.h:
Patch Set #1, Line 168:
base::WeakPtrFactory<ImageDecoderExternal> decode_weak_factory_{this};
See all the comments inside of the class (which also need updating). […]
HPA() is ignored when the ExecutionContext is destroyed. Is this working as intended?
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Omer Katz.
Omer Katz uploaded patch set #2 to this change.
Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal
Bug: chromium:1246423
Change-Id: I8d7897546e3bc66122b5799575e09a3172a5b84b
---
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.h
M third_party/blink/renderer/platform/heap/weak_cell.h
3 files changed, 46 insertions(+), 33 deletions(-)
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Michael Lippautz.
7 comments:
Patchset:
Would you help me understand why we need to explicitly invalidate the weak pointers? Why is it not o […]
That is a valid question, but I can't answer it. I'm just trying to replace a known risky WeakPtrFactory with a safe WeakCellFactory to eliminate the risk.
Perhaps someone more familiar with this area can pick this up as a followup to rethink whether invalidation can be avoided.This is not fixing an immediate issue, is it? Can we add the traits right away since you are already […]
I tried that originally and it wasn't a 5 minute detour. I'll take another look at it today.
Please update the comments before submitting. […]
Acknowledged
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.h:
Patch Set #1, Line 168:
base::WeakPtrFactory<ImageDecoderExternal> decode_weak_factory_{this};
HPA() is ignored when the ExecutionContext is destroyed. […]
Kentaro were you asking about why HPA was working or why WeakPtrFactory was working?
WeakPtrFactory is generally unsafe for GCed types, but 1) it would be very rare for it actually crash, and 2) I assume the rest of this class was crafted to mitigate the risk as much as possible (thus making bugs even more rare).
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc:
Patch Set #1, Line 286:
// See OnContextDestroyed(); WeakPtrs must be invalidated ahead of GC.Fix or remove comment now?
Done
Patch Set #1, Line 475:
// WeakPtrs need special consideration when used with a garbage collectedFix/remove comment.
Done
Patch Set #1, Line 486:
// WARNING: All pending WeakPtr bindings must be tracked here. I.e., allFix / remove comment.
Done
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Dale Curtis, Michael Lippautz, Omer Katz.
1 comment:
Patchset:
In theory, base::WeakPtrFactory should not be needed for Oilpan types, and we removed them from Blink when we introduced Oilpan. I don't object to landing this CL and revisiting it later but I think we should get there 😊
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Michael Lippautz, Omer Katz.
Omer Katz uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Omer Katz
Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal
Bug: chromium:1246423
Change-Id: I8d7897546e3bc66122b5799575e09a3172a5b84b
---
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.h
M third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h
M third_party/blink/renderer/platform/heap/weak_cell.h
4 files changed, 53 insertions(+), 40 deletions(-)
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Michael Lippautz.
2 comments:
Patchset:
I tried that originally and it wasn't a 5 minute detour. I'll take another look at it today.
Done
Patchset:
In theory, base::WeakPtrFactory should not be needed for Oilpan types, and we removed them from Blin […]
I suppose people had a usecase that required invalidating pointers to live objects, but I can't say for sure. If we want to re-evaluate WeakPtrFactory here, we should file an issue against the owners of this code so they can investigate this further.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Dale Curtis, Michael Lippautz, Omer Katz.
2 comments:
Patchset:
I suppose people had a usecase that required invalidating pointers to live objects, but I can't say […]
But here's a question: Do we really need the WeakCell infra? When we introduced Oilpan, we intentionally didn't introduce it and instead removed WeakPtrFactory.
I'm okay with introducing if there's a use case but want to understand the reasoning.
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.h:
Patch Set #1, Line 168:
base::WeakPtrFactory<ImageDecoderExternal> decode_weak_factory_{this};
Kentaro were you asking about why HPA was working or why WeakPtrFactory was working? […]
I was asking why WeakPtrFactory was working. Now I understand the situation, thanks!
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Michael Lippautz.
2 comments:
Patchset:
But here's a question: Do we really need the WeakCell infra? When we introduced Oilpan, we intention […]
I see. However we are not introducing it now. It already exists and this CL is simply extending the usage.
See https://mail.google.com/mail/u/0/?source=sync&tf=1&view=pt&th=18a8f7c31fbbd2d7&search=all&msg and crbug.com/1485318 for the discussion that led to its introduction (the bug mentions an issue with delayed invalidation due to delayed finalization in Oilpan).If eventually invalidation is no longer needed and all usages are removed, we could also get rid of `WeakCell`.
I think the question of whether invalidation is actually needed/desired with Oilpan is a valid question, but also orthogonal to this CL.
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.h:
Patch Set #1, Line 168:
base::WeakPtrFactory<ImageDecoderExternal> decode_weak_factory_{this};
I was asking why WeakPtrFactory was working. […]
Acknowledged
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Dale Curtis, Michael Lippautz, Omer Katz.
Patch set 3:Code-Review +1
1 comment:
Patchset:
ok, LGTM. Please file a bug and ask the owner to see if we can remove WeakCellFactory.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
ok, LGTM. Please file a bug and ask the owner to see if we can remove WeakCellFactory.
Done. crbug.com/1504805
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Patchset:
ping mlippautz
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Dale Curtis, Omer Katz.
1 comment:
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
Why is that moved here?
Are we missing on converting a `CrossThreadHandle` to an `UnwrappingCrossThreadHandle`?
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Michael Lippautz.
1 comment:
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
Why is that moved here? […]
Yes.
When used with `AsyncCal().Then()` (like in `image_decoder_external.cc`) the off-thread callback that we execute never sees the `CrossThreadHandle`. My understanding is that `CrossThreadHandle` assumed it would be passed to the callback so that the callback can explicitly call `MakeUnwrappingCrossThreadHandle`. However, in this case, the `AsyncCall` infra keeps the `CrossThreadHandle` hidden and only tries to unwrap it once when triggering the `Then` callback.
I didn't think that adding specialization in `AsyncCall().Then()` for `CrossThreadHandle` was a viable solution here.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Dale Curtis, Daniel Cheng, Omer Katz.
Omer Katz has uploaded this change for review.
Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal
Bug: chromium:1246423
Change-Id: I8d7897546e3bc66122b5799575e09a3172a5b84b
---
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.h
M third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h
M third_party/blink/renderer/platform/heap/weak_cell.h
4 files changed, 53 insertions(+), 40 deletions(-)
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Dale Curtis, Daniel Cheng, Omer Katz.
Yes. […]
Can you start with passing an unwrapping handle right away?
If we soften the restriction around non-wrapping handle then we may as well remove it.
I remember that we wanted the difference initially but it came up repeatedly as a burden so far. @dch...@chromium.org for input.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Daniel Cheng, Kentaro Hara, Omer Katz.
Omer Katz uploaded patch set #4 to this change.
The following approvals got outdated and were removed: Code-Review+1 by Kentaro Hara
The change is no longer submittable: Code-Review is unsatisfied now.
Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal
Bug: chromium:1246423
Change-Id: I8d7897546e3bc66122b5799575e09a3172a5b84b
---
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.h
M third_party/blink/renderer/platform/heap/weak_cell.h
3 files changed, 45 insertions(+), 32 deletions(-)
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Daniel Cheng, Kentaro Hara, Michael Lippautz.
1 comment:
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
Can you start with passing an unwrapping handle right away? […]
Done.
Personally, since `GetOnCreationThread` checks that we're on the right thread, I think it would be just as safe to merge CrossThreadHandle and UnwrappingCrossThreadHandle.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Dale Curtis, Daniel Cheng, Kentaro Hara, Omer Katz.
1 comment:
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
Done. […]
Yeah, by now I think that the benefits of differentiating here are not in relation to the code cruft added. Let's wait for Daniel.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Daniel Cheng, Kentaro Hara, Michael Lippautz, Omer Katz.
3 comments:
Patchset:
I see. However we are not introducing it now. […]
This class cancels what it can (AbortFlag) and ignores the rest (WeakPtr) after certain operations given the high memory / cpu cost of decoding operations.
A mostly equivalent approach would be a pair of sequence counters which are checked on operation return, but there may also be some lifetime issues that I don't recall properly. I left a similar comment at https://bugs.chromium.org/p/chromium/issues/detail?id=1504805#c1
This is a pretty common non-Blink pattern, so I like the idea of WeakCell, but if it's not something the Blink team wants, we can try to replace the usage and see if anything explodes.
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.h:
Patch Set #1, Line 168:
base::WeakPtrFactory<ImageDecoderExternal> decode_weak_factory_{this};
Acknowledged
Re: Michael's HPA() question: ContextDestroyed() invalidates the WeakPointers and prevents any new ones from being generated.
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc:
Patch Set #4, Line 153:
// If the context is already destroyed we will never get an OnContextDestroyedI don't think this comment is as important anymore? You could say `ImageDecoder requires an active context to operate correctly.` or just delete the comment entirely now I think.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Michael Lippautz, Omer Katz.
1 comment:
Patchset:
But here's a question: Do we really need the WeakCell infra? When we introduced Oilpan, we intentionally didn't introduce it and instead removed WeakPtrFactory.
> I'm okay with introducing if there's a use case but want to understand the reasoning.
I think it's relatively rare to need invalidation, but there are some cases where it's useful, and people have been getting around it by just using WeakPtrFactory (often unsafely). So it would be better to ban + have a safe pattern.
If that safe pattern is "WeakPtrFactory must be paired with a pre finalizer", that'd work for me too. But it doesn't seem like it'd be simpler/easier to reason about, at least from this media code.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Michael Lippautz, Omer Katz.
Omer Katz uploaded patch set #5 to this change.
Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal
Bug: chromium:1246423
Change-Id: I8d7897546e3bc66122b5799575e09a3172a5b84b
---
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.h
M third_party/blink/renderer/platform/heap/weak_cell.h
3 files changed, 44 insertions(+), 33 deletions(-)
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Michael Lippautz.
1 comment:
File third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc:
Patch Set #4, Line 153:
// If the context is already destroyed we will never get an OnContextDestroyed
I don't think this comment is as important anymore? You could say `ImageDecoder requires an active c […]
Done
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara, Michael Lippautz, Omer Katz.
1 comment:
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
Yeah, by now I think that the benefits of differentiating here are not in relation to the code cruft […]
Are you OK with MakeCrossThreadHandle() automatically unwrapping if it's used as the receiver pointer? I think that's the one context that there's clearly an intent to unwrap it for calls. We'd leave all other behaviour as-is, right?
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Dale Curtis, Daniel Cheng, Kentaro Hara, Omer Katz.
1 comment:
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
Are you OK with MakeCrossThreadHandle() automatically unwrapping if it's used as the receiver pointe […]
How would we do that automatically? Idea generally sounds good.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Kentaro Hara, Michael Lippautz.
1 comment:
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
How would we do that automatically? Idea generally sounds good.
We can specialize `UnderlyingReceiverType`, but it would be even better if we could just make it work with `std::to_address`: https://source.chromium.org/chromium/chromium/src/+/main:base/functional/bind_internal.h;l=1275;drc=3b01151f2a0692863c0415e18a1e9c1f0e7a38c5
Either way, something for a followup probably.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Daniel Cheng, Kentaro Hara, Omer Katz.
Patch set 5:Code-Review +1
2 comments:
Patchset:
lgtm
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
We can specialize `UnderlyingReceiverType`, but it would be even better if we could just make it wor […]
Thanks.
Omer, can you file a follow up bug. (Doesn't need to be fixed right away.)
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Daniel Cheng, Kentaro Hara.
1 comment:
Patchset:
Kentaro, please +1 again
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
File third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h:
Thanks. […]
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Daniel Cheng, Omer Katz.
To view, visit change 5049885. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Daniel Cheng.
Patch set 5:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Replace WeakPtrFactory with WeakCellFactory in ImageDecoderExternal
Bug: chromium:1246423
Change-Id: I8d7897546e3bc66122b5799575e09a3172a5b84b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5049885
Commit-Queue: Omer Katz <omer...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1229836}
---
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.cc
M third_party/blink/renderer/modules/webcodecs/image_decoder_external.h
M third_party/blink/renderer/platform/heap/weak_cell.h
3 files changed, 44 insertions(+), 33 deletions(-)