...
rdar://71962196
Created attachment 425445 [details] Patch
Created attachment 425475 [details] Patch
Comment on attachment 425475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425475&action=review > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:70 > + postTaskReply(CrossThreadTask([this, callback = WTFMove(callback), origins = crossThreadCopy(m_server->getOrigins())]() mutable { Don't we need either a protected or a weak this here? > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:88 > + postTaskReply(CrossThreadTask([this, callback = WTFMove(callback)]() mutable { ditto > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:106 > + postTaskReply(CrossThreadTask([this, callback = WTFMove(callback)]() mutable { ditto
Created attachment 425521 [details] Patch
(In reply to Alex Christensen from comment #4) > Comment on attachment 425475 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425475&action=review > > > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:70 > > + postTaskReply(CrossThreadTask([this, callback = WTFMove(callback), origins = crossThreadCopy(m_server->getOrigins())]() mutable { > > Don't we need either a protected or a weak this here? > Updated the patch. By the way do you think revealing sessionID in thread name is a bad idea?
Comment on attachment 425521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425521&action=review This looks good. I have some comments. I didn’t set review+ because I am not sure I’m enough of an expert on this area to spot mistakes, but I think it looks right. > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:97 > + enum class DataTaskCountUpdateType : uint8_t { Decrease, Increase }; > + void updateDataTaskCount(DataTaskCountUpdateType); This is a peculiar interface. I suggest two separate named functions rather than a function with an enum class argument. Also, to try to make this harder to get wrong, I suggest we make an object that increments on creation and decrements on destruction. We can capture that object in lambdas, instead of capturing "protectedThis" and writing an explicit decrement function. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2675 > + if (auto* webIDBServer = m_webIDBServers.get(sessionID)) > + webIDBServer->removeConnection(connection); In a tight context like this, I really like single word variable names that don’t try to disambiguate or be precise. Like: if (auto server = m_webIDBServers.get(sessionID)) server->removeConnection(connection); Sure, the local variable can be named webIDBServer or even webIDBServerForSession, but it doesn’t need to be. Single words work really well to make such code clear and readable.
Created attachment 425542 [details] Patch
(In reply to Darin Adler from comment #7) > Comment on attachment 425521 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425521&action=review > > This looks good. I have some comments. I didn’t set review+ because I am not > sure I’m enough of an expert on this area to spot mistakes, but I think it > looks right. > > > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:97 > > + enum class DataTaskCountUpdateType : uint8_t { Decrease, Increase }; > > + void updateDataTaskCount(DataTaskCountUpdateType); > > This is a peculiar interface. I suggest two separate named functions rather > than a function with an enum class argument. > > Also, to try to make this harder to get wrong, I suggest we make an object > that increments on creation and decrements on destruction. We can capture > that object in lambdas, instead of capturing "protectedThis" and writing an > explicit decrement function. > Thanks for the comments. I've replaced it with a RefCounter. > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2675 > > + if (auto* webIDBServer = m_webIDBServers.get(sessionID)) > > + webIDBServer->removeConnection(connection); > > In a tight context like this, I really like single word variable names that > don’t try to disambiguate or be precise. Like: > > if (auto server = m_webIDBServers.get(sessionID)) > server->removeConnection(connection); > > Sure, the local variable can be named webIDBServer or even > webIDBServerForSession, but it doesn’t need to be. Single words work really > well to make such code clear and readable. Updated.
Comment on attachment 425542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425542&action=review > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:44 > + , m_dataTaskCounter([protectedThis = makeRef(*this)](RefCounterEvent event) { protectedThis->tryClose(); }) This creates a reference cycle since the WebIDBServer owns its m_dataTaskCounter, which in turn owns this lambda, which in turn references the WebIDBServer, so I think the WebIDBServer will leak.
Created attachment 425555 [details] Patch
Comment on attachment 425555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425555&action=review > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:435 > + m_closeCallback(); It would probably be worth null checking m_closeCallback to make this more robust in case some tries to call tryClose twice in the future. > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:107 > + typedef RefCounter<DataTaskCounterType> DataTaskCounter; "using DataTaskCounter = RefCounter<DataTaskCounterType>" is the cool new way to do this. Same with DataTaskCounterToken > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2330 > RefPtr<StorageQuotaManager> storageQuotaManager = weakThis ? this->storageQuotaManager(sessionID, origin) : nullptr; If you used weakThis-> instead of this-> then you wouldn't need to capture this. That would be better because it wouldn't allow you to accidentally use an unchecked this pointer (which is easy to do).
Comment on attachment 425555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425555&action=review > Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:62 > + send(Messages::NetworkConnectionToWebProcess::AddIDBConnection()); Should there be a corresponding remove idb connection message in the destructor?
Comment on attachment 425555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425555&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2330 >> RefPtr<StorageQuotaManager> storageQuotaManager = weakThis ? this->storageQuotaManager(sessionID, origin) : nullptr; > > If you used weakThis-> instead of this-> then you wouldn't need to capture this. That would be better because it wouldn't allow you to accidentally use an unchecked this pointer (which is easy to do). This is excellent advice. Also, when something is ThreadSafeRefCounted, we typically need to ref it before dereferencing it, not just dereference a weak pointer. There is likely a reason that’s not needed here, but *generally* the idiom would be to put the weak pointer into a RefPtr first.
Created attachment 425650 [details] Patch for landing
Created attachment 425652 [details] Patch for landing
Created attachment 425653 [details] Patch for landing
Comment on attachment 425555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425555&action=review >> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:435 >> + m_closeCallback(); > > It would probably be worth null checking m_closeCallback to make this more robust in case some tries to call tryClose twice in the future. Added. >> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:107 >> + typedef RefCounter<DataTaskCounterType> DataTaskCounter; > > "using DataTaskCounter = RefCounter<DataTaskCounterType>" is the cool new way to do this. Same with DataTaskCounterToken Cool. >>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2330 >>> RefPtr<StorageQuotaManager> storageQuotaManager = weakThis ? this->storageQuotaManager(sessionID, origin) : nullptr; >> >> If you used weakThis-> instead of this-> then you wouldn't need to capture this. That would be better because it wouldn't allow you to accidentally use an unchecked this pointer (which is easy to do). > > This is excellent advice. > > Also, when something is ThreadSafeRefCounted, we typically need to ref it before dereferencing it, not just dereference a weak pointer. There is likely a reason that’s not needed here, but *generally* the idiom would be to put the weak pointer into a RefPtr first. ah, I think we do need to ref it here. Updated in the patch. >> Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:62 >> + send(Messages::NetworkConnectionToWebProcess::AddIDBConnection()); > > Should there be a corresponding remove idb connection message in the destructor? NetworkProcess::connectionToWebProcessClosed should already cover this case.
Committed r275779 (236352@main): <https://commits.webkit.org/236352@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425653 [details].
Reopening to attach new patch.
Created attachment 425666 [details] [fast-cq] Patch
Committed r275784 (236355@main): <https://commits.webkit.org/236355@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425666 [details].
Created attachment 425680 [details] [fast-cq] Patch
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Created attachment 425681 [details] [fast-cq] Patch
Committed r275794 (236365@main): <https://commits.webkit.org/236365@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425681 [details].
Created attachment 425685 [details] Patch for landing
Created attachment 425686 [details] Patch for landing
Patch 425685 does not build
Committed r275799 (236370@main): <https://commits.webkit.org/236370@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425686 [details].
Reopened as the patch is reverted in r275799
Created attachment 425748 [details] Patch
Committed r275846 (236411@main): <https://commits.webkit.org/236411@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425748 [details].
Filed: Bug 225089 – HashTableConstIterator's consistency assertion fails while closing m_webIDBServers in NetworkProcess::didClose since r275846