152213 2015-12-11 23:27:43 -0800 Web Inspector: Too many derefs when RemoteInspectorXPCConnection fails to validate connection 2015-12-11 23:44:47 -0800 1 1 1 Unclassified WebKit Web Inspector Other All All RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck bburg commit-queue graouts joepeck keith_miller mark.lam mattbaker msaboff nvasilyev rniwa saam timothy webkit-bug-importer oldest_to_newest 1148670 0 joepeck 2015-12-11 23:27:43 -0800 * SUMMARY Too many derefs when RemoteInspectorXPCConnection fails to validate connection. We call closeOnQueue() and deref(), but the closeOnQueue will cause an XPC_ERROR_CONNECTION_INVALID to deref() the connection again. We should only deref in one place, this code should just trigger the close. 1148671 1 webkit-bug-importer 2015-12-11 23:27:56 -0800 <rdar://problem/23870973> 1148672 2 267223 joepeck 2015-12-11 23:29:34 -0800 Created attachment 267223 [PATCH] Proposed Fix 1148673 3 267223 rniwa 2015-12-11 23:33:00 -0800 Comment on attachment 267223 [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267223&action=review rs=me > Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:-182 > - m_closed = true; > - m_client = nullptr; It looks like we always evaluate these two statements before calling closeOnQueue() elsewhere. Is it really safe not to set these values? 1148675 4 joepeck 2015-12-11 23:43:07 -0800 (In reply to comment #3) > Comment on attachment 267223 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267223&action=review > > rs=me > > > Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:-182 > > - m_closed = true; > > - m_client = nullptr; > > It looks like we always evaluate these two statements before calling > closeOnQueue() elsewhere. > Is it really safe not to set these values? Thanks for the careful review! I don't think it is necessary here. We don't want to set m_client = nullptr yet, because we do want to inform the client that we are closing, which will happen in the handling of XPC_ERROR_CONNECTION_INVALID. The case of `close` and `closeFromMessage` it is the client doing the closing, so they don't need to be informed that the connection is closing. We could set m_close, but we might as well delay that until when we actually do the closing with XPC_ERROR_CONNECTION_INVALID triggered by this. So waiting and closing there feels more natural. 1148677 5 joepeck 2015-12-11 23:44:47 -0800 <http://trac.webkit.org/changeset/194005> 267223 2015-12-11 23:29:34 -0800 2015-12-11 23:33:00 -0800 [PATCH] Proposed Fix fix.patch text/plain 2010 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGU5ZWU4NWQuLjFhYWI1MTcgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE1LTEyLTExICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogVG9vIG1h bnkgZGVyZWZzIHdoZW4gUmVtb3RlSW5zcGVjdG9yWFBDQ29ubmVjdGlvbiBmYWlscyB0byB2YWxp ZGF0ZSBjb25uZWN0aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xNTIyMTMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIGluc3BlY3Rvci9yZW1vdGUvUmVtb3RlSW5zcGVjdG9yWFBDQ29ubmVjdGlvbi5t bToKKyAgICAgICAgKEluc3BlY3Rvcjo6UmVtb3RlSW5zcGVjdG9yWFBDQ29ubmVjdGlvbjo6aGFu ZGxlRXZlbnQpOgorICAgICAgICBXZSBzaG91bGQganVzdCBjbG9zZSB0aGUgWFBDIGNvbm5lY3Rp b24gdHJpZ2dlcmluZyBYUENfRVJST1JfQ09OTkVDVElPTl9JTlZBTElECisgICAgICAgIHdoaWNo IHdpbGwgdGhlbiBncmFjZWZ1bCB0ZWFyZG93biB0aGUgY29ubmVjdGlvbiBhcyBleHBlY3RlZC4K KwogMjAxNS0xMi0xMSAgQmVuamFtaW4gUG91bGFpbiAgPGJwb3VsYWluQGFwcGxlLmNvbT4KIAog ICAgICAgICBbSlNDXSBBZGQgRmxvYXRpbmcgUG9pbnQgQWJzKCkgdG8gQjMKZGlmZiAtLWdpdCBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnNwZWN0b3IvcmVtb3RlL1JlbW90ZUluc3BlY3RvclhQ Q0Nvbm5lY3Rpb24ubW0gYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaW5zcGVjdG9yL3JlbW90ZS9S ZW1vdGVJbnNwZWN0b3JYUENDb25uZWN0aW9uLm1tCmluZGV4IGRlZTc0MGQuLmVlMmQ1NDYgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnNwZWN0b3IvcmVtb3RlL1JlbW90ZUlu c3BlY3RvclhQQ0Nvbm5lY3Rpb24ubW0KKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2luc3Bl Y3Rvci9yZW1vdGUvUmVtb3RlSW5zcGVjdG9yWFBDQ29ubmVjdGlvbi5tbQpAQCAtMTczLDE2ICsx NzMsOSBAQCB2b2lkIFJlbW90ZUluc3BlY3RvclhQQ0Nvbm5lY3Rpb246OmhhbmRsZUV2ZW50KHhw Y19vYmplY3RfdCBvYmplY3QpCiAgICAgICAgIGF1ZGl0X3Rva2VuX3QgdG9rZW47CiAgICAgICAg IHhwY19jb25uZWN0aW9uX2dldF9hdWRpdF90b2tlbihtX2Nvbm5lY3Rpb24sICZ0b2tlbik7CiAg ICAgICAgIGlmICghYXVkaXRUb2tlbkhhc0VudGl0bGVtZW50KHRva2VuLCBAImNvbS5hcHBsZS5w cml2YXRlLndlYmluc3BlY3Rvci53ZWJpbnNwZWN0b3JkIikpIHsKLSAgICAgICAgICAgIHsKLSAg ICAgICAgICAgICAgICBzdGQ6OmxvY2tfZ3VhcmQ8TG9jaz4gbG9jayhtX211dGV4KTsKLSAgICAg ICAgICAgICAgICBpZiAobV9jbGllbnQpCi0gICAgICAgICAgICAgICAgICAgIG1fY2xpZW50LT54 cGNDb25uZWN0aW9uRmFpbGVkKHRoaXMpOwotCi0gICAgICAgICAgICAgICAgbV9jbG9zZWQgPSB0 cnVlOwotICAgICAgICAgICAgICAgIG1fY2xpZW50ID0gbnVsbHB0cjsKLSAgICAgICAgICAgICAg ICBjbG9zZU9uUXVldWUoKTsKLSAgICAgICAgICAgIH0KLSAgICAgICAgICAgIGRlcmVmKCk7Cisg ICAgICAgICAgICBzdGQ6OmxvY2tfZ3VhcmQ8TG9jaz4gbG9jayhtX211dGV4KTsKKyAgICAgICAg ICAgIC8vIFRoaXMgd2lsbCB0cmlnZ2VyIG9uZSBsYXN0IFhQQ19FUlJPUl9DT05ORUNUSU9OX0lO VkFMSUQgZXZlbnQgb24gdGhlIHF1ZXVlIGFuZCBkZXJlZiB1cy4KKyAgICAgICAgICAgIGNsb3Nl T25RdWV1ZSgpOwogICAgICAgICAgICAgcmV0dXJuOwogICAgICAgICB9CiAgICAgICAgIG1fdmFs aWRhdGVkID0gdHJ1ZTsK