Bug 202074 – Stop refing UniqueIDBDatabaseTransaction in callbacks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

After quota check is introduced to IDB, the callback that takes the ref of UniqueIDBDatabaseTransaction will be passed to StorageQuotaManager. Previously the callback was only passed to UniqueIDBDatabase and UniqueIDBDatabase would make sure they were called before UniqueIDBDatabase was destroyed. Now UniqueIDBDatabaseTransaction may live longer than UniqueIDBDatabase.

[Sihui Liu]

Created attachment 379313 [details]
Patch

[youenn fablet]

Comment on attachment 379313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379313&action=review

> Source/WebCore/ChangeLog:10
> +        UniqueIDBDatabase.

Your patch seems to move away from refing UniqueIDBDatabaseTransaction and instead use WeakPtr.
Would it be possible to make UniqueIDBDatabaseTransaction no longer be RefCounted?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1229
> +    requestSpace(taskSize, "putOrAdd", [this, taskSize, transaction = makeWeakPtr(transaction), requestData, keyData, value, callback = WTFMove(callback), overwriteMode](auto error) mutable {

With your changes, it seems requestSpace could take a UniqueIDBDatabaseTransaction&, create the makeWeakPtr itself and return UnknownError.
This would put the transaction check in only one place.
This would also apply well to waitForRequestSpaceCompletion.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:82
> +    database->abortTransaction(*this, UniqueIDBDatabase::WaitForPendingTasks::Yes, [this, weakThis = makeWeakPtr(*this)](const IDBError& error) {

Could use auto& for error

[Sihui Liu]

(In reply to youenn fablet from comment #2)
> Comment on attachment 379313 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379313&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        UniqueIDBDatabase.
> 
> Your patch seems to move away from refing UniqueIDBDatabaseTransaction and
> instead use WeakPtr.
> Would it be possible to make UniqueIDBDatabaseTransaction no longer be
> RefCounted?
> 
UniqueIDBDatabaseTransaction can be kept alive by UniqueIDBDatabase and be destroyed when it's finished. We just need to make sure last ref of UniqueIDBDatabaseTransaction is properly released when UniqueIDBDatabase is closed.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1229
> > +    requestSpace(taskSize, "putOrAdd", [this, taskSize, transaction = makeWeakPtr(transaction), requestData, keyData, value, callback = WTFMove(callback), overwriteMode](auto error) mutable {
> 
> With your changes, it seems requestSpace could take a
> UniqueIDBDatabaseTransaction&, create the makeWeakPtr itself and return
> UnknownError.
> This would put the transaction check in only one place.
> This would also apply well to waitForRequestSpaceCompletion.
> 
Okay. Will make this change.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:82
> > +    database->abortTransaction(*this, UniqueIDBDatabase::WaitForPendingTasks::Yes, [this, weakThis = makeWeakPtr(*this)](const IDBError& error) {
> 
> Could use auto& for error

Sure.

[Sihui Liu]

Created attachment 379373 [details]
Patch

[youenn fablet]

Comment on attachment 379373 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379373&action=review

> Source/WebCore/ChangeLog:9
> +        passed to StorageQuotaManager. This can make UniqueIDBDatabaseTransaction live longer than UniqueIDBDatabase.

LGTM.
As long as UniqueIDBDatabaseTransaction is RefCounted, we have no real way of guaranteeing that UniqueIDBDatabaseTransaction will never live longer than UniqueIDBDatabase.
Ideally, we should try to make UniqueIDBDatabaseTransaction no longer RefCounted.
Instead, UniqueIDBDatabase will have unique_ptr<UniqueIDBDatabaseTransaction> and other users of transaction will have WeakPtr<UniqueIDBDatabaseTransaction>.

[Sihui Liu]

(In reply to youenn fablet from comment #5)
> Comment on attachment 379373 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379373&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        passed to StorageQuotaManager. This can make UniqueIDBDatabaseTransaction live longer than UniqueIDBDatabase.
> 
> LGTM.
> As long as UniqueIDBDatabaseTransaction is RefCounted, we have no real way
> of guaranteeing that UniqueIDBDatabaseTransaction will never live longer
> than UniqueIDBDatabase.
> Ideally, we should try to make UniqueIDBDatabaseTransaction no longer
> RefCounted.
> Instead, UniqueIDBDatabase will have
> unique_ptr<UniqueIDBDatabaseTransaction> and other users of transaction will
> have WeakPtr<UniqueIDBDatabaseTransaction>.

I see. I will make a follow-up patch if that does not break things.

[WebKit Commit Bot]

Comment on attachment 379373 [details]
Patch

Clearing flags on attachment: 379373

Committed r250317: <https://trac.webkit.org/changeset/250317>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55676811>