Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525443002:
[IndexedDB] Delete callbacks state on worker thread exit. (Closed)
|
Created:
4 years, 1 month ago by Reilly Grant (use Gerrit) Modified:
4 years, 1 month ago Reviewers:
jsbell CC:
chromium-reviews, cmumford, darin-cc_chromium.org, jam, jsbell+idb_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[IndexedDB] Delete callbacks state on worker thread exit.
During refactoring of r430110 the logic to actually delete the internal
state of IndexedDBCallbacksImpl and IndexedDBDatabaseCallbacksImpl
objects that lives on worker threads was accidentally removed.
BUG=667465
Committed: https://crrev.com/ff03e42e8773445a9ee1649dbcbb5b4aae7c2567
Cr-Commit-Position: refs/heads/master@{#433707}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 18 (9 generated)
Description was changed from ========== [IndexedDB] Delete callbacks state on worker thread exit. During refactoring of r430110 the logic to actually delete the internal state of IndexedDBCallbacksImpl and IndexedDBDatabaseCallbacksImpl objects that lives on worker threads was accidentally removed. BUG=627484 ========== to ========== [IndexedDB] Delete callbacks state on worker thread exit. During refactoring of r430110 the logic to actually delete the internal state of IndexedDBCallbacksImpl and IndexedDBDatabaseCallbacksImpl objects that lives on worker threads was accidentally removed. BUG=667465 ==========
reillyg@chromium.org changed reviewers: + jsbell@chromium.org
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... content/child/indexed_db/indexed_db_dispatcher.cc:206: it->second.release(); Is the release() necessary? https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... content/child/indexed_db/indexed_db_dispatcher.h:131: // with out freeing them seems to be impossible so use a map instead so that nit: "...impossible so use a map..." -> "...impossible. Use a map..."
Oops, meant to say: lgtm
https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... content/child/indexed_db/indexed_db_dispatcher.cc:206: it->second.release(); On 2016/11/21 at 22:38:06, jsbell wrote: > Is the release() necessary? Yes. Otherwise erase(it) will free the object and that would be bad because this is called from within the object's destructor.
still lgtm, especially now that I've engaged my brain. https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2525443002/diff/1/content/child/indexed_db/in... content/child/indexed_db/indexed_db_dispatcher.cc:206: it->second.release(); On 2016/11/21 23:26:10, Reilly Grant wrote: > On 2016/11/21 at 22:38:06, jsbell wrote: > > Is the release() necessary? > > Yes. Otherwise erase(it) will free the object and that would be bad because this > is called from within the object's destructor. Derp. My brain was reading `reset()` for some reason.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by reillyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1479772585184700, "parent_rev":
"c0838faab4b0063d5ccedc19d0a0887a5655181f", "commit_rev":
"9a14f667c061439949d5031053dc8a7ad498455d"}
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Delete callbacks state on worker thread exit. During refactoring of r430110 the logic to actually delete the internal state of IndexedDBCallbacksImpl and IndexedDBDatabaseCallbacksImpl objects that lives on worker threads was accidentally removed. BUG=667465 ========== to ========== [IndexedDB] Delete callbacks state on worker thread exit. During refactoring of r430110 the logic to actually delete the internal state of IndexedDBCallbacksImpl and IndexedDBDatabaseCallbacksImpl objects that lives on worker threads was accidentally removed. BUG=667465 Committed: https://crrev.com/ff03e42e8773445a9ee1649dbcbb5b4aae7c2567 Cr-Commit-Position: refs/heads/master@{#433707} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/ff03e42e8773445a9ee1649dbcbb5b4aae7c2567 Cr-Commit-Position: refs/heads/master@{#433707}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2521773003/ by piman@chromium.org. The reason for reverting is: Likely cause of storage/indexeddb/pending-version-change-stuck-works-with-terminate.html crashes: https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu... https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
