fix: make IndexedDB cancellation callback-safe - #70
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the WebGL IndexedDB bridge against cancellation races by ensuring delayed JavaScript callbacks cannot observe freed/reused GCHandles, and by adding tests to cover cancellation → subsequent operation correctness.
Changes:
- Refactors
IDBValueTaskSourceto track managed/native/consumer completion separately and only return pooled sources after both native terminal callback + consumer completion. - Adds
CompletionStateflags/helpers to support the new lifecycle model. - Updates the WebGL
.jslibto enforce exactly-once completion and to close DB / free temporary buffers across terminal paths; adds regression tests for pooling and cancellation.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Runtime/IDBValueTaskSource.cs | Separates managed/native/consumer completion and delays pooling until both are done. |
| src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Runtime/CompletionState.cs | Introduces completion flags + small helpers to coordinate lifecycle safely. |
| src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Runtime/CompletionState.cs.meta | Unity meta for the new runtime file. |
| src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Plugins/WebGL/native_idb.jslib | Adds a completion guard, broader terminal error handling, and tighter memory/DB cleanup. |
| src/LocalPrefs.Unity/Assets/Tests/IDBUtilsTest.cs | Adds regression tests for pooling behavior and cancel-followed-by-next-operation correctness. |
| src/LocalPrefs.Unity/Assets/Tests/IDBUtilsTest.cs.meta | Unity meta for the new test file. |
Files not reviewed (2)
- src/LocalPrefs.Unity/Assets/Tests/IDBUtilsTest.cs.meta: Generated file
- src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Runtime/CompletionState.cs.meta: Generated file
Comments suppressed due to low confidence (3)
src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Plugins/WebGL/native_idb.jslib:45
onupgradeneededcan throw (e.g., createObjectStore failures). If that happens, no terminal callback is sent to C#, leaving theGCHandleallocated and the operation hanging/leaking. Wrap the handler in try/catch and route failures throughfail(...)so exactly-once completion still holds.
request.onupgradeneeded = function (event) {
const db = event.target.result;
if (!db.objectStoreNames.contains(Config.STORE_NAME)) {
db.createObjectStore(Config.STORE_NAME);
}
src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Plugins/WebGL/native_idb.jslib:126
- Same as above: an exception in
onupgradeneededwould skip notifying C# and leak the pending operation/handle. Use try/catch and callfail(exception, db)to guarantee a terminal callback.
request.onupgradeneeded = function (event) {
const db = event.target.result;
if (!db.objectStoreNames.contains(Config.STORE_NAME)) {
db.createObjectStore(Config.STORE_NAME);
}
src/LocalPrefs.Unity/Packages/jp.andantetribe.localprefs/Plugins/WebGL/native_idb.jslib:207
- Same risk here: if
onupgradeneededthrows,completedis never set and neither success nor error is invoked, so the managed await can hang and theGCHandlewon’t be released. Catch exceptions and forward them tofail(...).
request.onupgradeneeded = function (event) {
const db = event.target.result;
if (!db.objectStoreNames.contains(Config.STORE_NAME)) {
db.createObjectStore(Config.STORE_NAME);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
WebGL の IndexedDB 操作をキャンセルした後、遅れて到着する JavaScript コールバックが解放済み/再利用済みの
GCHandleを参照し得る問題を修正します。C# 側のコールバック寿命管理と JavaScript 側の exactly-once 完了保証は片方だけでは安全性を満たせないため、同じPRにまとめています。公開 API は変更しません。
発生していた問題
IDBValueTaskSourceは pool され、JavaScript へGCHandleのポインターを渡します。従来はキャンセルにより managed のValueTaskが完了すると、consumer のGetResult時点でGCHandleを解放し、source を reset して pool へ返していました。しかし、ブラウザー側の IndexedDB request 自体はキャンセルできず、後から success / error callback が到着します。そのため、遅延 callback が解放済みハンドルへ触れたり、pool から別操作へ再利用された source を誤って完了させる可能性がありました。
逆に C# 側だけで native callback を待つ設計へ変えても、JavaScript 側が terminal callback を必ず返さなければハンドルを解放できず、リークします。従来の jslib には同期例外、open blocked、transaction abort など、完了通知が欠ける経路があり、複数エラー経路から重複 callback が起きる可能性もありました。
修正内容
C# 側
GCHandleは native terminal callback 到着時に解放Awaitable.WaitForSecondsAsyncを使用JavaScript 側
completedguard と共通完了処理を導入onblocked、transaction のonabort/onerrorを terminal error として通知HEAPU8.sliceで独立コピーして寿命を明確化影響範囲
検証
dotnet formatを変更 C# ファイルへ実行All test(s) succeededを確認テスト結果受け渡しについて
ブラウザー上のテスト自体は全件成功しました。一方、Unity Test Framework から Editor への結果送信時に接続が切れることがあり、Editor 側では結果取得に失敗する場合があります。今回もブラウザー画面では成功を確認しており、既知の接続不安定性とテスト失敗は区別しています。