Fix dangling pointer to IDBRequestQueueItem in IDBRequestLoader [chromium/src : main]
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Requesting reviews from Evan as the owner of the files and Paul as the author of the earlier CL that made IDBRequestLoader garbage-collected. Thank you!
Related details
- Evan Stade
- Paul Semel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Semel (Gerrit)
Paul Semel voted![]( "Open in Gerrit")
- Abhishek Shanthkumar
- Evan Stade
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 3 comments![]( "Open in Gerrit")
thanks for the fix!
base::WeakPtr<IDBRequestQueueItem> queue_item,I'm not a huge fan of making this a `WeakPtr` because it makes it harder to understand lifetimes. You have to do a good deal of sleuthing to understand the object relationships. Since it is possible (and easy), I think it is better if the queue item explicitly calls into the request loader to let it know the former is going away.
Alternatively (and preferably, although it is more work), sever the dependency from IDBRequestLoader to IDBRequestQueueItem and instead inject (a) a single callback taking the place of OnResultLoadComplete and (b) an ExecutionContext.
queue_item_->OnResultLoadComplete();Was this or any other access to `queue_item_` actually occurring after `queue_item_` was freed, or is the check just a matter of the raw_ptr itself outliving the object it referenced?
I am guessing the latter (in part because otherwise we'd have seen crash reports), in which case I think these added checks are misleading and undesirable, because they should never fail.
Related details
- Abhishek Shanthkumar
- Paul Semel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar added 2 comments![]( "Open in Gerrit")
base::WeakPtr<IDBRequestQueueItem> queue_item,I'm not a huge fan of making this a `WeakPtr` because it makes it harder to understand lifetimes. You have to do a good deal of sleuthing to understand the object relationships. Since it is possible (and easy), I think it is better if the queue item explicitly calls into the request loader to let it know the former is going away.
Alternatively (and preferably, although it is more work), sever the dependency from IDBRequestLoader to IDBRequestQueueItem and instead inject (a) a single callback taking the place of OnResultLoadComplete and (b) an ExecutionContext.
Thanks for the inputs! I agree that the other two approaches are better. I tried the first approach but it didn’t seem future-proof and there are several places where we need to notify the loader. Let me try the second approach if we’re OK with a bit of churn. Do we bind to an unretained reference to the queue item when creating the callback though? This ties in to the other comment below - I didn’t find any crash reports in this code so I’m assuming there is no incorrect access happening currently.
queue_item_->OnResultLoadComplete();Was this or any other access to `queue_item_` actually occurring after `queue_item_` was freed, or is the check just a matter of the raw_ptr itself outliving the object it referenced?
I am guessing the latter (in part because otherwise we'd have seen crash reports), in which case I think these added checks are misleading and undesirable, because they should never fail.
Touched upon this in the other comment above - I too believe it’s the latter. I can replace the conditional checks with CHECK statements if we decide to proceed with the weak ptr approach.
Related details
- Abhishek Shanthkumar
- Evan Stade
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 2 comments![]( "Open in Gerrit")
base::WeakPtr<IDBRequestQueueItem> queue_item,Abhishek ShanthkumarI'm not a huge fan of making this a `WeakPtr` because it makes it harder to understand lifetimes. You have to do a good deal of sleuthing to understand the object relationships. Since it is possible (and easy), I think it is better if the queue item explicitly calls into the request loader to let it know the former is going away.
Alternatively (and preferably, although it is more work), sever the dependency from IDBRequestLoader to IDBRequestQueueItem and instead inject (a) a single callback taking the place of OnResultLoadComplete and (b) an ExecutionContext.
Thanks for the inputs! I agree that the other two approaches are better. I tried the first approach but it didn’t seem future-proof and there are several places where we need to notify the loader. Let me try the second approach if we’re OK with a bit of churn. Do we bind to an unretained reference to the queue item when creating the callback though? This ties in to the other comment below - I didn’t find any crash reports in this code so I’m assuming there is no incorrect access happening currently.
there are several places where we need to notify the loader
It doesn't just need to happen from the dtor?
Let me try the second approach if we’re OK with a bit of churn
Thanks!
Do we bind to an unretained reference to the queue item when creating the callback though?
Good question --- in this case I would use a WeakPtr but add a comment to the effect that we don't expect it to be called after the weak pointer is invalidated, and it's just there over Unretained for future-proofing. The problem with Unretained is that I don't think it will be obvious if it turns out the assumption is not true (i.e. there's no guarantee of an easy to understand stack trace or anything), and using a perhaps-unnecessary WeakPtr is not that as misleading if there's an explanatory comment there.
queue_item_->OnResultLoadComplete();Abhishek ShanthkumarWas this or any other access to `queue_item_` actually occurring after `queue_item_` was freed, or is the check just a matter of the raw_ptr itself outliving the object it referenced?
I am guessing the latter (in part because otherwise we'd have seen crash reports), in which case I think these added checks are misleading and undesirable, because they should never fail.
Touched upon this in the other comment above - I too believe it’s the latter. I can replace the conditional checks with CHECK statements if we decide to proceed with the weak ptr approach.
I don't think the CHECK is even necessary since it's cooked into WeakPtr.
Related details
- Abhishek Shanthkumar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar removed a vote from this change![]( "Open in Gerrit")
Removed Auto-Submit+1 by Abhishek Shanthkumar <abhishek.s...@microsoft.com>
Related details
- Abhishek Shanthkumar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Commit-Queue | +1 |
3 comments
Updated the fix to a better one and resolved comments.
Abhishek ShanthkumarI'm not a huge fan of making this a `WeakPtr` because it makes it harder to understand lifetimes. You have to do a good deal of sleuthing to understand the object relationships. Since it is possible (and easy), I think it is better if the queue item explicitly calls into the request loader to let it know the former is going away.
Alternatively (and preferably, although it is more work), sever the dependency from IDBRequestLoader to IDBRequestQueueItem and instead inject (a) a single callback taking the place of OnResultLoadComplete and (b) an ExecutionContext.
Evan StadeThanks for the inputs! I agree that the other two approaches are better. I tried the first approach but it didn’t seem future-proof and there are several places where we need to notify the loader. Let me try the second approach if we’re OK with a bit of churn. Do we bind to an unretained reference to the queue item when creating the callback though? This ties in to the other comment below - I didn’t find any crash reports in this code so I’m assuming there is no incorrect access happening currently.
there are several places where we need to notify the loader
It doesn't just need to happen from the dtor?
Let me try the second approach if we’re OK with a bit of churn
Thanks!
Do we bind to an unretained reference to the queue item when creating the callback though?Good question --- in this case I would use a WeakPtr but add a comment to the effect that we don't expect it to be called after the weak pointer is invalidated, and it's just there over Unretained for future-proofing. The problem with Unretained is that I don't think it will be obvious if it turns out the assumption is not true (i.e. there's no guarantee of an easy to understand stack trace or anything), and using a perhaps-unnecessary WeakPtr is not that as misleading if there's an explanatory comment there.
It doesn't just need to happen from the dtor?
IDBRequestQueueItem explicitly clears the IDBRequestLoader in a couple of places. This notification does not reach the loader when it's a GC class, so I thought it should be notified explicitly in these cases too for correctness, though it's not strictly required.
Let me try the second approach if we’re OK with a bit of churn
I tried this and ran into a few more tricky cases. ExecutionContext is a GC object, so it seems wrong to retain a reference to it as a Member in IDBRequestLoader since we don't want to actually keep it alive but just check for its liveness before proceeding. The alternative is to expect another callback in IDBRequestLoader which provides the ExecutionContext, which seemed like 1 too many callbacks...
Hence, I dug a bit more and figured that IDBRequestLoader itself does not need to be a GC object. The parts of it that use FileReaderLoader (which is GC-ed) are well-defined and those parts need to be GC-ed. Hence, I kept only the logic to process items in `values_` one after the other in IDBRequestLoader and made it an off-heap object like before, and moved the use of FileReaderLoader to a private class which is GC-ed. Now, the notifications about resetting IDBRequestLoader will reach it correctly like before, and it seems easier to reason about the lifetimes of the various objects involved here:
IDBRequestLoader is one per IDBRequestQueueItem and tied to it; it spins out garbage-collected SingleValueLoaders one after the other to process each value in `values_`. At any point, if the processing is cancelled, the notification is bubbled up to the current SingleValueLoader. SingleValueLoader does not maintain a strong reference to any object that may get deleted by the time its processing is finished - it notifies IDBRequestLoader through a weak pointer.
This should make it easier to implement parallel blob processing too in the future per the comment in IDBRequestLoader::Start(). Let me know your thoughts!
Abhishek ShanthkumarWas this or any other access to `queue_item_` actually occurring after `queue_item_` was freed, or is the check just a matter of the raw_ptr itself outliving the object it referenced?
I am guessing the latter (in part because otherwise we'd have seen crash reports), in which case I think these added checks are misleading and undesirable, because they should never fail.
Evan StadeTouched upon this in the other comment above - I too believe it’s the latter. I can replace the conditional checks with CHECK statements if we decide to proceed with the weak ptr approach.
I don't think the CHECK is even necessary since it's cooked into WeakPtr.
The check does not apply now since queue_item_ is a strong raw pointer.
Related details
- Evan Stade
- Paul Semel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 4 comments![]( "Open in Gerrit")
const raw_ref<Vector<std::unique_ptr<IDBValue>>> values_;this was probably also a problem...
raw_ptr<IDBRequestQueueItem> queue_item_;should this also be a `raw_ref`? It can't be null
private:
LoadCompleteCallback load_complete_callback_;
base::OnceClosure load_failed_callback_;
optional: for simplicity, combine these. A null argument value can indicate failure.
base::WeakPtr<IDBRequestQueueItem> queue_item,Abhishek ShanthkumarI'm not a huge fan of making this a `WeakPtr` because it makes it harder to understand lifetimes. You have to do a good deal of sleuthing to understand the object relationships. Since it is possible (and easy), I think it is better if the queue item explicitly calls into the request loader to let it know the former is going away.
Alternatively (and preferably, although it is more work), sever the dependency from IDBRequestLoader to IDBRequestQueueItem and instead inject (a) a single callback taking the place of OnResultLoadComplete and (b) an ExecutionContext.
Evan StadeThanks for the inputs! I agree that the other two approaches are better. I tried the first approach but it didn’t seem future-proof and there are several places where we need to notify the loader. Let me try the second approach if we’re OK with a bit of churn. Do we bind to an unretained reference to the queue item when creating the callback though? This ties in to the other comment below - I didn’t find any crash reports in this code so I’m assuming there is no incorrect access happening currently.
Abhishek Shanthkumarthere are several places where we need to notify the loader
It doesn't just need to happen from the dtor?
Let me try the second approach if we’re OK with a bit of churn
Thanks!
Do we bind to an unretained reference to the queue item when creating the callback though?Good question --- in this case I would use a WeakPtr but add a comment to the effect that we don't expect it to be called after the weak pointer is invalidated, and it's just there over Unretained for future-proofing. The problem with Unretained is that I don't think it will be obvious if it turns out the assumption is not true (i.e. there's no guarantee of an easy to understand stack trace or anything), and using a perhaps-unnecessary WeakPtr is not that as misleading if there's an explanatory comment there.
It doesn't just need to happen from the dtor?
IDBRequestQueueItem explicitly clears the IDBRequestLoader in a couple of places. This notification does not reach the loader when it's a GC class, so I thought it should be notified explicitly in these cases too for correctness, though it's not strictly required.
Let me try the second approach if we’re OK with a bit of churn
I tried this and ran into a few more tricky cases. ExecutionContext is a GC object, so it seems wrong to retain a reference to it as a Member in IDBRequestLoader since we don't want to actually keep it alive but just check for its liveness before proceeding. The alternative is to expect another callback in IDBRequestLoader which provides the ExecutionContext, which seemed like 1 too many callbacks...
Hence, I dug a bit more and figured that IDBRequestLoader itself does not need to be a GC object. The parts of it that use FileReaderLoader (which is GC-ed) are well-defined and those parts need to be GC-ed. Hence, I kept only the logic to process items in `values_` one after the other in IDBRequestLoader and made it an off-heap object like before, and moved the use of FileReaderLoader to a private class which is GC-ed. Now, the notifications about resetting IDBRequestLoader will reach it correctly like before, and it seems easier to reason about the lifetimes of the various objects involved here:
IDBRequestLoader is one per IDBRequestQueueItem and tied to it; it spins out garbage-collected SingleValueLoaders one after the other to process each value in `values_`. At any point, if the processing is cancelled, the notification is bubbled up to the current SingleValueLoader. SingleValueLoader does not maintain a strong reference to any object that may get deleted by the time its processing is finished - it notifies IDBRequestLoader through a weak pointer.
This should make it easier to implement parallel blob processing too in the future per the comment in IDBRequestLoader::Start(). Let me know your thoughts!
It doesn't just need to happen from the dtor?
IDBRequestQueueItem explicitly clears the IDBRequestLoader in a couple of places. This notification does not reach the loader when it's a GC class, so I thought it should be notified explicitly in these cases too for correctness, though it's not strictly required.
Let me try the second approach if we’re OK with a bit of churn
I tried this and ran into a few more tricky cases. ExecutionContext is a GC object, so it seems wrong to retain a reference to it as a Member in IDBRequestLoader since we don't want to actually keep it alive but just check for its liveness before proceeding. The alternative is to expect another callback in IDBRequestLoader which provides the ExecutionContext, which seemed like 1 too many callbacks...
Wouldn't that be the purpose of `WeakMember`? But what I actually had in mind was making `IDBRequestLoader` an `ExecutionContextLifecycleObserver` (which stores the `ExecutionContext` as a `WeakMember`)
I agree 3 callbacks is a little much and starts to suggest declaring an interface, but it's really only one callback since the ExecutionContext is a member and the other two callbacks can be combined.
The collateral advantage of doing the above would be to remove the cyclical dependency between IDBRequestLoader and IDBRequestQueueItem, which doesn't have any immediate practical benefit but it is generally better for things to be less tightly coupled.
Hence, I dug a bit more and figured that IDBRequestLoader itself does not need to be a GC object. The parts of it that use FileReaderLoader (which is GC-ed) are well-defined and those parts need to be GC-ed. Hence, I kept only the logic to process items in `values_` one after the other in IDBRequestLoader and made it an off-heap object like before, and moved the use of FileReaderLoader to a private class which is GC-ed. Now, the notifications about resetting IDBRequestLoader will reach it correctly like before, and it seems easier to reason about the lifetimes of the various objects involved here:
IDBRequestLoader is one per IDBRequestQueueItem and tied to it; it spins out garbage-collected SingleValueLoaders one after the other to process each value in `values_`. At any point, if the processing is cancelled, the notification is bubbled up to the current SingleValueLoader. SingleValueLoader does not maintain a strong reference to any object that may get deleted by the time its processing is finished - it notifies IDBRequestLoader through a weak pointer.
I do like that the GC ugliness is confined to implementation details of `IDBRequestLoader`! And thank you for proofing this out so it's possible to see it concretely and have a more grounded discussion about this approach.
What I don't love is that this adds yet another layer of indirection which makes it just that much harder to follow what's going on. `IDBRequestLoader` is already a helper that encapsulates functionality that could have been part of `IDBRequestQueueItem` (somewhat poorly imho, given the cyclical dependency with `IDBRequestQueueItem`...) and to add another wrapper class to deal with the GC/non-GC boundary seems unfortunate. I feel like this extreme coupling between the two classes (they're essentially sharing a member variable in `values_`?!) is the core issue here.
This should make it easier to implement parallel blob processing too in the future per the comment in IDBRequestLoader::Start().
This does seem like a practical advantage if we ever get around to dealing with that comment. But for now that is an uncertain *if*.
Related details
- Abhishek Shanthkumar
- Paul Semel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar added 1 comment![]( "Open in Gerrit")
Wouldn't that be the purpose of `WeakMember`? But what I actually had in mind was making `IDBRequestLoader` an `ExecutionContextLifecycleObserver` (which stores the `ExecutionContext` as a `WeakMember`)
I agree 3 callbacks is a little much and starts to suggest declaring an interface, but it's really only one callback since the ExecutionContext is a member and the other two callbacks can be combined.
The collateral advantage of doing the above would be to remove the cyclical dependency between IDBRequestLoader and IDBRequestQueueItem, which doesn't have any immediate practical benefit but it is generally better for things to be less tightly coupled.
Right, got it...
What I don't love is that this adds yet another layer of indirection which makes it just that much harder to follow what's going on. `IDBRequestLoader` is already a helper that encapsulates functionality that could have been part of `IDBRequestQueueItem` (somewhat poorly imho, given the cyclical dependency with `IDBRequestQueueItem`...) and to add another wrapper class to deal with the GC/non-GC boundary seems unfortunate. I feel like this extreme coupling between the two classes (they're essentially sharing a member variable in `values_`?!) is the core issue here.
Yes, agreed... What if we move the `values_` processing bits from `IDBRequestLoader` to `IDBRequestQueueItem` itself, and make `IDBRequestLoader` a GC class responsible for processing just one value (basically make it `SingleValueLoader` from Patchset 3)? This will address the coupling through the shared `values_` that exists currently. In the current patchset, `IDBRequestLoader` outside of `SingleValueLoader` has no significant logic encapsulated within it, and exposes just two public methods to start and cancel the queue processing.
Related details
- Evan Stade
- Paul Semel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted and added 8 comments![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Commit-Queue | +1 |
8 comments
Resolved comments and added notes about the changes in Patchset 4.
// if the context is destroyed. |values| must be kept alive until either the
// loader calls |load_complete_callback| or the load is canceled.Without this relaxation (compared to the earlier stricter but unnecessary requirement of `values` outliving this class through the use of a `raw_ref`), the tests still hit a dangling pointer check because the reference to `values` becomes dangling once the `IDBRequestQueueItem` processes the callback and gets destructed.
should this also be a `raw_ref`? It can't be null
NA to Patchset 4.
private:
LoadCompleteCallback load_complete_callback_;
base::OnceClosure load_failed_callback_;
optional: for simplicity, combine these. A null argument value can indicate failure.
Used a single callback in Patchset 4.
base::WeakPtr<IDBRequestQueueItem> queue_item,Agreed about achieving the overall aim of decoupling the two classes; Patchset 4 takes this approach.
`ExecutionContext` destruction is currently already handled by `IDBRequest` - it invokes `IDBRequestQueueItem::CancelLoading()` on context destruction, which calls `IDBRequestLoader::Cancel()`. If we make `IDBRequestLoader` an `ExecutionContextLifecycleObserver` and cancel the load ourself on context destruction, it will lead to a double cancelation, which hits a DCHECK. We can either require callers of `IDBRequestLoader` to cancel the request on execution context destruction, or only handle it ourself and remove the cancellation invocation from `IDBRequestQueueItem`. Patchset 4 takes the former approach. The latter approach is trickier since `IDBRequestQueueItem::CancelLoading()` is also called from `IDBRequest::Abort()`, in which case we do need to cancel the loader.
Yes, agreed... What if we move the `values_` processing bits from `IDBRequestLoader` to `IDBRequestQueueItem` itself, and make `IDBRequestLoader` a GC class responsible for processing just one value (basically make it `SingleValueLoader` from Patchset 3)? This will address the coupling through the shared `values_` that exists currently. In the current patchset, `IDBRequestLoader` outside of `SingleValueLoader` has no significant logic encapsulated within it, and exposes just two public methods to start and cancel the queue processing.
At second glance, this does not seem like a great approach since it reduces the utility of `IDBRequestLoader`.
// The execution context was torn down. The loader will eventually get a
// Cancel() call.
if (!execution_context_) {
return;
}@est...@chromium.org, can/should we move this to the beginning of the function rather than checking it after iterating over `values_`?
DCHECK(!ready_);This is from the existing error-handling version of `OnResultLoadComplete()`. I guess we can remove it since there is already a strong `CHECK` below, @est...@chromium.org?
request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);@est...@chromium.org, the test `IDBRequestTest.ErrorWithQueuedLoadingResult` that calls this method still fails under the dangling ptr check because this call bypasses the `IDBRequestLoader` and calls the completion callback on `IDBRequestQueueItem` directly, thus violating the loader's requirement of `values` persisting till it invokes the callback. What are your thoughts on the best way to mitigate this? Ideally, we would wait for the unwrapping to complete from the loader. Other options are to cancel the request here, or to use non-wrapped values in the test, but I'm not sure about the correctness of these approaches.
Related details
- Evan Stade
- Paul Semel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 4 comments![]( "Open in Gerrit")
WeakMember<ExecutionContext> execution_context_;I think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]
// if the context is destroyed. |values| must be kept alive until either the
// loader calls |load_complete_callback| or the load is canceled.Without this relaxation (compared to the earlier stricter but unnecessary requirement of `values` outliving this class through the use of a `raw_ref`), the tests still hit a dangling pointer check because the reference to `values` becomes dangling once the `IDBRequestQueueItem` processes the callback and gets destructed.
I don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.
// The execution context was torn down. The loader will eventually get a
// Cancel() call.
if (!execution_context_) {
return;
}@est...@chromium.org, can/should we move this to the beginning of the function rather than checking it after iterating over `values_`?
If this were being written from scratch, I would probably say yes, but since it already exists, I'd leave it alone in case changing it breaks some crazy edge case we can't think of.
DCHECK(!ready_);This is from the existing error-handling version of `OnResultLoadComplete()`. I guess we can remove it since there is already a strong `CHECK` below, @est...@chromium.org?
- Abhishek Shanthkumar
- Paul Semel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar added 3 comments![]( "Open in Gerrit")
WeakMember<ExecutionContext> execution_context_;I think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]
I added a reply about this on the earlier comment; rewording it below.
I assume we want `IDBRequestLoader` to listen to context destruction so that it can cancel the load when the context goes away. `IDBRequest` - which supplies the `ExecutionContext` transitively to `IDBRequestLoader` and which we would pass to the constructor of `ExecutionContextLifecycleObserver` - is an `ExecutionContextLifecycleObserver` itself and invokes `IDBRequestQueueItem::CancelLoading()` from `ContextDestroyed()`, which calls `IDBRequestLoader::Cancel()`.
As things stand currently, if `IDBRequestLoader` too calls `Cancel()` in `ContextDestroyed()`, it would hit a `DCHECK` for double-cancellation.
Should we remove the cancellation call from `IDBRequest` through to `IDBRequestLoader` in this case and let `IDBRequestLoader` alone handle it? The cancellation message still needs to reach `IDBRequestQueueItem` though, and another scenario for cancellation is when `IDBRequest::Abort()` is called, and in this case, we would want `IDBRequestQueueItem` to cancel the load on `IDBRequestLoader`.
// if the context is destroyed. |values| must be kept alive until either the
// loader calls |load_complete_callback| or the load is canceled.Evan StadeWithout this relaxation (compared to the earlier stricter but unnecessary requirement of `values` outliving this class through the use of a `raw_ref`), the tests still hit a dangling pointer check because the reference to `values` becomes dangling once the `IDBRequestQueueItem` processes the callback and gets destructed.
I don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.
IIUC, this parameter contains the wrapped values that are read and unwrapped by `IDBRequestLoader` (and written back in the same positions in the vector), so it needs to be passed to this class. Is that not right?
One option is to expect callers to std::move the values here to the constructor, and `IDBRequestLoader` can std::move them back to the caller in the load complete callback (and as the return value of the `Cancel()` call, since the callback is not invoked when the load is canceled). What do you think?
request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);@est...@chromium.org, the test `IDBRequestTest.ErrorWithQueuedLoadingResult` that calls this method still fails under the dangling ptr check because this call bypasses the `IDBRequestLoader` and calls the completion callback on `IDBRequestQueueItem` directly, thus violating the loader's requirement of `values` persisting till it invokes the callback. What are your thoughts on the best way to mitigate this? Ideally, we would wait for the unwrapping to complete from the loader. Other options are to cancel the request here, or to use non-wrapped values in the test, but I'm not sure about the correctness of these approaches.
@est...@chromium.org, do let me know your thoughts here. Is there a way to wait for the load to complete "organically" through the loader itself instead of short-circuiting it on `IDBRequestQueueItem`?
Related details
- Evan Stade
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 3 comments![]( "Open in Gerrit")
WeakMember<ExecutionContext> execution_context_;Abhishek ShanthkumarI think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]
I added a reply about this on the earlier comment; rewording it below.
I assume we want `IDBRequestLoader` to listen to context destruction so that it can cancel the load when the context goes away. `IDBRequest` - which supplies the `ExecutionContext` transitively to `IDBRequestLoader` and which we would pass to the constructor of `ExecutionContextLifecycleObserver` - is an `ExecutionContextLifecycleObserver` itself and invokes `IDBRequestQueueItem::CancelLoading()` from `ContextDestroyed()`, which calls `IDBRequestLoader::Cancel()`.
As things stand currently, if `IDBRequestLoader` too calls `Cancel()` in `ContextDestroyed()`, it would hit a `DCHECK` for double-cancellation.
Should we remove the cancellation call from `IDBRequest` through to `IDBRequestLoader` in this case and let `IDBRequestLoader` alone handle it? The cancellation message still needs to reach `IDBRequestQueueItem` though, and another scenario for cancellation is when `IDBRequest::Abort()` is called, and in this case, we would want `IDBRequestQueueItem` to cancel the load on `IDBRequestLoader`.
sorry for missing it earlier.
I guess based on what you've stated, it's not actually a problem that the `WeakMember<ExecutionContext> execution_context_` could be non-null after the context is destroyed, because it won't actually be accessed after destruction because `Cancel()` is called. But that is very subtle! I think we should at least set it it to null in `Cancel()`, although even that is subtle in that we're relying on code in some other file to call `Cancel()` when the context is destroyed. It's easier for the next reader to come along and verify correctness if `this` is an `ExecutionContextLifecycleObserver`.
As things stand currently [...]
I feel pretty meh about those DCHECKs. Could easily turn them into early returns. Seems like they were probably added by the original author in order to be more sure the code was working as intended at the time, and not because calling `Cancel()` twice would lead to some disastrous consequence.
I've leave it to you which approach you prefer to go with.
// if the context is destroyed. |values| must be kept alive until either the
// loader calls |load_complete_callback| or the load is canceled.Evan StadeWithout this relaxation (compared to the earlier stricter but unnecessary requirement of `values` outliving this class through the use of a `raw_ref`), the tests still hit a dangling pointer check because the reference to `values` becomes dangling once the `IDBRequestQueueItem` processes the callback and gets destructed.
Abhishek ShanthkumarI don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.
IIUC, this parameter contains the wrapped values that are read and unwrapped by `IDBRequestLoader` (and written back in the same positions in the vector), so it needs to be passed to this class. Is that not right?
One option is to expect callers to std::move the values here to the constructor, and `IDBRequestLoader` can std::move them back to the caller in the load complete callback (and as the return value of the `Cancel()` call, since the callback is not invoked when the load is canceled). What do you think?
OK, I guess I didn't look closely enough to see that it was an in-out param of sorts. Yes, moving them in and then out sounds more ideal.
request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);Abhishek Shanthkumar@est...@chromium.org, the test `IDBRequestTest.ErrorWithQueuedLoadingResult` that calls this method still fails under the dangling ptr check because this call bypasses the `IDBRequestLoader` and calls the completion callback on `IDBRequestQueueItem` directly, thus violating the loader's requirement of `values` persisting till it invokes the callback. What are your thoughts on the best way to mitigate this? Ideally, we would wait for the unwrapping to complete from the loader. Other options are to cancel the request here, or to use non-wrapped values in the test, but I'm not sure about the correctness of these approaches.
@est...@chromium.org, do let me know your thoughts here. Is there a way to wait for the load to complete "organically" through the loader itself instead of short-circuiting it on `IDBRequestQueueItem`?
I hope cleaning up the ownership of `IDBRequestLoader::values_` fixes this.
Related details
- Abhishek Shanthkumar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
There is also an interpretation of the tightly coupled relationship between IDBRequestLoader and IDBRequestQueueItem that concludes these two classes should actually just be one class. It's not the case that either one of these classes is particularly long or byzantine, so fusing them would not create a monolith. And I don't really see why it would be difficult to convert `IDBRequestQueueItem` to being GC'd*.
*famous last words uttered by someone who hasn't actually completed this task yet...
Related details
- Abhishek Shanthkumar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted and added 5 comments![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Commit-Queue | +1 |
There is also an interpretation of the tightly coupled relationship between IDBRequestLoader and IDBRequestQueueItem that concludes these two classes should actually just be one class. It's not the case that either one of these classes is particularly long or byzantine, so fusing them would not create a monolith. And I don't really see why it would be difficult to convert `IDBRequestQueueItem` to being GC'd*.
*famous last words uttered by someone who hasn't actually completed this task yet...
Haha yes... 😊 Maybe worth trying in the next CL!
Resolved comments; the CL is ready for review.
Abhishek ShanthkumarI think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]
Evan StadeI added a reply about this on the earlier comment; rewording it below.
I assume we want `IDBRequestLoader` to listen to context destruction so that it can cancel the load when the context goes away. `IDBRequest` - which supplies the `ExecutionContext` transitively to `IDBRequestLoader` and which we would pass to the constructor of `ExecutionContextLifecycleObserver` - is an `ExecutionContextLifecycleObserver` itself and invokes `IDBRequestQueueItem::CancelLoading()` from `ContextDestroyed()`, which calls `IDBRequestLoader::Cancel()`.
As things stand currently, if `IDBRequestLoader` too calls `Cancel()` in `ContextDestroyed()`, it would hit a `DCHECK` for double-cancellation.
Should we remove the cancellation call from `IDBRequest` through to `IDBRequestLoader` in this case and let `IDBRequestLoader` alone handle it? The cancellation message still needs to reach `IDBRequestQueueItem` though, and another scenario for cancellation is when `IDBRequest::Abort()` is called, and in this case, we would want `IDBRequestQueueItem` to cancel the load on `IDBRequestLoader`.
sorry for missing it earlier.
I guess based on what you've stated, it's not actually a problem that the `WeakMember<ExecutionContext> execution_context_` could be non-null after the context is destroyed, because it won't actually be accessed after destruction because `Cancel()` is called. But that is very subtle! I think we should at least set it it to null in `Cancel()`, although even that is subtle in that we're relying on code in some other file to call `Cancel()` when the context is destroyed. It's easier for the next reader to come along and verify correctness if `this` is an `ExecutionContextLifecycleObserver`.
As things stand currently [...]
I feel pretty meh about those DCHECKs. Could easily turn them into early returns. Seems like they were probably added by the original author in order to be more sure the code was working as intended at the time, and not because calling `Cancel()` twice would lead to some disastrous consequence.
I've leave it to you which approach you prefer to go with.
Looking at it more closely, `IDBRequestLoader` does not need to get immediately notified when the `ExecutionContext` is destroyed. It uses the context only when spinning up an instance of `FileReaderLoader` in `StartNextValue()`, and there is already a check for liveness of `ExecutionContext` there. I updated it to also check `IsContextDestroyed()` explicitly (and added code comments) in Patchset 6.
`WeakMember` resets the pointer to null when the context is GC-ed, so we don't need to worry about a UAF in any case.
From the perspective of `IDBRequestLoader`, it is not strictly necessary for a `Cancel()` call to come through when the context is destroyed. Calling `Cancel()` in this case is a "business" decision that must be made by the consumer of `IDBRequestLoader`, in which case `IDBRequestLoader` will return the `values_` and call it done. If the caller does not care about the values when the context is destroyed, then it can not call `Cancel()` at all, and the "right thing" will still happen (`IDBRequestLoader` stops unwrapping more values). This seems fine to me, but do share your thoughts!
// if the context is destroyed. |values| must be kept alive until either the
// loader calls |load_complete_callback| or the load is canceled.Evan StadeWithout this relaxation (compared to the earlier stricter but unnecessary requirement of `values` outliving this class through the use of a `raw_ref`), the tests still hit a dangling pointer check because the reference to `values` becomes dangling once the `IDBRequestQueueItem` processes the callback and gets destructed.
Abhishek ShanthkumarI don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.
Evan StadeIIUC, this parameter contains the wrapped values that are read and unwrapped by `IDBRequestLoader` (and written back in the same positions in the vector), so it needs to be passed to this class. Is that not right?
One option is to expect callers to std::move the values here to the constructor, and `IDBRequestLoader` can std::move them back to the caller in the load complete callback (and as the return value of the `Cancel()` call, since the callback is not invoked when the load is canceled). What do you think?
OK, I guess I didn't look closely enough to see that it was an in-out param of sorts. Yes, moving them in and then out sounds more ideal.
Done
request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);Abhishek Shanthkumar@est...@chromium.org, the test `IDBRequestTest.ErrorWithQueuedLoadingResult` that calls this method still fails under the dangling ptr check because this call bypasses the `IDBRequestLoader` and calls the completion callback on `IDBRequestQueueItem` directly, thus violating the loader's requirement of `values` persisting till it invokes the callback. What are your thoughts on the best way to mitigate this? Ideally, we would wait for the unwrapping to complete from the loader. Other options are to cancel the request here, or to use non-wrapped values in the test, but I'm not sure about the correctness of these approaches.
Evan Stade@est...@chromium.org, do let me know your thoughts here. Is there a way to wait for the load to complete "organically" through the loader itself instead of short-circuiting it on `IDBRequestQueueItem`?
I hope cleaning up the ownership of `IDBRequestLoader::values_` fixes this.
Yes... Kind of. In Patchset 6, I made this a private function and I'm passing fake unwrapped values in this call. The `IDBRequestLoader` still holds the original copy that will be cleaned up at the end of the test.
Related details
- Evan Stade
- Paul Semel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade voted and added 4 comments![]( "Open in Gerrit")
Votes added by Evan Stade
| Code-Review | +1 |
4 comments
This CL fixes the issue by removing the coupling between IDBRequestLoader and IDBRequestQueueItem through the use of WeakPtr-backed callbacks.meganit: line wrap and "a WeakPtr-backed callback"
WeakMember<ExecutionContext> execution_context_;Abhishek ShanthkumarI think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]
Evan StadeI added a reply about this on the earlier comment; rewording it below.
I assume we want `IDBRequestLoader` to listen to context destruction so that it can cancel the load when the context goes away. `IDBRequest` - which supplies the `ExecutionContext` transitively to `IDBRequestLoader` and which we would pass to the constructor of `ExecutionContextLifecycleObserver` - is an `ExecutionContextLifecycleObserver` itself and invokes `IDBRequestQueueItem::CancelLoading()` from `ContextDestroyed()`, which calls `IDBRequestLoader::Cancel()`.
As things stand currently, if `IDBRequestLoader` too calls `Cancel()` in `ContextDestroyed()`, it would hit a `DCHECK` for double-cancellation.
Should we remove the cancellation call from `IDBRequest` through to `IDBRequestLoader` in this case and let `IDBRequestLoader` alone handle it? The cancellation message still needs to reach `IDBRequestQueueItem` though, and another scenario for cancellation is when `IDBRequest::Abort()` is called, and in this case, we would want `IDBRequestQueueItem` to cancel the load on `IDBRequestLoader`.
Abhishek Shanthkumarsorry for missing it earlier.
I guess based on what you've stated, it's not actually a problem that the `WeakMember<ExecutionContext> execution_context_` could be non-null after the context is destroyed, because it won't actually be accessed after destruction because `Cancel()` is called. But that is very subtle! I think we should at least set it it to null in `Cancel()`, although even that is subtle in that we're relying on code in some other file to call `Cancel()` when the context is destroyed. It's easier for the next reader to come along and verify correctness if `this` is an `ExecutionContextLifecycleObserver`.
As things stand currently [...]
I feel pretty meh about those DCHECKs. Could easily turn them into early returns. Seems like they were probably added by the original author in order to be more sure the code was working as intended at the time, and not because calling `Cancel()` twice would lead to some disastrous consequence.
I've leave it to you which approach you prefer to go with.
Looking at it more closely, `IDBRequestLoader` does not need to get immediately notified when the `ExecutionContext` is destroyed. It uses the context only when spinning up an instance of `FileReaderLoader` in `StartNextValue()`, and there is already a check for liveness of `ExecutionContext` there. I updated it to also check `IsContextDestroyed()` explicitly (and added code comments) in Patchset 6.
`WeakMember` resets the pointer to null when the context is GC-ed, so we don't need to worry about a UAF in any case.
From the perspective of `IDBRequestLoader`, it is not strictly necessary for a `Cancel()` call to come through when the context is destroyed. Calling `Cancel()` in this case is a "business" decision that must be made by the consumer of `IDBRequestLoader`, in which case `IDBRequestLoader` will return the `values_` and call it done. If the caller does not care about the values when the context is destroyed, then it can not call `Cancel()` at all, and the "right thing" will still happen (`IDBRequestLoader` stops unwrapping more values). This seems fine to me, but do share your thoughts!
IsContextDestroyed check is sufficient, thanks
if (IDBValueUnwrapper::IsWrapped(values_)) {
loader_ = MakeGarbageCollected<IDBRequestLoader>(
std::move(values_), request_->GetExecutionContext(),
WTF::BindOnce(&IDBRequestQueueItem::OnLoadComplete,
weak_factory_.GetWeakPtr()));
}optional: pull this out into a helper `MaybeInitLoader()` as it's repeated 5 times.
(I think
```
bool is_wrapped = IDBValueUnwrapper::IsWrapped(value.get());
values_.push_back(std::move(value));
if (is_wrapped) {
```
is the same as
```
values_.push_back(std::move(value));
if (IDBValueUnwrapper::IsWrapped(values_)) {
```
)
Related details
- Abhishek Shanthkumar
- Paul Semel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted and added 5 comments![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Auto-Submit | +1 |
| Commit-Queue | +2 |
5 comments
Resolved the last of the comments.
This CL fixes the issue by removing the coupling between IDBRequestLoader and IDBRequestQueueItem through the use of WeakPtr-backed callbacks.meganit: line wrap and "a WeakPtr-backed callback"
Done
if (owner_->started_loading_) {
// Try again now that the values exist.
owner_->StartLoading();
}This seems a bit suspicious... Shouldn't we be `StartLoading()` if loading _wasn't_ started? Something to check in a future CL.
if (IDBValueUnwrapper::IsWrapped(values_)) {
loader_ = MakeGarbageCollected<IDBRequestLoader>(
std::move(values_), request_->GetExecutionContext(),
WTF::BindOnce(&IDBRequestQueueItem::OnLoadComplete,
weak_factory_.GetWeakPtr()));
}optional: pull this out into a helper `MaybeInitLoader()` as it's repeated 5 times.
(I think
```
bool is_wrapped = IDBValueUnwrapper::IsWrapped(value.get());
values_.push_back(std::move(value));
if (is_wrapped) {
```is the same as
```
values_.push_back(std::move(value));
if (IDBValueUnwrapper::IsWrapped(values_)) {
```
)
Done.
Evan StadeThis is from the existing error-handling version of `OnResultLoadComplete()`. I guess we can remove it since there is already a strong `CHECK` below, @est...@chromium.org?
I concur
Not applicable now because the function remains as-is; keeping the DCHECK.
Related details
- Paul Semel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Commit-Queue | +1 |
1 comment
@paul...@chromium.org, the CL underwent a few changes but is in its final shape now and has been LGTMed by Evan. Can you take another look? Thank you!
Related details
- Paul Semel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Semel (Gerrit)
Paul Semel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Paul Semel
| Code-Review | +1 |
1 comment
@paul...@chromium.org, the CL underwent a few changes but is in its final shape now and has been LGTMed by Evan. Can you take another look? Thank you!
- Abhishek Shanthkumar
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix dangling pointer to IDBRequestQueueItem in IDBRequestLoader
IDBRequestQueueItem is an off-heap class that originally created and
owned the IDBRequestLoader object through a unique_ptr. IDBRequestLoader
maintains a reference to the owner IDBRequestQueueItem as a raw_ptr.
The CL https://chromium-review.googlesource.com/c/chromium/src/+/4382944
made IDBRequestLoader garbage-collected and replaced the unique_ptr with
a Persistent reference to IDBRequestLoader, but this led to a dangling
pointer since IDBRequestQueueItem can (and often does) get destructed
before IDBRequestLoader.
Tests that hit the dangling pointer check:
- IDBRequestTest.ContextDestroyedWithQueuedErrorResult
- IDBRequestTest.ErrorWithQueuedLoadingResult
- IDBRequestTest.EventsAfterEarlyDeathStopWithQueuedResult
- IDBRequestTest.EventsAfterEarlyDeathStopWithTwoQueuedResults
- IDBTransactionTest.ContextDestroyedWithQueuedResult
- IDBTransactionTest.ContextDestroyedWithTwoQueuedResults
- IDBTransactionTest.DocumentShutdownWithQueuedAndBlockedResults
This CL fixes the issue by removing the coupling between
IDBRequestLoader and IDBRequestQueueItem through the use of a
WeakPtr-backed callback.
Validation:
Verified that the above tests pass under dangling pointer detection.
- M third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
- M third_party/blink/renderer/modules/indexeddb/idb_request_loader.h
- M third_party/blink/renderer/modules/indexeddb/idb_request_queue_item.cc
- M third_party/blink/renderer/modules/indexeddb/idb_request_queue_item.h
- M third_party/blink/renderer/modules/indexeddb/idb_request_test.cc
Code-Review: +1 by Paul Semel, +1 by Evan Stade
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |