Bug 199208 – Add HashMap.contains check in SessionStorageNamespace::removeAllowedConnection

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Alex Christensen]

Add HashMap.contains check in SessionStorageNamespace::removeAllowedConnection

[Alex Christensen]

Created attachment 372870 [details]
Patch

[Alex Christensen]

<rdar://problem/32106147>

[Chris Dumez]

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

*** This bug has been marked as a duplicate of bug 198966 ***

[Alex Christensen]

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.