Summary: | HashTableConstIterator's consistency assertion fails while closing m_webIDBServers in NetworkProcess::didClose since r275846 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||
Component: | WebKit2 | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cdumez, darin, ddkilzer, ggaren, kkinnunen, ryanhaddad, sihui_liu, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 224305 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Fujii Hironori
2021-04-26 23:07:10 PDT
Created attachment 427124 [details]
Patch
Comment on attachment 427124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427124&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 > - for (auto& server : m_webIDBServers.values()) > - server->close(); Nice catch! Can we replace this with something like: for (auto& sessionID : copyToVector(m_webIDBServers.keys())) m_webIDBServers.get(sessionID)->close(); So we are sure the loop will only run limited times (even when logic of close() changes not). Comment on attachment 427124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427124&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 >> - server->close(); > > Nice catch! Can we replace this with something like: > > for (auto& sessionID : copyToVector(m_webIDBServers.keys())) > m_webIDBServers.get(sessionID)->close(); > > So we are sure the loop will only run limited times (even when logic of close() changes not). Sihui, would you mind explaining why your proposal is better than one Fujii proposed? I kinda like his proposal because: 1. No need for a copy to a vector 2. No hash table look-up of IDs. Comment on attachment 427124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427124&action=review >>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 >>> - server->close(); >> >> Nice catch! Can we replace this with something like: >> >> for (auto& sessionID : copyToVector(m_webIDBServers.keys())) >> m_webIDBServers.get(sessionID)->close(); >> >> So we are sure the loop will only run limited times (even when logic of close() changes not). > > Sihui, would you mind explaining why your proposal is better than one Fujii proposed? I kinda like his proposal because: > 1. No need for a copy to a vector > 2. No hash table look-up of IDs. Not to mention that the dereference of m_webIDBServers.get(sessionID) looks potentially unsafe here. (In reply to Chris Dumez from comment #5) > Comment on attachment 427124 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427124&action=review > > >>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 > >>> - server->close(); > >> > >> Nice catch! Can we replace this with something like: > >> > >> for (auto& sessionID : copyToVector(m_webIDBServers.keys())) > >> m_webIDBServers.get(sessionID)->close(); > >> > >> So we are sure the loop will only run limited times (even when logic of close() changes not). > > > > Sihui, would you mind explaining why your proposal is better than one Fujii proposed? I kinda like his proposal because: > > 1. No need for a copy to a vector > > 2. No hash table look-up of IDs. > > Not to mention that the dereference of m_webIDBServers.get(sessionID) looks > potentially unsafe here. Yes, should better check if m_webIDBServers contains sessionID first. I suggested that just because it makes sure the loop ends no matter what WebIDBServer::close() does. I guess adding an assert to current implementation to make sure the value does not exist in map after close() will also do. Comment on attachment 427124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427124&action=review >>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 >>>> - server->close(); >>> >>> Nice catch! Can we replace this with something like: >>> >>> for (auto& sessionID : copyToVector(m_webIDBServers.keys())) >>> m_webIDBServers.get(sessionID)->close(); >>> >>> So we are sure the loop will only run limited times (even when logic of close() changes not). >> >> Sihui, would you mind explaining why your proposal is better than one Fujii proposed? I kinda like his proposal because: >> 1. No need for a copy to a vector >> 2. No hash table look-up of IDs. > > Not to mention that the dereference of m_webIDBServers.get(sessionID) looks potentially unsafe here. I agree with Chris that Fujii’s original is better than the suggested replacement. I have two thoughts: 1) It’s a *little* scary that the server is removed indirectly through the call to close. This code relies on it happening synchronously and 100% reliably in that function. 2) There is a function named "random()" that does the same thing as begin(), more efficiently. Its name makes it sound less apropos than begin, but it’s probably better for a case like this, for just selecting one item from a hash table to operate on without preference. Comment on attachment 427124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427124&action=review >>>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 >>>>> - server->close(); >>>> >>>> Nice catch! Can we replace this with something like: >>>> >>>> for (auto& sessionID : copyToVector(m_webIDBServers.keys())) >>>> m_webIDBServers.get(sessionID)->close(); >>>> >>>> So we are sure the loop will only run limited times (even when logic of close() changes not). >>> >>> Sihui, would you mind explaining why your proposal is better than one Fujii proposed? I kinda like his proposal because: >>> 1. No need for a copy to a vector >>> 2. No hash table look-up of IDs. >> >> Not to mention that the dereference of m_webIDBServers.get(sessionID) looks potentially unsafe here. > > Yes, should better check if m_webIDBServers contains sessionID first. > > I suggested that just because it makes sure the loop ends no matter what WebIDBServer::close() does. > > I guess adding an assert to current implementation to make sure the value does not exist in map after close() will also do. To be safe and make sure we cannot end up in an infinite loop, I guess the best approach is to tweak Sihui's proposal like so then: ``` for (auto& sessionID : copyToVector(m_webIDBServers.keys())) { if (auto* server = m_webIDBServers.get(sessionID)) server->close(); } ``` I think the following would be a bit more efficient and concise though: ``` for (auto& server : copyToVector(m_webIDBServers.values())) server->close(); ``` (In reply to Darin Adler from comment #7) > Comment on attachment 427124 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427124&action=review > > >>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 > >>>> - server->close(); > >>> > >>> Nice catch! Can we replace this with something like: > >>> > >>> for (auto& sessionID : copyToVector(m_webIDBServers.keys())) > >>> m_webIDBServers.get(sessionID)->close(); > >>> > >>> So we are sure the loop will only run limited times (even when logic of close() changes not). > >> > >> Sihui, would you mind explaining why your proposal is better than one Fujii proposed? I kinda like his proposal because: > >> 1. No need for a copy to a vector > >> 2. No hash table look-up of IDs. > > > > Not to mention that the dereference of m_webIDBServers.get(sessionID) looks potentially unsafe here. > > I agree with Chris that Fujii’s original is better than the suggested > replacement. I have two thoughts: > > 1) It’s a *little* scary that the server is removed indirectly through the > call to close. This code relies on it happening synchronously and 100% > reliably in that function. > > 2) There is a function named "random()" that does the same thing as begin(), > more efficiently. Its name makes it sound less apropos than begin, but it’s > probably better for a case like this, for just selecting one item from a > hash table to operate on without preference. I looked at the implementation of random() and it does not look more efficient than begin() to me? By the way, I did not mean to overwrite Darin's r+. I had a bugzilla conflict :/ Sorry about that. Wow, I was totally wrong about random(); please don’t switch to it. I was confusing it with takeAny(). Comment on attachment 427124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427124&action=review Restoring r+, I personally think the patch is fine as is. Each WebIDBServer has a closeCallback that get passed to it when constructed and which removes the WebIDBServer from m_webIDBServers. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:278 > + m_webIDBServers.values().begin()->get()->close(); I am surprised you really need the `->get()` here. Comment on attachment 427124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427124&action=review >>>>>>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-278 >>>>>>>> - server->close(); >>>>>>> >>>>>>> Nice catch! Can we replace this with something like: >>>>>>> >>>>>>> for (auto& sessionID : copyToVector(m_webIDBServers.keys())) >>>>>>> m_webIDBServers.get(sessionID)->close(); >>>>>>> >>>>>>> So we are sure the loop will only run limited times (even when logic of close() changes not). >>>>>> >>>>>> Sihui, would you mind explaining why your proposal is better than one Fujii proposed? I kinda like his proposal because: >>>>>> 1. No need for a copy to a vector >>>>>> 2. No hash table look-up of IDs. >>>>> >>>>> Not to mention that the dereference of m_webIDBServers.get(sessionID) looks potentially unsafe here. >>>> >>>> Yes, should better check if m_webIDBServers contains sessionID first. >>>> >>>> I suggested that just because it makes sure the loop ends no matter what WebIDBServer::close() does. >>>> >>>> I guess adding an assert to current implementation to make sure the value does not exist in map after close() will also do. >>> >>> I agree with Chris that Fujii’s original is better than the suggested replacement. I have two thoughts: >>> >>> 1) It’s a *little* scary that the server is removed indirectly through the call to close. This code relies on it happening synchronously and 100% reliably in that function. >>> >>> 2) There is a function named "random()" that does the same thing as begin(), more efficiently. Its name makes it sound less apropos than begin, but it’s probably better for a case like this, for just selecting one item from a hash table to operate on without preference. >> >> To be safe and make sure we cannot end up in an infinite loop, I guess the best approach is to tweak Sihui's proposal like so then: >> ``` >> for (auto& sessionID : copyToVector(m_webIDBServers.keys())) { >> if (auto* server = m_webIDBServers.get(sessionID)) >> server->close(); >> } >> ``` >> >> I think the following would be a bit more efficient and concise though: >> ``` >> for (auto& server : copyToVector(m_webIDBServers.values())) >> server->close(); >> ``` > > I looked at the implementation of random() and it does not look more efficient than begin() to me? A downside to removing begin() repeatedly is that computing begin() requires iterating from the start of the table to find an in-use bucket, and that iteration becomes increasingly long as you remove what you find. In the worst case this would be O(N^2); however, the hash table eventually saves you by rehashing as it shrinks. In theory, removing random() could result in a less lopsided pattern of iteration and removal, possibly saving some time. I think the ideal behavior would move the existing table into a temporary, and then iterate the temporary calling close() (without removing from it). That saves all the rehashing commotion, and any need to copy. Not sure if it's valid to effectively remove an entry before calling close() in this case, though. Created attachment 427191 [details]
Patch
Comment on attachment 427191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427191&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:277 > + auto servers = WTFMove(m_webIDBServers); We prefer `std::exchange(m_webIDBServers, { })` in such cases, since m_webIDBServers may get used later on. Comment on attachment 427191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427191&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:278 > + for (auto& server : servers.values()) Also, this can be done on a single line: ``` for (auto& server : std::exchange(m_webIDBServers, { })) server->close(); ``` Comment on attachment 427191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427191&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:278 >> + for (auto& server : servers.values()) > > Also, this can be done on a single line: > ``` > for (auto& server : std::exchange(m_webIDBServers, { })) > server->close(); > ``` Unfortunately, the single line approach still causes the assertion failure. for (auto& server : std::exchange(m_webIDBServers, { }).values()) It seems that I should ensure the HashMap is alive for the 'for' statement. Created attachment 427195 [details]
Patch
Comment on attachment 427195 [details]
Patch
Oh yes. That's because of the `.values()` call. My bad.
Comment on attachment 427195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427195&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:278 > + auto servers = std::exchange(m_webIDBServers, { }); > + for (auto& server : servers.values()) So annoying, this difference between function calls (temporaries last until the end of the full expression) and things like the range-based for statement (temporaries last only until the end of the expression and are destroyed before we start iterating the loop). Committed r276671 (237090@main): <https://commits.webkit.org/237090@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427195 [details]. |