Bug 199208

Summary: Add HashMap.contains check in SessionStorageNamespace::removeAllowedConnection
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: cdumez, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=198966
Attachments:
Description Flags
Patch none

Alex Christensen
Reported 2019-06-25 15:48:02 PDT
Add HashMap.contains check in SessionStorageNamespace::removeAllowedConnection
Attachments
Patch (1.88 KB, patch)
2019-06-25 15:49 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 2019-06-25 15:49:56 PDT
Created attachment 372870 [details] Patch
Alex Christensen
Comment 2 2019-06-25 15:49:58 PDT
<rdar://problem/32106147>
Chris Dumez
Comment 3 2019-06-25 15:55:22 PDT
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.
Chris Dumez
Comment 4 2019-06-25 15:58:41 PDT
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.
Alex Christensen
Comment 5 2019-06-25 16:27:31 PDT
*** This bug has been marked as a duplicate of bug 198966 ***
Alex Christensen
Comment 6 2019-06-25 16:28:10 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.