Add HashMap.contains check in SessionStorageNamespace::removeAllowedConnection
Created attachment 372870 [details] Patch
<rdar://problem/32106147>
Comment on attachment 372870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372870&action=review > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:485 > + if (m_allowedConnections.contains(allowedConnection)) This change is very obscure. Based on the radar, it seems that it is trying to solve some kind of threading issue. However, looking at this code, somebody would be very tempted to refactor it to drop your contains check given that remove() is already a no-op when the key is not present. Also, it goes against our usual coding practices to do double look-up in the case where the key is actually in the map. I think we should find a good way to address the crash in the radar, I do not believe this is it.
Comment on attachment 372870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372870&action=review >> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:485 >> + if (m_allowedConnections.contains(allowedConnection)) > > This change is very obscure. Based on the radar, it seems that it is trying to solve some kind of threading issue. However, looking at this code, somebody would be very tempted to refactor it to drop your contains check given that remove() is already a no-op when the key is not present. > Also, it goes against our usual coding practices to do double look-up in the case where the key is actually in the map. > > I think we should find a good way to address the crash in the radar, I do not believe this is it. This seems potentially related to https://bugs.webkit.org/show_bug.cgi?id=198966 BTW.
*** This bug has been marked as a duplicate of bug 198966 ***
That's why I couldn't see a missing null check. All use is on the same serial queue, so adding a mutex wouldn't help here.