225089 – HashTableConstIterator's consistency assertion fails while closing m_webIDBServers in NetworkProcess::didClose since r275846

Bug 225089 - HashTableConstIterator's consistency assertion fails while closing m_webIDBServers in NetworkProcess::didClose since r275846

Summary: HashTableConstIterator's consistency assertion fails while closing m_webIDBSe...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebKit2 (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Fujii Hironori

URL:
Keywords:	InRadar

Depends on:	224305
Blocks:
	Show dependency tree / graph

Reported:	2021-04-26 23:07 PDT by Fujii Hironori
Modified:	2021-04-27 15:11 PDT (History)
CC List:	9 users (show)

See Also:

Attachments
Patch (1.58 KB, patch) 2021-04-26 23:35 PDT, Fujii Hironori	no flags	Details \| Formatted Diff \| Diff
Patch (1.55 KB, patch) 2021-04-27 13:46 PDT, Fujii Hironori	no flags	Details \| Formatted Diff \| Diff
Patch (1.56 KB, patch) 2021-04-27 14:12 PDT, Fujii Hironori	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Fujii Hironori 2021-04-26 23:07:10 PDT

HashTableConstIterator's consistency assertion fails while closing m_webIDBServers in NetworkProcess::didClose since r275846

WinCairo WK2 Debug
WebKitNetworkProcess.exe is crashing while running layout tests.

Callstack:

 # Child-SP          RetAddr           Call Site
00 00000033`2bfff380 00007ffc`3ba940c1 WTF!WTFCrash(void)+0x1f [C:\home\webkit\gc\Source\WTF\wtf\Assertions.cpp @ 305]
01 00000033`2bfff3b0 00007ffc`3c575d12 WebKit2!WTFCrashWithInfo(int __formal = 0n219, char * __formal = 0x00007ffc`418912e0 "C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf/HashTable.h", char * __formal = 0x00007ffc`43a9bb10 "WTF::HashTableConstIterator<class WTF::HashTable<class PAL::SessionID,struct WTF::KeyValuePair<class PAL::SessionID,class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> > >,struct WTF::KeyValuePairKeyExtractor<struct WTF::KeyValuePair<class PAL::SessionID,class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> > > >,struct WTF::DefaultHash<class PAL::SessionID>,struct WTF::HashMap<class PAL::SessionID,class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> >,struct WTF::DefaultHash<class PAL::SessionID>,struct WTF::HashTraits<class PAL::SessionID>,struct WTF::HashTraits<class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> > >,struct WTF::HashTableTraits>::KeyValuePairTraits,struct WTF::HashTraits<class PAL::SessionID> >,class PAL::SessionID,struct WTF::KeyValuePair<class PAL::SessionID,class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> > >,struct WTF::KeyValuePairKeyExtractor<struct WTF::KeyValuePair<class PAL::SessionID,class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> > > >,struct WTF::DefaultHash<class PAL::SessionID>,struct WTF::HashMap<class PAL::SessionID,class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> >,struct WTF::DefaultHash<class PAL::SessionID>,struct WTF::HashTraits<class PAL::SessionID>,struct WTF::HashTraits<class WTF::RefPtr<class WebKit::WebIDBServer,struct WTF::RawPtrTraits<class WebKit::WebIDBServer>,struct WTF::DefaultRefDerefTraits<class WebKit::WebIDBServer> > >,struct WTF::HashTableTraits>::KeyValuePairTraits,struct WTF::HashTraits<class PAL::SessionID> >::checkValidity", int __formal = 0n72)+0x31 [C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf\Assertions.h @ 693]
02 00000033`2bfff3e0 00007ffc`3c561fd8 WebKit2!WTF::HashTableConstIterator<WTF::HashTable<PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >,PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >::checkValidity(void)+0x72 [C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf\HashTable.h @ 219]
03 00000033`2bfff410 00007ffc`3c56259b WebKit2!WTF::HashTableConstIterator<WTF::HashTable<PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >,PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >::operator++(void)+0x28 [C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf\HashTable.h @ 187]
04 00000033`2bfff440 00007ffc`3c5627db WebKit2!WTF::HashTableIterator<WTF::HashTable<PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >,PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >::operator++(void)+0x2b [C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf\HashTable.h @ 273]
05 00000033`2bfff470 00007ffc`3c5629db WebKit2!WTF::HashTableIteratorAdapter<WTF::HashTable<PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >::operator++(void)+0x2b [C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf\HashIterators.h @ 80]
06 00000033`2bfff4a0 00007ffc`3c4cdb37 WebKit2!WTF::HashTableValuesIterator<WTF::HashTable<PAL::SessionID,WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > > >,WTF::DefaultHash<PAL::SessionID>,WTF::HashMap<PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> >,WTF::DefaultHash<PAL::SessionID>,WTF::HashTraits<PAL::SessionID>,WTF::HashTraits<WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >,WTF::HashTableTraits>::KeyValuePairTraits,WTF::HashTraits<PAL::SessionID> >,PAL::SessionID,WTF::RefPtr<WebKit::WebIDBServer,WTF::RawPtrTraits<WebKit::WebIDBServer>,WTF::DefaultRefDerefTraits<WebKit::WebIDBServer> > >::operator++(void)+0x2b [C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf\HashIterators.h @ 163]
07 00000033`2bfff4d0 00007ffc`3c7cc2be WebKit2!WebKit::NetworkProcess::didClose(class IPC::Connection * __formal = 0x000001b5`0afb1bb0)+0x197 [C:\home\webkit\gc\Source\WebKit\NetworkProcess\NetworkProcess.cpp @ 277]
08 00000033`2bfff670 00007ffc`3c7cef0f WebKit2!<lambda_6f468efff3d3d300976f9a19a0593f24>::operator()(void)+0x9e [C:\home\webkit\gc\Source\WebKit\Platform\IPC\Connection.cpp @ 895]
09 00000033`2bfff6c0 00007ffc`6cc89cb7 WebKit2!WTF::Detail::CallableWrapper<<lambda_6f468efff3d3d300976f9a19a0593f24>,void>::call(void)+0x2f [C:\home\webkit\gc\WebKitBuild\Debug\WTF\Headers\wtf\Function.h @ 52]
0a 00000033`2bfff6f0 00007ffc`6ccffe87 WTF!WTF::Function<void __cdecl(void)+0xa7 [C:\home\webkit\gc\Source\WTF\wtf\Function.h @ 84]
0b 00000033`2bfff730 00007ffc`6cdf37c5 WTF!WTF::RunLoop::performWork(void)+0x187 [C:\home\webkit\gc\Source\WTF\wtf\RunLoop.cpp @ 134]
0c 00000033`2bfff830 00007ffc`6cdf3718 WTF!WTF::RunLoop::wndProc(struct HWND__ * hWnd = 0x00000000`00740d62, unsigned int message = 0x401, unsigned int64 wParam = 0x000001b5`0af93060, int64 lParam = 0n0)+0x55 [C:\home\webkit\gc\Source\WTF\wtf\win\RunLoopWin.cpp @ 57]
0d 00000033`2bfff870 00007ffc`b318e858 WTF!WTF::RunLoop::RunLoopWndProc(struct HWND__ * hWnd = 0x00000000`00740d62, unsigned int message = 0x401, unsigned int64 wParam = 0x000001b5`0af93060, int64 lParam = 0n0)+0x68 [C:\home\webkit\gc\Source\WTF\wtf\win\RunLoopWin.cpp @ 39]
0e 00000033`2bfff8c0 00007ffc`b318e299 USER32!UserCallWinProcCheckWow+0x2f8
0f 00000033`2bfffa50 00007ffc`6cdf2b54 USER32!DispatchMessageWorker+0x249
10 00000033`2bfffad0 00007ffc`3bb2496f WTF!WTF::RunLoop::run(void)+0x64 [C:\home\webkit\gc\Source\WTF\wtf\win\RunLoopWin.cpp @ 74]
11 00000033`2bfffb60 00007ffc`3bb24091 WebKit2!WebKit::AuxiliaryProcessMainBase<WebKit::NetworkProcess,0>::run(int argc = 0n7, char ** argv = 0x000001b5`0af9d880)+0xbf [C:\home\webkit\gc\Source\WebKit\Shared\AuxiliaryProcessMain.h @ 71]
12 00000033`2bfffbb0 00007ffc`3bb23f9f WebKit2!WebKit::AuxiliaryProcessMain<WebKit::NetworkProcessMainCurl>(int argc = 0n7, char ** argv = 0x000001b5`0af9d880)+0x71 [C:\home\webkit\gc\Source\WebKit\Shared\AuxiliaryProcessMain.h @ 97]
13 00000033`2bfffc80 00007ff6`ec5e1030 WebKit2!WebKit::NetworkProcessMain(int argc = 0n7, char ** argv = 0x000001b5`0af9d880)+0x2f [C:\home\webkit\gc\Source\WebKit\NetworkProcess\curl\NetworkProcessMainCurl.cpp @ 45]
14 00000033`2bfffcb0 00007ff6`ec5e1270 WebKitNetworkProcess!main(int argc = 0n7, char ** argv = 0x000001b5`0af9d880)+0x30 [C:\home\webkit\gc\Source\WebKit\NetworkProcess\EntryPoint\win\NetworkProcessMain.cpp @ 35]
15 (Inline Function) --------`-------- WebKitNetworkProcess!invoke_main(void)+0x22 [D:\a01\_work\26\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 78]
16 00000033`2bfffce0 00007ffc`b30d7034 WebKitNetworkProcess!__scrt_common_main_seh(void)+0x10c [D:\a01\_work\26\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 288]
17 00000033`2bfffd20 00007ffc`b4862651 KERNEL32!BaseThreadInitThunk+0x14
18 00000033`2bfffd50 00000000`00000000 ntdll!RtlUserThreadStart+0x21

Comment 1 Fujii Hironori 2021-04-26 23:35:33 PDT

Created attachment 427124 [details]
Patch

Comment 2 Radar WebKit Bug Importer 2021-04-27 08:21:26 PDT

<rdar://problem/77210724>

Comment 3 Sihui Liu 2021-04-27 08:38:01 PDT

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 4 Chris Dumez 2021-04-27 08:40:32 PDT

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 5 Chris Dumez 2021-04-27 08:44:01 PDT

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.

Comment 6 Sihui Liu 2021-04-27 09:09:09 PDT

(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 7 Darin Adler 2021-04-27 09:13:38 PDT

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 8 Chris Dumez 2021-04-27 09:14:54 PDT

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();
```

Comment 9 Chris Dumez 2021-04-27 09:18:31 PDT

(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?

Comment 10 Chris Dumez 2021-04-27 09:19:16 PDT

By the way, I did not mean to overwrite Darin's r+. I had a bugzilla conflict :/ Sorry about that.

Comment 11 Darin Adler 2021-04-27 09:30:53 PDT

Wow, I was totally wrong about random(); please don’t switch to it. I was confusing it with takeAny().

Comment 12 Chris Dumez 2021-04-27 09:33:12 PDT

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 13 Geoffrey Garen 2021-04-27 11:16:27 PDT

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.

Comment 14 Fujii Hironori 2021-04-27 13:46:41 PDT

Created attachment 427191 [details]
Patch

Comment 15 Chris Dumez 2021-04-27 13:51:37 PDT

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 16 Chris Dumez 2021-04-27 13:52:40 PDT

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 17 Fujii Hironori 2021-04-27 14:11:54 PDT

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.

Comment 18 Fujii Hironori 2021-04-27 14:12:29 PDT

Created attachment 427195 [details]
Patch

Comment 19 Chris Dumez 2021-04-27 14:28:51 PDT

Comment on attachment 427195 [details]
Patch

Oh yes. That's because of the `.values()` call. My bad.

Comment 20 Darin Adler 2021-04-27 14:49:57 PDT

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).

Comment 21 EWS 2021-04-27 15:11:24 PDT

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].