188029 2018-07-25 16:32:24 -0700 Speculative fix for StorageProcess crash 2022-02-08 16:29:38 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED INVALID P2 Normal --- 1 achristensen achristensen alecflett beidson cdumez ews-watchlist jsbell sihui_liu oldest_to_newest 1445214 0 achristensen 2018-07-25 16:32:24 -0700 Speculative fix for StorageProcess crash 1445217 1 345801 achristensen 2018-07-25 16:34:56 -0700 Created attachment 345801 Patch 1445220 2 345801 cdumez 2018-07-25 16:42:11 -0700 Comment on attachment 345801 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345801&action=review I am not in favor of making such a change: 1. We do not know for sure this is related to threading. 2. AFAIK, UniqueIDBDatabaseConnection should not be dealt with from non-main thread, they are not even ThreadSafeRefCounted If we suspect threading, I think we should look at call sites. Also, adding the isMainThread() assertions is a good idea (maybe even release assertions?). > Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:87 > + Lock m_databaseConnectionsLock; What about IDBConnectionToClient::connectionToClientClosed() ? You're failing to lock there so the Lock is not useful. 1445221 3 cdumez 2018-07-25 16:45:16 -0700 Also, this does not look safe: void IDBConnectionToClient::connectionToClientClosed() { auto databaseConnections = m_databaseConnections; for (auto connection : databaseConnections) { if (m_databaseConnections.contains(connection)) connection->connectionClosedFromClient(); } m_databaseConnections.clear(); } databaseConnections is a Vector of raw pointers. I am assuming we're copying m_databaseConnections to a vector before looping because m_databaseConnections may change while we iterate. If databaseConnection objects get destroyed while we iterate, we'll use a stale pointer and crash, despite us copying. 1445222 4 cdumez 2018-07-25 16:46:17 -0700 (In reply to Chris Dumez from comment #3) > Also, this does not look safe: > void IDBConnectionToClient::connectionToClientClosed() > { > auto databaseConnections = m_databaseConnections; > > for (auto connection : databaseConnections) { > if (m_databaseConnections.contains(connection)) > connection->connectionClosedFromClient(); > } > > m_databaseConnections.clear(); > } > > databaseConnections is a Vector of raw pointers. I am assuming we're copying > m_databaseConnections to a vector before looping because > m_databaseConnections may change while we iterate. If databaseConnection > objects get destroyed while we iterate, we'll use a stale pointer and crash, > despite us copying. We may want: auto databaseConnections = copyToVectorOf<RefPtr<UniqueIDBDatabaseConnection>>(m_databaseConnections); 1445449 5 cdumez 2018-07-26 09:55:24 -0700 (In reply to Chris Dumez from comment #4) > (In reply to Chris Dumez from comment #3) > > Also, this does not look safe: > > void IDBConnectionToClient::connectionToClientClosed() > > { > > auto databaseConnections = m_databaseConnections; > > > > for (auto connection : databaseConnections) { > > if (m_databaseConnections.contains(connection)) > > connection->connectionClosedFromClient(); > > } > > > > m_databaseConnections.clear(); > > } > > > > databaseConnections is a Vector of raw pointers. I am assuming we're copying > > m_databaseConnections to a vector before looping because > > m_databaseConnections may change while we iterate. If databaseConnection > > objects get destroyed while we iterate, we'll use a stale pointer and crash, > > despite us copying. > > We may want: > auto databaseConnections = > copyToVectorOf<RefPtr<UniqueIDBDatabaseConnection>>(m_databaseConnections); Actually, it appears to be safe. The following check: if (m_databaseConnections.contains(connection)) should make sure that the connection is still alive since the connection destructor would have removed it from m_databaseConnections. 1838951 6 sihui_liu 2022-02-08 16:29:38 -0800 StorageProcess does not exist now. 345801 2018-07-25 16:34:56 -0700 2018-07-25 16:42:11 -0700 Patch bug-188029-20180725163455.patch text/plain 2954 achristensen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzNDIxOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDE4LTA3LTI1ICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgU3BlY3VsYXRp dmUgZml4IGZvciBTdG9yYWdlUHJvY2VzcyBjcmFzaAorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTg4MDI5CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS80 MTYyOTAxND4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBUaGVyZSBpcyBhIHZlcnkgcmFyZSBjcmFzaCBwcm9iYWJseSBmcm9tIEhhc2hUYWJsZSBjb3Jy dXB0aW9uLgorICAgICAgICBNYXliZSB0aGlzIGlzIG51bGwgaW4gVW5pcXVlSURCRGF0YWJhc2VD b25uZWN0aW9uOjpjb25uZWN0aW9uQ2xvc2VkRnJvbUNsaWVudC4KKyAgICAgICAgTWF5YmUgdGhp cyBpcyBjYXVzZWQgYnkgSURCQ29ubmVjdGlvblRvQ2xpZW50J3MgbV9kYXRhYmFzZUNvbm5lY3Rp b25zIGJlaW5nIGFjY2Vzc2VkIGZyb20gbXVsdGlwbGUgdGhyZWFkcy4KKworICAgICAgICAqIE1v ZHVsZXMvaW5kZXhlZGRiL3NlcnZlci9JREJDb25uZWN0aW9uVG9DbGllbnQuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6SURCU2VydmVyOjpJREJDb25uZWN0aW9uVG9DbGllbnQ6OnJlZ2lzdGVyRGF0 YWJhc2VDb25uZWN0aW9uKToKKyAgICAgICAgKFdlYkNvcmU6OklEQlNlcnZlcjo6SURCQ29ubmVj dGlvblRvQ2xpZW50Ojp1bnJlZ2lzdGVyRGF0YWJhc2VDb25uZWN0aW9uKToKKyAgICAgICAgKiBN b2R1bGVzL2luZGV4ZWRkYi9zZXJ2ZXIvSURCQ29ubmVjdGlvblRvQ2xpZW50Lmg6CisKIDIwMTgt MDctMjUgIFphbGFuIEJ1anRhcyAgPHphbGFuQGFwcGxlLmNvbT4KIAogICAgICAgICBSRUdSRVNT SU9OKHIyMjc1NzcpIFRleHQgb24gVFYgJiBNb3ZpZXMgcGFnZSBkb2Vzbid0IHdyYXAgcHJvcGVy bHkgaW4gaVR1bmVzCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2luZGV4ZWRkYi9zZXJ2 ZXIvSURCQ29ubmVjdGlvblRvQ2xpZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29y ZS9Nb2R1bGVzL2luZGV4ZWRkYi9zZXJ2ZXIvSURCQ29ubmVjdGlvblRvQ2xpZW50LmNwcAkocmV2 aXNpb24gMjM0MjA5KQorKysgU291cmNlL1dlYkNvcmUvTW9kdWxlcy9pbmRleGVkZGIvc2VydmVy L0lEQkNvbm5lY3Rpb25Ub0NsaWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI5LDYgKzI5LDcg QEAKICNpZiBFTkFCTEUoSU5ERVhFRF9EQVRBQkFTRSkKIAogI2luY2x1ZGUgIlVuaXF1ZUlEQkRh dGFiYXNlQ29ubmVjdGlvbi5oIgorI2luY2x1ZGUgPHd0Zi9NYWluVGhyZWFkLmg+CiAKIG5hbWVz cGFjZSBXZWJDb3JlIHsKIG5hbWVzcGFjZSBJREJTZXJ2ZXIgewpAQCAtMTY1LDEzICsxNjYsMjIg QEAgdm9pZCBJREJDb25uZWN0aW9uVG9DbGllbnQ6OmRpZEdldEFsbERhdAogCiB2b2lkIElEQkNv bm5lY3Rpb25Ub0NsaWVudDo6cmVnaXN0ZXJEYXRhYmFzZUNvbm5lY3Rpb24oVW5pcXVlSURCRGF0 YWJhc2VDb25uZWN0aW9uJiBjb25uZWN0aW9uKQogeworICAgIEFTU0VSVChpc01haW5UaHJlYWQo KSk7CiAgICAgQVNTRVJUKCFtX2RhdGFiYXNlQ29ubmVjdGlvbnMuY29udGFpbnMoJmNvbm5lY3Rp b24pKTsKLSAgICBtX2RhdGFiYXNlQ29ubmVjdGlvbnMuYWRkKCZjb25uZWN0aW9uKTsKKyAgICAK KyAgICB7CisgICAgICAgIExvY2tlcjxMb2NrPiBsb2NrZXIobV9kYXRhYmFzZUNvbm5lY3Rpb25z TG9jayk7CisgICAgICAgIG1fZGF0YWJhc2VDb25uZWN0aW9ucy5hZGQoJmNvbm5lY3Rpb24pOwor ICAgIH0KIH0KIAogdm9pZCBJREJDb25uZWN0aW9uVG9DbGllbnQ6OnVucmVnaXN0ZXJEYXRhYmFz ZUNvbm5lY3Rpb24oVW5pcXVlSURCRGF0YWJhc2VDb25uZWN0aW9uJiBjb25uZWN0aW9uKQogewot ICAgIG1fZGF0YWJhc2VDb25uZWN0aW9ucy5yZW1vdmUoJmNvbm5lY3Rpb24pOworICAgIEFTU0VS VChpc01haW5UaHJlYWQoKSk7CisgICAgeworICAgICAgICBMb2NrZXI8TG9jaz4gbG9ja2VyKG1f ZGF0YWJhc2VDb25uZWN0aW9uc0xvY2spOworICAgICAgICBtX2RhdGFiYXNlQ29ubmVjdGlvbnMu cmVtb3ZlKCZjb25uZWN0aW9uKTsKKyAgICB9CiB9CiAKIHZvaWQgSURCQ29ubmVjdGlvblRvQ2xp ZW50Ojpjb25uZWN0aW9uVG9DbGllbnRDbG9zZWQoKQpJbmRleDogU291cmNlL1dlYkNvcmUvTW9k dWxlcy9pbmRleGVkZGIvc2VydmVyL0lEQkNvbm5lY3Rpb25Ub0NsaWVudC5oCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJDb3JlL01vZHVsZXMvaW5kZXhlZGRiL3NlcnZlci9JREJDb25uZWN0aW9u VG9DbGllbnQuaAkocmV2aXNpb24gMjM0MjA5KQorKysgU291cmNlL1dlYkNvcmUvTW9kdWxlcy9p bmRleGVkZGIvc2VydmVyL0lEQkNvbm5lY3Rpb25Ub0NsaWVudC5oCSh3b3JraW5nIGNvcHkpCkBA IC04NCw2ICs4NCw3IEBAIHByaXZhdGU6CiAgICAgCiAgICAgUmVmPElEQkNvbm5lY3Rpb25Ub0Ns aWVudERlbGVnYXRlPiBtX2RlbGVnYXRlOwogICAgIEhhc2hTZXQ8VW5pcXVlSURCRGF0YWJhc2VD b25uZWN0aW9uKj4gbV9kYXRhYmFzZUNvbm5lY3Rpb25zOworICAgIExvY2sgbV9kYXRhYmFzZUNv bm5lY3Rpb25zTG9jazsKIH07CiAKIH0gLy8gbmFtZXNwYWNlIElEQlNlcnZlcgo=