Fix dangling pointer to IDBRequestQueueItem in IDBRequestLoader [chromium/src : main]

0 views
Skip to first unread message

Abhishek Shanthkumar (Gerrit)

unread,
Apr 17, 2024, 5:50:23 AMApr 17
to Evan Stade, Paul Semel, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
Attention needed from Evan Stade and Paul Semel

Abhishek Shanthkumar voted and added 1 comment

Votes added by Abhishek Shanthkumar

Auto-Submit+1
Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Abhishek Shanthkumar . resolved

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!

Open in Gerrit

Related details

Attention is currently required from:
  • Evan Stade
  • Paul Semel
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
Gerrit-Change-Number: 5460527
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
Gerrit-Reviewer: Evan Stade <est...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-CC: Steve Becker <ste...@microsoft.com>
Gerrit-Attention: Paul Semel <paul...@chromium.org>
Gerrit-Attention: Evan Stade <est...@chromium.org>
Gerrit-Comment-Date: Wed, 17 Apr 2024 09:50:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Paul Semel (Gerrit)

unread,
Apr 17, 2024, 9:29:28 AMApr 17
to Abhishek Shanthkumar, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
Attention needed from Abhishek Shanthkumar and Evan Stade

Paul Semel voted

Code-Review+1
Commit-Queue+2
Open in Gerrit

Related details

Attention is currently required from:
  • Abhishek Shanthkumar
  • Evan Stade
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
Gerrit-Change-Number: 5460527
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
Gerrit-Reviewer: Evan Stade <est...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-CC: Steve Becker <ste...@microsoft.com>
Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
Gerrit-Attention: Evan Stade <est...@chromium.org>
Gerrit-Comment-Date: Wed, 17 Apr 2024 13:29:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Evan Stade (Gerrit)

unread,
Apr 17, 2024, 1:17:23 PMApr 17
to Abhishek Shanthkumar, Paul Semel, Chromium LUCI CQ, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
Attention needed from Abhishek Shanthkumar and Paul Semel

Evan Stade added 3 comments

Patchset-level comments
Evan Stade . resolved

thanks for the fix!

File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
Line 24, Patchset 1 (Latest): base::WeakPtr<IDBRequestQueueItem> queue_item,
Evan Stade . unresolved

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.

Line 141, Patchset 1 (Latest): queue_item_->OnResultLoadComplete();
Evan Stade . unresolved

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.

Open in Gerrit

Related details

Attention is currently required from:
  • Abhishek Shanthkumar
  • Paul Semel
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
    Gerrit-Change-Number: 5460527
    Gerrit-PatchSet: 1
    Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Evan Stade <est...@chromium.org>
    Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
    Gerrit-CC: Steve Becker <ste...@microsoft.com>
    Gerrit-Attention: Paul Semel <paul...@chromium.org>
    Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 17:17:13 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Abhishek Shanthkumar (Gerrit)

    unread,
    Apr 17, 2024, 1:55:54 PMApr 17
    to Paul Semel, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
    Attention needed from Abhishek Shanthkumar and Evan Stade

    Abhishek Shanthkumar added 2 comments

    File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
    Line 24, Patchset 1 (Latest): base::WeakPtr<IDBRequestQueueItem> queue_item,
    Evan Stade . unresolved

    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.

    Abhishek Shanthkumar

    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.

    Line 141, Patchset 1 (Latest): queue_item_->OnResultLoadComplete();
    Evan Stade . unresolved

    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.

    Abhishek Shanthkumar

    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.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Abhishek Shanthkumar
    • Evan Stade
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
    Gerrit-Change-Number: 5460527
    Gerrit-PatchSet: 1
    Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Evan Stade <est...@chromium.org>
    Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
    Gerrit-CC: Steve Becker <ste...@microsoft.com>
    Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Attention: Evan Stade <est...@chromium.org>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 17:53:55 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Evan Stade <est...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Evan Stade (Gerrit)

    unread,
    Apr 17, 2024, 2:18:22 PMApr 17
    to Abhishek Shanthkumar, Paul Semel, Chromium LUCI CQ, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
    Attention needed from Abhishek Shanthkumar

    Evan Stade added 2 comments

    File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
    Line 24, Patchset 1 (Latest): base::WeakPtr<IDBRequestQueueItem> queue_item,
    Evan Stade . unresolved

    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.

    Abhishek Shanthkumar

    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.

    Evan Stade

    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.

    Line 141, Patchset 1 (Latest): queue_item_->OnResultLoadComplete();
    Evan Stade . unresolved

    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.

    Abhishek Shanthkumar

    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.

    Evan Stade

    I don't think the CHECK is even necessary since it's cooked into WeakPtr.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Abhishek Shanthkumar
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
    Gerrit-Change-Number: 5460527
    Gerrit-PatchSet: 1
    Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Evan Stade <est...@chromium.org>
    Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
    Gerrit-CC: Steve Becker <ste...@microsoft.com>
    Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 18:18:10 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Comment-In-Reply-To: Evan Stade <est...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Abhishek Shanthkumar (Gerrit)

    unread,
    Apr 18, 2024, 5:39:05 AMApr 18
    to Paul Semel, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
    Attention needed from Abhishek Shanthkumar

    Abhishek Shanthkumar removed a vote from this change

    Removed Auto-Submit+1 by Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Abhishek Shanthkumar
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: deleteVote
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Abhishek Shanthkumar (Gerrit)

    unread,
    Apr 18, 2024, 8:35:47 AMApr 18
    to Paul Semel, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
    Attention needed from Evan Stade and Paul Semel

    Abhishek Shanthkumar voted and added 3 comments

    Votes added by Abhishek Shanthkumar

    Commit-Queue+1

    3 comments

    Patchset-level comments
    File-level comment, Patchset 3 (Latest):
    Abhishek Shanthkumar . resolved

    Updated the fix to a better one and resolved comments.

    File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
    Line 24, Patchset 1: base::WeakPtr<IDBRequestQueueItem> queue_item,
    Evan Stade . resolved

    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.

    Abhishek Shanthkumar

    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.

    Evan Stade

    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.

    Abhishek Shanthkumar

    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!

    Line 141, Patchset 1: queue_item_->OnResultLoadComplete();
    Evan Stade . resolved

    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.

    Abhishek Shanthkumar

    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.

    Evan Stade

    I don't think the CHECK is even necessary since it's cooked into WeakPtr.

    Abhishek Shanthkumar

    The check does not apply now since queue_item_ is a strong raw pointer.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Evan Stade
    • Paul Semel
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
    Gerrit-Change-Number: 5460527
    Gerrit-PatchSet: 3
    Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
    Gerrit-Reviewer: Evan Stade <est...@chromium.org>
    Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
    Gerrit-CC: Steve Becker <ste...@microsoft.com>
    Gerrit-Attention: Paul Semel <paul...@chromium.org>
    Gerrit-Attention: Evan Stade <est...@chromium.org>
    Gerrit-Comment-Date: Thu, 18 Apr 2024 12:35:29 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Evan Stade (Gerrit)

    unread,
    Apr 18, 2024, 1:01:34 PMApr 18
    to Abhishek Shanthkumar, Paul Semel, Chromium LUCI CQ, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
    Attention needed from Abhishek Shanthkumar and Paul Semel

    Evan Stade added 4 comments

    File third_party/blink/renderer/modules/indexeddb/idb_request_loader.h
    Line 125, Patchset 3 (Latest): const raw_ref<Vector<std::unique_ptr<IDBValue>>> values_;
    Evan Stade . resolved

    this was probably also a problem...

    Line 119, Patchset 3 (Latest): raw_ptr<IDBRequestQueueItem> queue_item_;
    Evan Stade . unresolved

    should this also be a `raw_ref`? It can't be null

    Line 87, Patchset 3 (Latest): private:
    LoadCompleteCallback load_complete_callback_;
    base::OnceClosure load_failed_callback_;
    Evan Stade . unresolved

    optional: for simplicity, combine these. A null argument value can indicate failure.

    File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
    Line 24, Patchset 1: base::WeakPtr<IDBRequestQueueItem> queue_item,
    Evan Stade . unresolved

    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.

    Abhishek Shanthkumar

    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.

    Evan Stade

    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.

    Abhishek Shanthkumar

    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!

    Evan Stade

    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*.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Abhishek Shanthkumar
    • Paul Semel
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 3
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Paul Semel <paul...@chromium.org>
      Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Comment-Date: Thu, 18 Apr 2024 17:01:19 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Abhishek Shanthkumar (Gerrit)

      unread,
      Apr 18, 2024, 1:54:24 PMApr 18
      to Paul Semel, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
      Attention needed from Evan Stade and Paul Semel

      Abhishek Shanthkumar added 1 comment

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
      Abhishek Shanthkumar

      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.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Evan Stade
      • Paul Semel
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 3
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Paul Semel <paul...@chromium.org>
      Gerrit-Attention: Evan Stade <est...@chromium.org>
      Gerrit-Comment-Date: Thu, 18 Apr 2024 17:54:02 +0000
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Abhishek Shanthkumar (Gerrit)

      unread,
      Apr 19, 2024, 7:26:13 AMApr 19
      to Paul Semel, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
      Attention needed from Evan Stade and Paul Semel

      Abhishek Shanthkumar voted and added 8 comments

      Votes added by Abhishek Shanthkumar

      Commit-Queue+1

      8 comments

      Patchset-level comments
      File-level comment, Patchset 5 (Latest):
      Abhishek Shanthkumar . resolved

      Resolved comments and added notes about the changes in Patchset 4.

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.h
      Line 41, Patchset 5 (Latest): // if the context is destroyed. |values| must be kept alive until either the
      // loader calls |load_complete_callback| or the load is canceled.
      Abhishek Shanthkumar . resolved

      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.

      Line 119, Patchset 3: raw_ptr<IDBRequestQueueItem> queue_item_;
      Evan Stade . resolved

      should this also be a `raw_ref`? It can't be null

      Abhishek Shanthkumar

      NA to Patchset 4.


      LoadCompleteCallback load_complete_callback_;
      base::OnceClosure load_failed_callback_;
      Evan Stade . resolved

      optional: for simplicity, combine these. A null argument value can indicate failure.

      Abhishek Shanthkumar

      Used a single callback in Patchset 4.

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
      Line 24, Patchset 1: base::WeakPtr<IDBRequestQueueItem> queue_item,
      Evan Stade . resolved

      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`.

      Line 80, Patchset 5 (Latest): // The execution context was torn down. The loader will eventually get a
      // Cancel() call.
      if (!execution_context_) {
      return;
      }
      Abhishek Shanthkumar . unresolved

      @est...@chromium.org, can/should we move this to the beginning of the function rather than checking it after iterating over `values_`?

      File third_party/blink/renderer/modules/indexeddb/idb_request_queue_item.cc
      Line 281, Patchset 5 (Latest): DCHECK(!ready_);
      Abhishek Shanthkumar . unresolved

      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?

      File third_party/blink/renderer/modules/indexeddb/idb_request_test.cc
      Line 236, Patchset 5 (Latest): request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);
      Abhishek Shanthkumar . unresolved

      @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.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Evan Stade
      • Paul Semel
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 5
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Paul Semel <paul...@chromium.org>
      Gerrit-Attention: Evan Stade <est...@chromium.org>
      Gerrit-Comment-Date: Fri, 19 Apr 2024 11:25:58 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Evan Stade (Gerrit)

      unread,
      Apr 19, 2024, 3:54:33 PMApr 19
      to Abhishek Shanthkumar, Paul Semel, Chromium LUCI CQ, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
      Attention needed from Abhishek Shanthkumar and Paul Semel

      Evan Stade added 4 comments

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.h
      Line 78, Patchset 5 (Latest): WeakMember<ExecutionContext> execution_context_;
      Evan Stade . unresolved

      I think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]

      [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/context_lifecycle_observer.cc;l=41;drc=c76cca217f4278f5c53a8d90f7870270ee4dd81e

      Line 41, Patchset 5 (Latest): // if the context is destroyed. |values| must be kept alive until either the
      // loader calls |load_complete_callback| or the load is canceled.
      Abhishek Shanthkumar . unresolved

      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.

      Evan Stade

      I don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.cc
      Line 80, Patchset 5 (Latest): // The execution context was torn down. The loader will eventually get a
      // Cancel() call.
      if (!execution_context_) {
      return;
      }
      Abhishek Shanthkumar . resolved

      @est...@chromium.org, can/should we move this to the beginning of the function rather than checking it after iterating over `values_`?

      Evan Stade

      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.

      File third_party/blink/renderer/modules/indexeddb/idb_request_queue_item.cc
      Abhishek Shanthkumar . unresolved

      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?

      Evan Stade

      I concur

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Abhishek Shanthkumar
      • Paul Semel
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 5
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Paul Semel <paul...@chromium.org>
      Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Comment-Date: Fri, 19 Apr 2024 19:54:20 +0000
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Abhishek Shanthkumar (Gerrit)

      unread,
      Apr 22, 2024, 10:06:16 AMApr 22
      to Paul Semel, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
      Attention needed from Evan Stade

      Abhishek Shanthkumar added 3 comments

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.h
      Line 78, Patchset 5 (Latest): WeakMember<ExecutionContext> execution_context_;
      Evan Stade . unresolved

      I think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]

      [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/context_lifecycle_observer.cc;l=41;drc=c76cca217f4278f5c53a8d90f7870270ee4dd81e

      Abhishek Shanthkumar

      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`.

      Line 41, Patchset 5 (Latest): // if the context is destroyed. |values| must be kept alive until either the
      // loader calls |load_complete_callback| or the load is canceled.
      Abhishek Shanthkumar . unresolved

      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.

      Evan Stade

      I don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.

      Abhishek Shanthkumar

      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?

      File third_party/blink/renderer/modules/indexeddb/idb_request_test.cc
      Line 236, Patchset 5 (Latest): request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);
      Abhishek Shanthkumar . unresolved

      @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.

      Abhishek Shanthkumar

      @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`?

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Evan Stade
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 5
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Evan Stade <est...@chromium.org>
      Gerrit-Comment-Date: Mon, 22 Apr 2024 14:06:04 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Comment-In-Reply-To: Evan Stade <est...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Evan Stade (Gerrit)

      unread,
      Apr 22, 2024, 4:24:48 PMApr 22
      to Abhishek Shanthkumar, Paul Semel, Chromium LUCI CQ, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
      Attention needed from Abhishek Shanthkumar

      Evan Stade added 3 comments

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.h
      Line 78, Patchset 5 (Latest): WeakMember<ExecutionContext> execution_context_;
      Evan Stade . unresolved

      I think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]

      [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/context_lifecycle_observer.cc;l=41;drc=c76cca217f4278f5c53a8d90f7870270ee4dd81e

      Abhishek Shanthkumar

      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`.

      Evan Stade

      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.

      Line 41, Patchset 5 (Latest): // if the context is destroyed. |values| must be kept alive until either the
      // loader calls |load_complete_callback| or the load is canceled.
      Abhishek Shanthkumar . unresolved

      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.

      Evan Stade

      I don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.

      Abhishek Shanthkumar

      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?

      Evan Stade

      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.

      File third_party/blink/renderer/modules/indexeddb/idb_request_test.cc
      Line 236, Patchset 5 (Latest): request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);
      Abhishek Shanthkumar . unresolved

      @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.

      Abhishek Shanthkumar

      @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`?

      Evan Stade

      I hope cleaning up the ownership of `IDBRequestLoader::values_` fixes this.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Abhishek Shanthkumar
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 5
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Comment-Date: Mon, 22 Apr 2024 20:24:35 +0000
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Evan Stade (Gerrit)

      unread,
      Apr 22, 2024, 4:31:36 PMApr 22
      to Abhishek Shanthkumar, Paul Semel, Chromium LUCI CQ, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
      Attention needed from Abhishek Shanthkumar

      Evan Stade added 1 comment

      Patchset-level comments
      Evan Stade . resolved

      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...

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Abhishek Shanthkumar
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 5
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Comment-Date: Mon, 22 Apr 2024 20:31:21 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Abhishek Shanthkumar (Gerrit)

      unread,
      Apr 23, 2024, 9:11:32 AMApr 23
      to Paul Semel, Chromium LUCI CQ, Evan Stade, Steve Becker, chromium...@chromium.org, blink-...@chromium.org, dmurph+wa...@chromium.org, enne...@chromium.org, jsbel...@chromium.org
      Attention needed from Evan Stade and Paul Semel

      Abhishek Shanthkumar voted and added 5 comments

      Votes added by Abhishek Shanthkumar

      Commit-Queue+1

      5 comments

      Patchset-level comments
      Evan Stade . resolved

      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...

      Abhishek Shanthkumar

      Haha yes... 😊 Maybe worth trying in the next CL!

      File-level comment, Patchset 6 (Latest):
      Abhishek Shanthkumar . resolved

      Resolved comments; the CL is ready for review.

      File third_party/blink/renderer/modules/indexeddb/idb_request_loader.h
      Line 78, Patchset 5: WeakMember<ExecutionContext> execution_context_;
      Evan Stade . resolved

      I think `this` needs to be a `ExecutionContextLifecycleObserver` rather than storing the context directly. The key thing this approach is missing is `NotifyContextDestroyed()`. [1]

      [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/context_lifecycle_observer.cc;l=41;drc=c76cca217f4278f5c53a8d90f7870270ee4dd81e

      Abhishek Shanthkumar

      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`.

      Evan Stade

      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.

      Abhishek Shanthkumar

      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!

      Line 41, Patchset 5: // if the context is destroyed. |values| must be kept alive until either the

      // loader calls |load_complete_callback| or the load is canceled.
      Abhishek Shanthkumar . resolved

      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.

      Evan Stade

      I don't see a purpose in this parameter. `IDBRequestLoader` should own its own members, and can `std::move` the values into the callback.

      Abhishek Shanthkumar

      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?

      Evan Stade

      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.

      Abhishek Shanthkumar

      Done

      File third_party/blink/renderer/modules/indexeddb/idb_request_test.cc
      Line 236, Patchset 5: request->queue_item_->OnResultLoadComplete(/*error=*/nullptr);
      Abhishek Shanthkumar . resolved

      @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.

      Abhishek Shanthkumar

      @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`?

      Evan Stade

      I hope cleaning up the ownership of `IDBRequestLoader::values_` fixes this.

      Abhishek Shanthkumar

      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.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Evan Stade
      • Paul Semel
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: If6ef5c19a5a565e6b6e0ac2a49a7e6f4c26c9fae
      Gerrit-Change-Number: 5460527
      Gerrit-PatchSet: 6
      Gerrit-Owner: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Abhishek Shanthkumar <abhishek.s...@microsoft.com>
      Gerrit-Reviewer: Evan Stade <est...@chromium.org>
      Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
      Gerrit-CC: Steve Becker <ste...@microsoft.com>
      Gerrit-Attention: Paul Semel <paul...@chromium.org>
      Gerrit-Attention: Evan Stade <est...@chromium.org>
      Gerrit-Comment-Date: Tue, 23 Apr 2024 13:11:15 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Evan Stade (Gerrit)

      unread,
      Apr 23, 2024, 11:52:16 AMApr 23