199208 2019-06-25 15:48:02 -0700 Add HashMap.contains check in SessionStorageNamespace::removeAllowedConnection 2019-06-25 16:28:10 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 198966 https://bugs.webkit.org/show_bug.cgi?id=198966 InRadar P2 Normal --- 1 achristensen achristensen cdumez webkit-bug-importer youennf oldest_to_newest 1547843 0 achristensen 2019-06-25 15:48:02 -0700 Add HashMap.contains check in SessionStorageNamespace::removeAllowedConnection 1547844 1 372870 achristensen 2019-06-25 15:49:56 -0700 Created attachment 372870 Patch 1547845 2 achristensen 2019-06-25 15:49:58 -0700 <rdar://problem/32106147> 1547848 3 372870 cdumez 2019-06-25 15:55:22 -0700 Comment on attachment 372870 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. 1547851 4 372870 cdumez 2019-06-25 15:58:41 -0700 Comment on attachment 372870 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. 1547869 5 achristensen 2019-06-25 16:27:31 -0700 *** This bug has been marked as a duplicate of bug 198966 *** 1547872 6 achristensen 2019-06-25 16:28:10 -0700 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. 372870 2019-06-25 15:49:56 -0700 2019-06-25 15:57:19 -0700 Patch bug-199208-20190625154955.patch text/plain 1929 achristensen U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ2ODA3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDM2MDY1NDBiM2RmY2Q0MWRj YTk4NjJkNzdlNjRkZTkyMmUzMWYyYWQuLmY2MTFhNmIwOGUzNjE4Y2I2MmI2OTYwOTg4ZWI2ZjY1 NWIyZmQ1YWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTktMDYtMjUgIEFsZXggQ2hy aXN0ZW5zZW4gIDxhY2hyaXN0ZW5zZW5Ad2Via2l0Lm9yZz4KKworICAgICAgICBBZGQgSGFzaE1h cC5jb250YWlucyBjaGVjayBpbiBTZXNzaW9uU3RvcmFnZU5hbWVzcGFjZTo6cmVtb3ZlQWxsb3dl ZENvbm5lY3Rpb24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE5OTIwOAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzIxMDYxNDc+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBOZXR3b3JrUHJvY2Vzcy9X ZWJTdG9yYWdlL1N0b3JhZ2VNYW5hZ2VyLmNwcDoKKyAgICAgICAgKFdlYktpdDo6U3RvcmFnZU1h bmFnZXI6OlNlc3Npb25TdG9yYWdlTmFtZXNwYWNlOjpyZW1vdmVBbGxvd2VkQ29ubmVjdGlvbik6 CisgICAgICAgIHJlbW92ZUFsbG93ZWRDb25uZWN0aW9uIGlzIGNhbGxlZCB3aGVuIGEgcGFnZSBp cyByZW1vdmVkIGFuZCB3aGVuIGEgY29ubmVjdGlvbiBpcyBjbG9zZWQsIGFuZCBzb21ldGltZXMg dGhleSBib3RoIGhhcHBlbi4KKyAgICAgICAgV2hlbiBpdCBkb2VzIGhhcHBlbiwgdGhlcmUncyBu b3RoaW5nIHRvIHJlbW92ZS4KKwogMjAxOS0wNi0yNSAgQWxleCBDaHJpc3RlbnNlbiAgPGFjaHJp c3RlbnNlbkB3ZWJraXQub3JnPgogCiAgICAgICAgIE1ha2UgSFRUUENvb2tpZUFjY2VwdFBvbGlj eSBhbiBlbnVtIGNsYXNzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNz L1dlYlN0b3JhZ2UvU3RvcmFnZU1hbmFnZXIuY3BwIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJv Y2Vzcy9XZWJTdG9yYWdlL1N0b3JhZ2VNYW5hZ2VyLmNwcAppbmRleCA1MGZkZTkwZDYxNWVkNTg4 OTYwMTVjNjlhMmIwOTE1MmZkMmZiMjdiLi44YWJhYmNhNDIxZmIwYmRjMDllMzVkYzk0MmMxYzBi YTFiMmE0Y2I0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL1dlYlN0 b3JhZ2UvU3RvcmFnZU1hbmFnZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nl c3MvV2ViU3RvcmFnZS9TdG9yYWdlTWFuYWdlci5jcHAKQEAgLTQ4Miw3ICs0ODIsOCBAQCB2b2lk IFN0b3JhZ2VNYW5hZ2VyOjpTZXNzaW9uU3RvcmFnZU5hbWVzcGFjZTo6YWRkQWxsb3dlZENvbm5l Y3Rpb24oSVBDOjpDb25uZWN0aQogdm9pZCBTdG9yYWdlTWFuYWdlcjo6U2Vzc2lvblN0b3JhZ2VO YW1lc3BhY2U6OnJlbW92ZUFsbG93ZWRDb25uZWN0aW9uKElQQzo6Q29ubmVjdGlvbjo6VW5pcXVl SUQgYWxsb3dlZENvbm5lY3Rpb24pCiB7CiAgICAgQVNTRVJUKG1fYWxsb3dlZENvbm5lY3Rpb25z LmNvbnRhaW5zKGFsbG93ZWRDb25uZWN0aW9uKSk7Ci0gICAgbV9hbGxvd2VkQ29ubmVjdGlvbnMu cmVtb3ZlKGFsbG93ZWRDb25uZWN0aW9uKTsKKyAgICBpZiAobV9hbGxvd2VkQ29ubmVjdGlvbnMu Y29udGFpbnMoYWxsb3dlZENvbm5lY3Rpb24pKQorICAgICAgICBtX2FsbG93ZWRDb25uZWN0aW9u cy5yZW1vdmUoYWxsb3dlZENvbm5lY3Rpb24pOwogfQogYXV0byBTdG9yYWdlTWFuYWdlcjo6U2Vz c2lvblN0b3JhZ2VOYW1lc3BhY2U6OmdldE9yQ3JlYXRlU3RvcmFnZUFyZWEoU2VjdXJpdHlPcmln aW5EYXRhJiYgc2VjdXJpdHlPcmlnaW4pIC0+IFJlZjxTdG9yYWdlQXJlYT4KIHsK