WebNN: Use raw_ref for WebNNContextImpl in WebNNTensorImpl [chromium/src : main]
Bernhart, Bryan (Gerrit)
Bernhart, Bryan added 1 comment![]( "Open in Gerrit")
Related details
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
: public WebNNObjectImpl<mojom::WebNNTensor,Can this be changed to `WebNNObjectBase` so that this class no longer uses reference counting?
Otherwise this patch is depending too much on the promises made by comments and the MiraclePtr checks for correctness.
Related details
- Bernhart, Bryan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bernhart, Bryan (Gerrit)
Bernhart, Bryan added 1 comment![]( "Open in Gerrit")
: public WebNNObjectImpl<mojom::WebNNTensor,Can this be changed to `WebNNObjectBase` so that this class no longer uses reference counting?
Otherwise this patch is depending too much on the promises made by comments and the MiraclePtr checks for correctness.
`WebNNTensorImpl` must remain ref-counted because it is passed into `gpu::Scheduler` tasks. These tasks need to extend the lifetime of the tensor until execution completes on the GPU sequence.
Moving `WebNNObjectBase` to unique ownership would force the scheduler to use `base::Unretained` or raw pointers, which is significantly more dangerous than `scoped_refptr`.
Switching to `raw_ref` moves us away from "promises made by comments" because it makes the relationship a compiler-enforced invariant. Currently, using `WeakPtr` allows for potential undefined behavior if a null-check is missed, whereas `raw_ref` would always `CHECK`.
We can continue to use `AsWeakPtr()` for asynchronous tasks (TFLite & CoreML backends) while using the `raw_ref` for synchronous logic where the context is guaranteed to be alive (DML & ORT backends). WDYT?
Related details
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 2 comments![]( "Open in Gerrit")
: public WebNNObjectImpl<mojom::WebNNTensor,Bernhart, BryanCan this be changed to `WebNNObjectBase` so that this class no longer uses reference counting?
Otherwise this patch is depending too much on the promises made by comments and the MiraclePtr checks for correctness.
`WebNNTensorImpl` must remain ref-counted because it is passed into `gpu::Scheduler` tasks. These tasks need to extend the lifetime of the tensor until execution completes on the GPU sequence.
Moving `WebNNObjectBase` to unique ownership would force the scheduler to use `base::Unretained` or raw pointers, which is significantly more dangerous than `scoped_refptr`.
Switching to `raw_ref` moves us away from "promises made by comments" because it makes the relationship a compiler-enforced invariant. Currently, using `WeakPtr` allows for potential undefined behavior if a null-check is missed, whereas `raw_ref` would always `CHECK`.
We can continue to use `AsWeakPtr()` for asynchronous tasks (TFLite & CoreML backends) while using the `raw_ref` for synchronous logic where the context is guaranteed to be alive (DML & ORT backends). WDYT?
Ack. Let me just suggest some comment updates to better explain what's happening here. Note that this is not compiler-enforced. As mentioned in the suggested comment above this adds a runtime check to debug builds and if the invariant is broken in a production build MiraclePtr only protects against the resulting use-after-free being exploitable.
// GPU process implementation of the MLTensor interface. A WebNNTensorImpl is
// always associated with a WebNNContextImpl and is guaranteed to be outlived by
// that context.```suggestion
// GPU process implementation of the `MLTensor` interface. While this class is
// reference-counted a `WebNNTensorImpl` is guaranteed not to outlive the
// `WebNNContextImpl` that created it because references are only held by tasks
// posted to the `gpu::SchedulerTaskRunner` that is shut down when the context is
// destroyed.
//
// This invariant is checked by the `raw_ref<WebNNContextImpl>` member, which will
// trigger dangling pointer warnings in debug builds and safe crashes in release
// builds.
```
Related details
- Bernhart, Bryan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
: public WebNNObjectImpl<mojom::WebNNTensor,Bernhart, BryanCan this be changed to `WebNNObjectBase` so that this class no longer uses reference counting?
Otherwise this patch is depending too much on the promises made by comments and the MiraclePtr checks for correctness.
Reilly Grant`WebNNTensorImpl` must remain ref-counted because it is passed into `gpu::Scheduler` tasks. These tasks need to extend the lifetime of the tensor until execution completes on the GPU sequence.
Moving `WebNNObjectBase` to unique ownership would force the scheduler to use `base::Unretained` or raw pointers, which is significantly more dangerous than `scoped_refptr`.
Switching to `raw_ref` moves us away from "promises made by comments" because it makes the relationship a compiler-enforced invariant. Currently, using `WeakPtr` allows for potential undefined behavior if a null-check is missed, whereas `raw_ref` would always `CHECK`.
We can continue to use `AsWeakPtr()` for asynchronous tasks (TFLite & CoreML backends) while using the `raw_ref` for synchronous logic where the context is guaranteed to be alive (DML & ORT backends). WDYT?
Ack. Let me just suggest some comment updates to better explain what's happening here. Note that this is not compiler-enforced. As mentioned in the suggested comment above this adds a runtime check to debug builds and if the invariant is broken in a production build MiraclePtr only protects against the resulting use-after-free being exploitable.
CC @aj...@chromium.org to confirm my understanding and sign off on this from a Security perspective.
Related details
- Bernhart, Bryan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hu, Ningxin (Gerrit)
Hu, Ningxin added 1 comment![]( "Open in Gerrit")
// GPU process implementation of the MLTensor interface. A WebNNTensorImpl is
// always associated with a WebNNContextImpl and is guaranteed to be outlived by
// that context.```suggestion
// GPU process implementation of the `MLTensor` interface. While this class is
// reference-counted a `WebNNTensorImpl` is guaranteed not to outlive the
// `WebNNContextImpl` that created it because references are only held by tasks
// posted to the `gpu::SchedulerTaskRunner` that is shut down when the context is
// destroyed.
//
// This invariant is checked by the `raw_ref<WebNNContextImpl>` member, which will
// trigger dangling pointer warnings in debug builds and safe crashes in release
// builds.
```
// a `WebNNTensorImpl` is guaranteed not to outlive the
// `WebNNContextImpl` that created it because references are only held by tasks
// posted to the `gpu::SchedulerTaskRunner` that is shut down when the context is
// destroyed.
Is it guaranteed for backends which still use `ResourceTask`? including CoreML and TFlite backends.
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
// GPU process implementation of the MLTensor interface. A WebNNTensorImpl is
// always associated with a WebNNContextImpl and is guaranteed to be outlived by
// that context.Hu, Ningxin```suggestion
// GPU process implementation of the `MLTensor` interface. While this class is
// reference-counted a `WebNNTensorImpl` is guaranteed not to outlive the
// `WebNNContextImpl` that created it because references are only held by tasks
// posted to the `gpu::SchedulerTaskRunner` that is shut down when the context is
// destroyed.
//
// This invariant is checked by the `raw_ref<WebNNContextImpl>` member, which will
// trigger dangling pointer warnings in debug builds and safe crashes in release
// builds.
```
// a `WebNNTensorImpl` is guaranteed not to outlive the
// `WebNNContextImpl` that created it because references are only held by tasks
// posted to the `gpu::SchedulerTaskRunner` that is shut down when the context is
// destroyed.Is it guaranteed for backends which still use `ResourceTask`? including CoreML and TFlite backends.
Yes, the `ResourceTask` bit has its own referencing counting that doesn't depend on the lifetime of the `WebNNTensorImpl`.