32626 2009-12-16 12:05:22 -0800 Move some code from Database::~Database() to Database::close() 2009-12-18 17:52:56 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 dumi dumi dglazkov dimich fishd michaeln webkit.review.bot oldest_to_newest 172450 0 dumi 2009-12-16 12:05:22 -0800 Currently, DatabaseTracker is notified that a Database is closing/closed in the Database's destructor. At that point, we are very limited in what we can do, because the Database object is being destroyed. Instead, it seems like the correct thing to do is notify the DatabaseTracker that a database is being closed in the close() method. 172467 1 45004 dumi 2009-12-16 12:26:07 -0800 Created attachment 45004 patch 172468 2 webkit.review.bot 2009-12-16 12:27:45 -0800 style-queue ran check-webkit-style on attachment 45004 without any errors. 172605 3 michaeln 2009-12-16 17:58:51 -0800 355 if (m_document->databaseThread()) 356 m_document->databaseThread()->unscheduleDatabaseTasks(this); Given the ASSERT on line 335, looks like we don't need to test for null'ness here. 359 m_document->removeOpenDatabase(this); This looks like a threading bug waiting to happen? The doc->addOpenDatabase() method is being called on the main thread, but the call to doc->removeOpenDatabase() happens on the db thread (per the ASSERT on line 336). But the underlying collection being used by the Document is not thread safe. This call to remove() used to be in the dtor, which contains code that indicates it can happen on a background thread in theory. In practice, who knows how often that does happen. This change as coded will guarantee that the underlying set is accessed in a non-thread-safe fashion. I think you need to poke at this in some way. 172615 4 45031 dumi 2009-12-16 18:41:27 -0800 Created attachment 45031 patch (In reply to comment #3) > 355 if (m_document->databaseThread()) > 356 m_document->databaseThread()->unscheduleDatabaseTasks(this); > > Given the ASSERT on line 335, looks like we don't need to test for null'ness > here. good point. removed. > 359 m_document->removeOpenDatabase(this); > > This looks like a threading bug waiting to happen? The doc->addOpenDatabase() > method is being called on the main thread, but the call to > doc->removeOpenDatabase() happens on the db thread (per the ASSERT on line > 336). But the underlying collection being used by the Document is not thread > safe. > > This call to remove() used to be in the dtor, which contains code that > indicates it can happen on a background thread in theory. In practice, who > knows how often that does happen. > > This change as coded will guarantee that the underlying set is accessed in a > non-thread-safe fashion. I think you need to poke at this in some way. you're right. i think this should be fixed now, please take another look. 172616 5 webkit.review.bot 2009-12-16 18:42:11 -0800 style-queue ran check-webkit-style on attachment 45031 without any errors. 172617 6 michaeln 2009-12-16 19:09:08 -0800 > you're right. i think this should be fixed now, please take another look. 365 DatabaseTracker::tracker().removeOpenDatabase(this); 366 ref(); 367 callOnMainThread(documentRemoveOpenDatabase, this); that works for me 173234 7 dumi 2009-12-18 11:35:56 -0800 Can a reviewer please take a look at this patch? 173266 8 dimich 2009-12-18 12:28:02 -0800 r=me On landing, could you please add a small comment against the ref() to say where is a balancing deref()? 366 ref(); 367 callOnMainThread(documentRemoveOpenDatabase, this); 173268 9 dumi 2009-12-18 12:29:25 -0800 (In reply to comment #8) > r=me > > On landing, could you please add a small comment against the ref() to say where > is a balancing deref()? > > 366 ref(); > 367 callOnMainThread(documentRemoveOpenDatabase, this); Will do. Thanks for taking a look at this patch! 173440 10 dumi 2009-12-18 17:52:56 -0800 Landed as r52367. 45004 2009-12-16 12:26:07 -0800 2009-12-16 18:41:27 -0800 patch patch text/plain 3235 dumi SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MjIxMikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTkgQEAKKzIwMDktMTItMTYgIER1bWl0cnUgRGFuaWxpdWMgIDxkdW1pQGNocm9t aXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBNb3ZlIHNvbWUgY29kZSByZWxhdGVkIHRvIGRhdGFiYXNlIGNsb3NpbmcgZnJvbSB0aGUgZGVz dHJ1Y3RvciB0bworICAgICAgICB0aGUgY2xvc2UoKSBtZXRob2QuIFRoaXMgd291bGQgYWxsb3cg dXMgdG8gZG8gdGhpbmdzIHN1Y2ggYXMgcG9zdAorICAgICAgICB0YXNrcyB0byBvdGhlciB0aHJl YWRzIHdoZW4gYSBkYXRhYmFzZSBjbG9zZXMsIHdoaWNoIGNhbm5vdCBiZQorICAgICAgICBkb25l IG5vdywgYmVjYXVzZSB3ZSBjYW5ub3QgaW5jcmVtZW50IHRoZSByZWYgY291bnQgdG8gYSBkYXRh YmFzZQorICAgICAgICBvYmplY3Qgd2hlbiB3ZSdyZSBpbiBpdHMgZGVzdHJ1Y3Rvci4KKworICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzI2MjYKKworICAg ICAgICAqIHN0b3JhZ2UvRGF0YWJhc2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RGF0YWJhc2U6 On5EYXRhYmFzZSk6CisgICAgICAgIChXZWJDb3JlOjpEYXRhYmFzZTo6Y2xvc2UpOgorCiAyMDA5 LTEyLTE2ICBhbnRvbiBtdWhpbiAgPGFudG9ubUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCkluZGV4OiBXZWJDb3JlL3N0b3JhZ2UvRGF0YWJhc2UuY3Bw Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFdlYkNvcmUvc3RvcmFnZS9EYXRhYmFzZS5jcHAJKHJldmlzaW9uIDUy MjEyKQorKysgV2ViQ29yZS9zdG9yYWdlL0RhdGFiYXNlLmNwcAkod29ya2luZyBjb3B5KQpAQCAt MTk4LDEyICsxOTgsNiBAQCBzdGF0aWMgdm9pZCBkZXJlZkRvY3VtZW50KHZvaWQqIGRvY3VtZW50 CiAKIERhdGFiYXNlOjp+RGF0YWJhc2UoKQogewotICAgIGlmIChtX2RvY3VtZW50LT5kYXRhYmFz ZVRocmVhZCgpKQotICAgICAgICBtX2RvY3VtZW50LT5kYXRhYmFzZVRocmVhZCgpLT51bnNjaGVk dWxlRGF0YWJhc2VUYXNrcyh0aGlzKTsKLQotICAgIERhdGFiYXNlVHJhY2tlcjo6dHJhY2tlcigp LnJlbW92ZU9wZW5EYXRhYmFzZSh0aGlzKTsKLSAgICBtX2RvY3VtZW50LT5yZW1vdmVPcGVuRGF0 YWJhc2UodGhpcyk7Ci0KICAgICAvLyBEZXJlZiBtX2RvY3VtZW50IG9uIHRoZSBtYWluIHRocmVh ZC4KICAgICBjYWxsT25NYWluVGhyZWFkKGRlcmVmRG9jdW1lbnQsIG1fZG9jdW1lbnQucmVsZWFz ZSgpLnJlbGVhc2VSZWYoKSk7CiB9CkBAIC0zMzUsMjcgKzMyOSwzNCBAQCB2b2lkIERhdGFiYXNl OjptYXJrQXNEZWxldGVkQW5kQ2xvc2UoKQogCiB2b2lkIERhdGFiYXNlOjpjbG9zZSgpCiB7Ci0g ICAgaWYgKG1fb3BlbmVkKSB7Ci0gICAgICAgIEFTU0VSVChtX2RvY3VtZW50LT5kYXRhYmFzZVRo cmVhZCgpKTsKLSAgICAgICAgQVNTRVJUKGN1cnJlbnRUaHJlYWQoKSA9PSBkb2N1bWVudCgpLT5k YXRhYmFzZVRocmVhZCgpLT5nZXRUaHJlYWRJRCgpKTsKLSAgICAgICAgbV9zcWxpdGVEYXRhYmFz ZS5jbG9zZSgpOwotICAgICAgICBtX2RvY3VtZW50LT5kYXRhYmFzZVRocmVhZCgpLT5yZWNvcmRE YXRhYmFzZUNsb3NlZCh0aGlzKTsKLSAgICAgICAgbV9vcGVuZWQgPSBmYWxzZTsKLQotICAgICAg ICB7Ci0gICAgICAgICAgICBNdXRleExvY2tlciBsb2NrZXIoZ3VpZE11dGV4KCkpOwotCi0gICAg ICAgICAgICBIYXNoU2V0PERhdGFiYXNlKj4qIGhhc2hTZXQgPSBndWlkVG9EYXRhYmFzZU1hcCgp LmdldChtX2d1aWQpOwotICAgICAgICAgICAgQVNTRVJUKGhhc2hTZXQpOwotICAgICAgICAgICAg QVNTRVJUKGhhc2hTZXQtPmNvbnRhaW5zKHRoaXMpKTsKLSAgICAgICAgICAgIGhhc2hTZXQtPnJl bW92ZSh0aGlzKTsKLSAgICAgICAgICAgIGlmIChoYXNoU2V0LT5pc0VtcHR5KCkpIHsKLSAgICAg ICAgICAgICAgICBndWlkVG9EYXRhYmFzZU1hcCgpLnJlbW92ZShtX2d1aWQpOwotICAgICAgICAg ICAgICAgIGRlbGV0ZSBoYXNoU2V0OwotICAgICAgICAgICAgICAgIGd1aWRUb1ZlcnNpb25NYXAo KS5yZW1vdmUobV9ndWlkKTsKLSAgICAgICAgICAgIH0KKyAgICBpZiAoIW1fb3BlbmVkKQorICAg ICAgICByZXR1cm47CisKKyAgICBBU1NFUlQobV9kb2N1bWVudC0+ZGF0YWJhc2VUaHJlYWQoKSk7 CisgICAgQVNTRVJUKGN1cnJlbnRUaHJlYWQoKSA9PSBkb2N1bWVudCgpLT5kYXRhYmFzZVRocmVh ZCgpLT5nZXRUaHJlYWRJRCgpKTsKKyAgICBtX3NxbGl0ZURhdGFiYXNlLmNsb3NlKCk7CisgICAg bV9kb2N1bWVudC0+ZGF0YWJhc2VUaHJlYWQoKS0+cmVjb3JkRGF0YWJhc2VDbG9zZWQodGhpcyk7 CisgICAgbV9vcGVuZWQgPSBmYWxzZTsKKworICAgIHsKKyAgICAgICAgTXV0ZXhMb2NrZXIgbG9j a2VyKGd1aWRNdXRleCgpKTsKKworICAgICAgICBIYXNoU2V0PERhdGFiYXNlKj4qIGhhc2hTZXQg PSBndWlkVG9EYXRhYmFzZU1hcCgpLmdldChtX2d1aWQpOworICAgICAgICBBU1NFUlQoaGFzaFNl dCk7CisgICAgICAgIEFTU0VSVChoYXNoU2V0LT5jb250YWlucyh0aGlzKSk7CisgICAgICAgIGhh c2hTZXQtPnJlbW92ZSh0aGlzKTsKKyAgICAgICAgaWYgKGhhc2hTZXQtPmlzRW1wdHkoKSkgewor ICAgICAgICAgICAgZ3VpZFRvRGF0YWJhc2VNYXAoKS5yZW1vdmUobV9ndWlkKTsKKyAgICAgICAg ICAgIGRlbGV0ZSBoYXNoU2V0OworICAgICAgICAgICAgZ3VpZFRvVmVyc2lvbk1hcCgpLnJlbW92 ZShtX2d1aWQpOwogICAgICAgICB9CiAgICAgfQorCisgICAgaWYgKG1fZG9jdW1lbnQtPmRhdGFi YXNlVGhyZWFkKCkpCisgICAgICAgIG1fZG9jdW1lbnQtPmRhdGFiYXNlVGhyZWFkKCktPnVuc2No ZWR1bGVEYXRhYmFzZVRhc2tzKHRoaXMpOworCisgICAgRGF0YWJhc2VUcmFja2VyOjp0cmFja2Vy KCkucmVtb3ZlT3BlbkRhdGFiYXNlKHRoaXMpOworICAgIG1fZG9jdW1lbnQtPnJlbW92ZU9wZW5E YXRhYmFzZSh0aGlzKTsKIH0KIAogdm9pZCBEYXRhYmFzZTo6c3RvcCgpCg== 45031 2009-12-16 18:41:27 -0800 2009-12-18 12:26:37 -0800 patch patch text/plain 3522 dumi SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MjIyMikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTkgQEAKKzIwMDktMTItMTYgIER1bWl0cnUgRGFuaWxpdWMgIDxkdW1pQGNocm9t aXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBNb3ZlIHNvbWUgY29kZSByZWxhdGVkIHRvIGRhdGFiYXNlIGNsb3NpbmcgZnJvbSB0aGUgZGVz dHJ1Y3RvciB0bworICAgICAgICB0aGUgY2xvc2UoKSBtZXRob2QuIFRoaXMgd291bGQgYWxsb3cg dXMgdG8gZG8gdGhpbmdzIHN1Y2ggYXMgcG9zdAorICAgICAgICB0YXNrcyB0byBvdGhlciB0aHJl YWRzIHdoZW4gYSBkYXRhYmFzZSBjbG9zZXMsIHdoaWNoIGNhbm5vdCBiZQorICAgICAgICBkb25l IG5vdywgYmVjYXVzZSB3ZSBjYW5ub3QgaW5jcmVtZW50IHRoZSByZWYgY291bnQgdG8gYSBkYXRh YmFzZQorICAgICAgICBvYmplY3Qgd2hlbiB3ZSdyZSBpbiBpdHMgZGVzdHJ1Y3Rvci4KKworICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzI2MjYKKworICAg ICAgICAqIHN0b3JhZ2UvRGF0YWJhc2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RGF0YWJhc2U6 On5EYXRhYmFzZSk6CisgICAgICAgIChXZWJDb3JlOjpEYXRhYmFzZTo6Y2xvc2UpOgorCiAyMDA5 LTEyLTE2ICBNYXJrIFJvd2UgIDxtcm93ZUBhcHBsZS5jb20+CiAKICAgICAgICAgQnVpbGQgZml4 LiAgRGlzYWJsZSBkZWJ1ZyB2YXJpYW50cyBvZiBXZWJLaXQgZnJhbWV3b3Jrcy4KSW5kZXg6IFdl YkNvcmUvc3RvcmFnZS9EYXRhYmFzZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9zdG9yYWdl L0RhdGFiYXNlLmNwcAkocmV2aXNpb24gNTIyMjIpCisrKyBXZWJDb3JlL3N0b3JhZ2UvRGF0YWJh c2UuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xOTgsMTIgKzE5OCw2IEBAIHN0YXRpYyB2b2lkIGRl cmVmRG9jdW1lbnQodm9pZCogZG9jdW1lbnQKIAogRGF0YWJhc2U6On5EYXRhYmFzZSgpCiB7Ci0g ICAgaWYgKG1fZG9jdW1lbnQtPmRhdGFiYXNlVGhyZWFkKCkpCi0gICAgICAgIG1fZG9jdW1lbnQt PmRhdGFiYXNlVGhyZWFkKCktPnVuc2NoZWR1bGVEYXRhYmFzZVRhc2tzKHRoaXMpOwotCi0gICAg RGF0YWJhc2VUcmFja2VyOjp0cmFja2VyKCkucmVtb3ZlT3BlbkRhdGFiYXNlKHRoaXMpOwotICAg IG1fZG9jdW1lbnQtPnJlbW92ZU9wZW5EYXRhYmFzZSh0aGlzKTsKLQogICAgIC8vIERlcmVmIG1f ZG9jdW1lbnQgb24gdGhlIG1haW4gdGhyZWFkLgogICAgIGNhbGxPbk1haW5UaHJlYWQoZGVyZWZE b2N1bWVudCwgbV9kb2N1bWVudC5yZWxlYXNlKCkucmVsZWFzZVJlZigpKTsKIH0KQEAgLTMzMywy OSArMzI3LDQ0IEBAIHZvaWQgRGF0YWJhc2U6Om1hcmtBc0RlbGV0ZWRBbmRDbG9zZSgpCiAgICAg c3luY2hyb25pemVyLndhaXRGb3JUYXNrQ29tcGxldGlvbigpOwogfQogCitzdGF0aWMgdm9pZCBk b2N1bWVudFJlbW92ZU9wZW5EYXRhYmFzZSh2b2lkKiBjb250ZXh0KQoreworICAgIEFTU0VSVChp c01haW5UaHJlYWQoKSk7CisgICAgRGF0YWJhc2UqIGRhdGFiYXNlID0gc3RhdGljX2Nhc3Q8RGF0 YWJhc2UqPihjb250ZXh0KTsKKyAgICBkYXRhYmFzZS0+ZG9jdW1lbnQoKS0+cmVtb3ZlT3BlbkRh dGFiYXNlKGRhdGFiYXNlKTsKKyAgICBkYXRhYmFzZS0+ZGVyZWYoKTsKK30KKwogdm9pZCBEYXRh YmFzZTo6Y2xvc2UoKQogewotICAgIGlmIChtX29wZW5lZCkgewotICAgICAgICBBU1NFUlQobV9k b2N1bWVudC0+ZGF0YWJhc2VUaHJlYWQoKSk7Ci0gICAgICAgIEFTU0VSVChjdXJyZW50VGhyZWFk KCkgPT0gZG9jdW1lbnQoKS0+ZGF0YWJhc2VUaHJlYWQoKS0+Z2V0VGhyZWFkSUQoKSk7Ci0gICAg ICAgIG1fc3FsaXRlRGF0YWJhc2UuY2xvc2UoKTsKLSAgICAgICAgbV9kb2N1bWVudC0+ZGF0YWJh c2VUaHJlYWQoKS0+cmVjb3JkRGF0YWJhc2VDbG9zZWQodGhpcyk7Ci0gICAgICAgIG1fb3BlbmVk ID0gZmFsc2U7Ci0KLSAgICAgICAgewotICAgICAgICAgICAgTXV0ZXhMb2NrZXIgbG9ja2VyKGd1 aWRNdXRleCgpKTsKLQotICAgICAgICAgICAgSGFzaFNldDxEYXRhYmFzZSo+KiBoYXNoU2V0ID0g Z3VpZFRvRGF0YWJhc2VNYXAoKS5nZXQobV9ndWlkKTsKLSAgICAgICAgICAgIEFTU0VSVChoYXNo U2V0KTsKLSAgICAgICAgICAgIEFTU0VSVChoYXNoU2V0LT5jb250YWlucyh0aGlzKSk7Ci0gICAg ICAgICAgICBoYXNoU2V0LT5yZW1vdmUodGhpcyk7Ci0gICAgICAgICAgICBpZiAoaGFzaFNldC0+ aXNFbXB0eSgpKSB7Ci0gICAgICAgICAgICAgICAgZ3VpZFRvRGF0YWJhc2VNYXAoKS5yZW1vdmUo bV9ndWlkKTsKLSAgICAgICAgICAgICAgICBkZWxldGUgaGFzaFNldDsKLSAgICAgICAgICAgICAg ICBndWlkVG9WZXJzaW9uTWFwKCkucmVtb3ZlKG1fZ3VpZCk7Ci0gICAgICAgICAgICB9CisgICAg aWYgKCFtX29wZW5lZCkKKyAgICAgICAgcmV0dXJuOworCisgICAgQVNTRVJUKG1fZG9jdW1lbnQt PmRhdGFiYXNlVGhyZWFkKCkpOworICAgIEFTU0VSVChjdXJyZW50VGhyZWFkKCkgPT0gZG9jdW1l bnQoKS0+ZGF0YWJhc2VUaHJlYWQoKS0+Z2V0VGhyZWFkSUQoKSk7CisgICAgbV9zcWxpdGVEYXRh YmFzZS5jbG9zZSgpOworICAgIG1fZG9jdW1lbnQtPmRhdGFiYXNlVGhyZWFkKCktPnJlY29yZERh dGFiYXNlQ2xvc2VkKHRoaXMpOworICAgIG1fb3BlbmVkID0gZmFsc2U7CisKKyAgICB7CisgICAg ICAgIE11dGV4TG9ja2VyIGxvY2tlcihndWlkTXV0ZXgoKSk7CisKKyAgICAgICAgSGFzaFNldDxE YXRhYmFzZSo+KiBoYXNoU2V0ID0gZ3VpZFRvRGF0YWJhc2VNYXAoKS5nZXQobV9ndWlkKTsKKyAg ICAgICAgQVNTRVJUKGhhc2hTZXQpOworICAgICAgICBBU1NFUlQoaGFzaFNldC0+Y29udGFpbnMo dGhpcykpOworICAgICAgICBoYXNoU2V0LT5yZW1vdmUodGhpcyk7CisgICAgICAgIGlmIChoYXNo U2V0LT5pc0VtcHR5KCkpIHsKKyAgICAgICAgICAgIGd1aWRUb0RhdGFiYXNlTWFwKCkucmVtb3Zl KG1fZ3VpZCk7CisgICAgICAgICAgICBkZWxldGUgaGFzaFNldDsKKyAgICAgICAgICAgIGd1aWRU b1ZlcnNpb25NYXAoKS5yZW1vdmUobV9ndWlkKTsKICAgICAgICAgfQogICAgIH0KKworICAgIG1f ZG9jdW1lbnQtPmRhdGFiYXNlVGhyZWFkKCktPnVuc2NoZWR1bGVEYXRhYmFzZVRhc2tzKHRoaXMp OworCisgICAgRGF0YWJhc2VUcmFja2VyOjp0cmFja2VyKCkucmVtb3ZlT3BlbkRhdGFiYXNlKHRo aXMpOworICAgIHJlZigpOworICAgIGNhbGxPbk1haW5UaHJlYWQoZG9jdW1lbnRSZW1vdmVPcGVu RGF0YWJhc2UsIHRoaXMpOwogfQogCiB2b2lkIERhdGFiYXNlOjpzdG9wKCkK