173739 2017-06-22 15:16:03 -0700 LibWebRTCSocketClient should not destroy its socket within signalClose callback 2017-06-23 13:43:46 -0700 1 1 1 Unclassified WebKit Media WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 youennf youennf achristensen bfulgham commit-queue ddkilzer eric.carlson webkit-bug-importer oldest_to_newest 1322082 0 youennf 2017-06-22 15:16:03 -0700 LibWebRTCSocketClient should not destroy its socket within signalClose callback 1322083 1 youennf 2017-06-22 15:16:17 -0700 rdar://problem/32922877 1322088 2 313666 youennf 2017-06-22 15:23:45 -0700 Created attachment 313666 Patch 1322305 3 313666 bfulgham 2017-06-23 09:21:32 -0700 Comment on attachment 313666 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313666&action=review > Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:141 > + m_rtcProvider.callOnRTCNetworkThread([socket = m_rtcProvider.takeSocket(m_identifier)] { }); How do we know this is sufficient lifespan? I don't think that anything really guarantees that the RTCNetworkThread doesn't wake up and do the destruction before the caller of LibWebRTCSocketClient::signalClose() has a chance to finish using the socket. It seems like the caller needs some way to keep 'socket' alive for as long as it needs to use it. 1322307 4 313666 bfulgham 2017-06-23 09:24:29 -0700 Comment on attachment 313666 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313666&action=review r=me >> Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:141 >> + m_rtcProvider.callOnRTCNetworkThread([socket = m_rtcProvider.takeSocket(m_identifier)] { }); > > How do we know this is sufficient lifespan? I don't think that anything really guarantees that the RTCNetworkThread doesn't wake up and do the destruction before the caller of LibWebRTCSocketClient::signalClose() has a chance to finish using the socket. It seems like the caller needs some way to keep 'socket' alive for as long as it needs to use it. Looking further, I guess the only caller of LibWebRTCSocketClient::signalClose is the WebRTCSocket message handler, which calls this method and doesn't do anything else. We might have a number of queued up messages to be handled that need socket, but by putting the destruction in the message queue, we guarantee that those tasks are done by the time we are allowed to destroy the socket. So I'm confident this change is correct. 1322321 5 youennf 2017-06-23 09:45:48 -0700 (In reply to Brent Fulgham from comment #4) > Comment on attachment 313666 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313666&action=review > > r=me > > >> Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:141 > >> + m_rtcProvider.callOnRTCNetworkThread([socket = m_rtcProvider.takeSocket(m_identifier)] { }); > > > > How do we know this is sufficient lifespan? I don't think that anything really guarantees that the RTCNetworkThread doesn't wake up and do the destruction before the caller of LibWebRTCSocketClient::signalClose() has a chance to finish using the socket. It seems like the caller needs some way to keep 'socket' alive for as long as it needs to use it. > > Looking further, I guess the only caller of > LibWebRTCSocketClient::signalClose is the WebRTCSocket message handler, > which calls this method and doesn't do anything else. > > We might have a number of queued up messages to be handled that need socket, > but by putting the destruction in the message queue, we guarantee that those > tasks are done by the time we are allowed to destroy the socket. > > So I'm confident this change is correct. Right, maybe I should beef up the change log? 1322410 6 313666 commit-queue 2017-06-23 13:43:45 -0700 Comment on attachment 313666 Patch Clearing flags on attachment: 313666 Committed r218759: <http://trac.webkit.org/changeset/218759> 1322411 7 commit-queue 2017-06-23 13:43:46 -0700 All reviewed patches have been landed. Closing bug. 313666 2017-06-22 15:23:45 -0700 2017-06-23 13:43:45 -0700 Patch bug-173739-20170622152344.patch text/plain 1907 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjE4Njk1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNDYyNDg2NjlmMDI5NWUz NzEzODU1NWM5OTk0MjJkZTc0M2E2NGNjNy4uNjMwZTdiZGRhNDA1MzQ1Y2I1NGNlNzFiNGM5ZTY1 ZDQ5ZGI3MzAxZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE3LTA2LTIyICBZb3Vl bm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNvbT4KKworICAgICAgICBMaWJXZWJSVENTb2NrZXRD bGllbnQgc2hvdWxkIG5vdCBkZXN0cm95IGl0cyBzb2NrZXQgd2l0aGluIHNpZ25hbENsb3NlIGNh bGxiYWNrCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x NzM3MzkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAq IE5ldHdvcmtQcm9jZXNzL3dlYnJ0Yy9MaWJXZWJSVENTb2NrZXRDbGllbnQuY3BwOgorICAgICAg ICAoV2ViS2l0OjpMaWJXZWJSVENTb2NrZXRDbGllbnQ6OnNpZ25hbENsb3NlKTogRGVzdHJveSAn dGhpcycgYXN5bmNocm9ub3VzbHkgdG8gbWFrZSB0aGUgY2FsbGVyIG9mIHNpZ25hbENsb3NlIHZh bGlkIHVudGlsIGl0IGlzIG5vdCB1c2VkLgorCiAyMDE3LTA2LTIyICBDYXJsb3MgR2FyY2lhIENh bXBvcyAgPGNnYXJjaWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBbV1BFXSBEb3dubG9hZHMgbmV2 ZXIgaGF2ZSBhIHdlYiB2aWV3IGFzc29jaWF0ZWQgaW4gV1BFCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViS2l0Mi9OZXR3b3JrUHJvY2Vzcy93ZWJydGMvTGliV2ViUlRDU29ja2V0Q2xpZW50LmNwcCBi L1NvdXJjZS9XZWJLaXQyL05ldHdvcmtQcm9jZXNzL3dlYnJ0Yy9MaWJXZWJSVENTb2NrZXRDbGll bnQuY3BwCmluZGV4IGNkMDMyY2YyZTdmMjVhYTY4YmQ4MzkwYTQ0NmM0OTk3YTc2NDAyMTMuLjM2 MThmZDY3YThlYzNiZGEyZGMwNzk3ODBhNWMzYjYxNWZiNGRhOWMgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJLaXQyL05ldHdvcmtQcm9jZXNzL3dlYnJ0Yy9MaWJXZWJSVENTb2NrZXRDbGllbnQuY3Bw CisrKyBiL1NvdXJjZS9XZWJLaXQyL05ldHdvcmtQcm9jZXNzL3dlYnJ0Yy9MaWJXZWJSVENTb2Nr ZXRDbGllbnQuY3BwCkBAIC0xMzYsNyArMTM2LDkgQEAgdm9pZCBMaWJXZWJSVENTb2NrZXRDbGll bnQ6OnNpZ25hbENsb3NlKHJ0Yzo6QXN5bmNQYWNrZXRTb2NrZXQqIHNvY2tldCwgaW50IGVycm8K ICAgICBtX3J0Y1Byb3ZpZGVyLnNlbmRGcm9tTWFpblRocmVhZChbaWRlbnRpZmllciA9IG1faWRl bnRpZmllciwgZXJyb3JdKElQQzo6Q29ubmVjdGlvbiYgY29ubmVjdGlvbikgewogICAgICAgICBj b25uZWN0aW9uLnNlbmQoTWVzc2FnZXM6OldlYlJUQ1NvY2tldDo6U2lnbmFsQ2xvc2UoZXJyb3Ip LCBpZGVudGlmaWVyKTsKICAgICB9KTsKLSAgICBtX3J0Y1Byb3ZpZGVyLnRha2VTb2NrZXQobV9p ZGVudGlmaWVyKTsKKyAgICAvLyBXZSB3YW50IHRvIHJlbW92ZSAndGhpcycgZnJvbSB0aGUgc29j a2V0IG1hcCBub3cgYnV0IHdlIHdpbGwgZGVzdHJveSBpdCBhc3luY2hyb25vdXNseQorICAgIC8v IHNvIHRoYXQgdGhlIHNvY2tldCBwYXJhbWV0ZXIgb2Ygc2lnbmFsQ2xvc2UgcmVtYWlucyBhbGl2 ZSBhcyB0aGUgY2FsbGVyIG9mIHNpZ25hbENsb3NlIG1heSBhY3R1YWxseSBiZWluZyB1c2luZyBp dCBhZnRlcndhcmRzLgorICAgIG1fcnRjUHJvdmlkZXIuY2FsbE9uUlRDTmV0d29ya1RocmVhZChb c29ja2V0ID0gbV9ydGNQcm92aWRlci50YWtlU29ja2V0KG1faWRlbnRpZmllcildIHsgfSk7CiB9 CiAKIH0gLy8gbmFtZXNwYWNlIFdlYktpdAo=