[webkit-reviews] review requested: [Bug 225089] HashTableConstIterator's consistency assertion fails while closing m_webIDBServers in NetworkProcess::didClose since r275846 : [Attachment 427124] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has asked  for review:
Bug 225089: HashTableConstIterator's consistency assertion fails while closing
m_webIDBServers in NetworkProcess::didClose since r275846
https://bugs.webkit.org/show_bug.cgi?id=225089

Attachment 427124: Patch

https://bugs.webkit.org/attachment.cgi?id=427124&action=review




--- Comment #8 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 427124
  --> https://bugs.webkit.org/attachment.cgi?id=427124
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();
```