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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request 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 #7 from Darin Adler <darin 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.

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.