104922 2012-12-13 09:13:27 -0800 [WK2] Frequent NetworkProcess crashes due to null connection when closing tabs 2012-12-13 10:34:08 -0800 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ap ap beidson oldest_to_newest 790660 0 ap 2012-12-13 09:13:27 -0800 When a tab is closed, WebProcess goes away, but NetworkProcess will still sometimes attempt to send messages to it if there were outstanding loads. NetworkResourceLoader and NetworkConnectionToWebProcess both have m_connection that is zeroed on on connection close, and that causes null pointer crashes in IPC machinery. This is easy to reproduce on sites that use long-standing connections, like gmail. <rdar://problem/12870065> 790665 1 179283 ap 2012-12-13 09:17:36 -0800 Created attachment 179283 proposed patch 790681 2 179283 oliver 2012-12-13 09:30:32 -0800 Comment on attachment 179283 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=179283&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-147 > - m_connection->unregisterObserver(this); Why arbitrarily delta this and not have if (RefPtr<blah> connection = m_connection) connection->unregisterObserver(this) ? > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-148 > - m_connection = 0; If connection is null this doesn't hurt, does it hurt to explicitly clear m_connection if it is non-null? 790711 3 179283 ap 2012-12-13 10:04:17 -0800 Comment on attachment 179283 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=179283&action=review >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-147 >> - m_connection->unregisterObserver(this); > > Why arbitrarily delta this and not have if (RefPtr<blah> connection = m_connection) connection->unregisterObserver(this) ? There is another unregisterObserver call in destructor, there is no reason to do this twice. >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-148 >> - m_connection = 0; > > If connection is null this doesn't hurt, does it hurt to explicitly clear m_connection if it is non-null? I'm not sure if I understand the question. m_connection being null is why this crash happens, so it definitely hurts to clear it. 790741 4 ap 2012-12-13 10:34:08 -0800 Committed <http://trac.webkit.org/changeset/137610>. 179283 2012-12-13 09:17:36 -0800 2012-12-13 10:32:45 -0800 proposed patch ClosedConnection.txt text/plain 3656 ap SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzNzYwMCkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTEyLTEzICBBbGV4ZXkg UHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgorCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8x Mjg3MDA2NT4gRnJlcXVlbnQgTmV0d29ya1Byb2Nlc3MgY3Jhc2hlcyBkdWUgdG8gbnVsbCBjb25u ZWN0aW9uIHdoZW4gY2xvc2luZyB0YWJzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMDQ5MjIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBEb24ndCB6ZXJvIG91dCBjb25uZWN0aW9uIHBvaW50ZXJzIHdoZW4g YSBjb25uZWN0aW9uIGNsb3Nlcy4gVGhlcmUgZG9lc24ndCBhcHBlYXIKKyAgICAgICAgdG8gYmUg YW55IHJlYXNvbiB0byBkbyB0aGlzLgorCisgICAgICAgIFRlc3RlZCBieSBoaXR0aW5nIGJyZWFr cG9pbnRzIGluIGFsbCByZWxldmFudCBvYmplY3RzIGRlc3RydWN0b3JzIHRvIGNvbmZpcm0gdGhh dAorICAgICAgICB0aGlzIGRvZXNuJ3QgaW50cm9kdWNlIGxlYWtzLgorCisgICAgICAgICogTmV0 d29ya1Byb2Nlc3MvTmV0d29ya0Nvbm5lY3Rpb25Ub1dlYlByb2Nlc3MuY3BwOgorICAgICAgICAo V2ViS2l0OjpOZXR3b3JrQ29ubmVjdGlvblRvV2ViUHJvY2Vzczo6fk5ldHdvcmtDb25uZWN0aW9u VG9XZWJQcm9jZXNzKToKKyAgICAgICAgKFdlYktpdDo6TmV0d29ya0Nvbm5lY3Rpb25Ub1dlYlBy b2Nlc3M6OmRpZENsb3NlKToKKyAgICAgICAgKiBOZXR3b3JrUHJvY2Vzcy9OZXR3b3JrUmVzb3Vy Y2VMb2FkZXIuY3BwOgorICAgICAgICAoV2ViS2l0OjpOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmNv bm5lY3Rpb25Ub1dlYlByb2Nlc3NEaWRDbG9zZSk6CisKIDIwMTItMTItMTMgIFRoaWFnbyBNYXJj b3MgUC4gU2FudG9zICA8dGhpYWdvLnNhbnRvc0BpbnRlbC5jb20+CiAKICAgICAgICAgW0VGTF0g VXBkYXRlIHRoZSB3YXkgd2Ugc2V0IHRoZSBjb29raWVzIHBvbGljeQpJbmRleDogU291cmNlL1dl YktpdDIvTmV0d29ya1Byb2Nlc3MvTmV0d29ya0Nvbm5lY3Rpb25Ub1dlYlByb2Nlc3MuY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL05ldHdvcmtQcm9jZXNzL05ldHdvcmtDb25uZWN0 aW9uVG9XZWJQcm9jZXNzLmNwcAkocmV2aXNpb24gMTM3NTE0KQorKysgU291cmNlL1dlYktpdDIv TmV0d29ya1Byb2Nlc3MvTmV0d29ya0Nvbm5lY3Rpb25Ub1dlYlByb2Nlc3MuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC01Niw3ICs1Niw2IEBAIE5ldHdvcmtDb25uZWN0aW9uVG9XZWJQcm9jZXNzOjpO ZXR3b3JrQ28KIAogTmV0d29ya0Nvbm5lY3Rpb25Ub1dlYlByb2Nlc3M6On5OZXR3b3JrQ29ubmVj dGlvblRvV2ViUHJvY2VzcygpCiB7Ci0gICAgQVNTRVJUKCFtX2Nvbm5lY3Rpb24pOwogICAgIEFT U0VSVChtX29ic2VydmVycy5pc0VtcHR5KCkpOwogfQogCkBAIC05OSwyMCArOTgsMjAgQEAgdm9p ZCBOZXR3b3JrQ29ubmVjdGlvblRvV2ViUHJvY2Vzczo6ZGlkUgogCiB2b2lkIE5ldHdvcmtDb25u ZWN0aW9uVG9XZWJQcm9jZXNzOjpkaWRDbG9zZShDb3JlSVBDOjpDb25uZWN0aW9uKikKIHsKLSAg ICAvLyBQcm90ZWN0IG91cnNlbGYgYXMgd2UgbWlnaHQgYmUgb3RoZXJ3aXNlIGJlIGRlbGV0ZWQg ZHVyaW5nIHRoaXMgZnVuY3Rpb24KKyAgICAvLyBQcm90ZWN0IG91cnNlbGYgYXMgd2UgbWlnaHQg YmUgb3RoZXJ3aXNlIGJlIGRlbGV0ZWQgZHVyaW5nIHRoaXMgZnVuY3Rpb24uCiAgICAgUmVmUHRy PE5ldHdvcmtDb25uZWN0aW9uVG9XZWJQcm9jZXNzPiBwcm90ZWN0b3IodGhpcyk7CiAgICAgCiAg ICAgTmV0d29ya1Byb2Nlc3M6OnNoYXJlZCgpLnJlbW92ZU5ldHdvcmtDb25uZWN0aW9uVG9XZWJQ cm9jZXNzKHRoaXMpOwogICAgIAorICAgIC8vIEZJWE1FIChOZXR3b3JrUHJvY2Vzcyk6IFdlIG1p Z2h0IGNvbnNpZGVyIGFjdGl2ZWx5IGNsZWFyaW5nIG91dCBhbGwgcmVxdWVzdHMgZm9yIHRoaXMg Y29ubmVjdGlvbi4KKyAgICAvLyBCdXQgdGhhdCBtaWdodCBub3QgYmUgbmVjZXNzYXJ5IGFzIHRo ZSBvYnNlcnZlciBtZWNoYW5pc20gdXNlZCBhYm92ZSBpcyBtdWNoIG1vcmUgZGlyZWN0LgorCiAg ICAgVmVjdG9yPE5ldHdvcmtDb25uZWN0aW9uVG9XZWJQcm9jZXNzT2JzZXJ2ZXIqPiBvYnNlcnZl cnM7CiAgICAgY29weVRvVmVjdG9yKG1fb2JzZXJ2ZXJzLCBvYnNlcnZlcnMpOwogICAgIGZvciAo c2l6ZV90IGkgPSAwOyBpIDwgb2JzZXJ2ZXJzLnNpemUoKTsgKytpKQogICAgICAgICBvYnNlcnZl cnNbaV0tPmNvbm5lY3Rpb25Ub1dlYlByb2Nlc3NEaWRDbG9zZSh0aGlzKTsKLSAgICAKLSAgICAv LyBGSVhNRSAoTmV0d29ya1Byb2Nlc3MpOiBXZSBtaWdodCBjb25zaWRlciBhY3RpdmVseSBjbGVh cmluZyBvdXQgYWxsIHJlcXVlc3RzIGZvciB0aGlzIGNvbm5lY3Rpb24uCi0gICAgLy8gQnV0IHRo YXQgbWlnaHQgbm90IGJlIG5lY2Vzc2FyeSBhcyB0aGUgb2JzZXJ2ZXIgbWVjaGFuaXNtIHVzZWQg YWJvdmUgaXMgbXVjaCBtb3JlIGRpcmVjdC4KLSAgICAKLSAgICBtX2Nvbm5lY3Rpb24gPSAwOwor CisgICAgLy8gVGhlIG9iamVjdCBtYXkgYmUgZGVzdHJveWVkIG5vdy4KIH0KIAogdm9pZCBOZXR3 b3JrQ29ubmVjdGlvblRvV2ViUHJvY2Vzczo6ZGlkUmVjZWl2ZUludmFsaWRNZXNzYWdlKENvcmVJ UEM6OkNvbm5lY3Rpb24qLCBDb3JlSVBDOjpTdHJpbmdSZWZlcmVuY2UsIENvcmVJUEM6OlN0cmlu Z1JlZmVyZW5jZSkKSW5kZXg6IFNvdXJjZS9XZWJLaXQyL05ldHdvcmtQcm9jZXNzL05ldHdvcmtS ZXNvdXJjZUxvYWRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvTmV0d29ya1By b2Nlc3MvTmV0d29ya1Jlc291cmNlTG9hZGVyLmNwcAkocmV2aXNpb24gMTM3NTE0KQorKysgU291 cmNlL1dlYktpdDIvTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jlc291cmNlTG9hZGVyLmNwcAkod29y a2luZyBjb3B5KQpAQCAtMTQ0LDggKzE0NCw2IEBAIHZvaWQgTmV0d29ya1Jlc291cmNlTG9hZGVy OjpzdG9wKCkKIHZvaWQgTmV0d29ya1Jlc291cmNlTG9hZGVyOjpjb25uZWN0aW9uVG9XZWJQcm9j ZXNzRGlkQ2xvc2UoTmV0d29ya0Nvbm5lY3Rpb25Ub1dlYlByb2Nlc3MqIGNvbm5lY3Rpb24pCiB7 CiAgICAgQVNTRVJUX0FSRyhjb25uZWN0aW9uLCBjb25uZWN0aW9uID09IG1fY29ubmVjdGlvbi5n ZXQoKSk7Ci0gICAgbV9jb25uZWN0aW9uLT51bnJlZ2lzdGVyT2JzZXJ2ZXIodGhpcyk7Ci0gICAg bV9jb25uZWN0aW9uID0gMDsKIH0KIAogdm9pZCBOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmRpZFJl Y2VpdmVSZXNwb25zZShSZXNvdXJjZUhhbmRsZSosIGNvbnN0IFJlc291cmNlUmVzcG9uc2UmIHJl c3BvbnNlKQo=