23006 2008-12-28 04:12:50 -0800 Many Loader::Host member functions are not safe to use reentrantly 2008-12-28 13:51:06 -0800 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) All All RESOLVED FIXED InRadar, Regression P2 Normal --- 1 zwarich zwarich darin ddkilzer oldest_to_newest 103576 0 zwarich 2008-12-28 04:12:50 -0800 Many Loader::Host member functions set m_processingResource to true when they begin processing a resource and set it to false when they are done. Thanks to JavaScript and the web inspector, almost anything can happen during the processing of a resource, including these functions being called reentrantly, which is unsafe due to this way of using m_processingResource. Since m_processingResource is used to determine whether a Host is deleted in Loader::servePendingRequests(), this is presumedly the cause behind memory corruption bugs like <rdar://problem/6216106>. 103577 1 zwarich 2008-12-28 04:13:03 -0800 <rdar://problem/6216106> 103578 2 26275 zwarich 2008-12-28 04:31:52 -0800 Created attachment 26275 Proposed patch I am looking for a lot of review comments on this one. If anything comes to mind, don't assume that I thought about it and did it right. ;-) 103605 3 26275 darin 2008-12-28 11:05:52 -0800 Comment on attachment 26275 Proposed patch This patch is fine. We could also instead have ProcessingResource save and restore the m_processingResource boolean, which would use more stack space but less space in the object. It's really just fine as is. I'm going to say r=me I don't understand your comment "There are no occurrences of crashes caused by this bug that are reproducible by multiple people". Who ran into this? How did you diagnose it? 103630 4 zwarich 2008-12-28 12:54:56 -0800 I diagnosed the problem in the Radar because people were saying that it happens a lot when they are using the debugger in the inspector, which I verified causes this bad reentrancy after breaking in GDB and noticing that the JS debugger reenters the event loop. I couldn't get it to actually delete one of the Hosts that it flagged as being safe for deletion. Other people had test cases they claimed were reproducible for them at various times, but I could never make any of them work for me. Dan Bernstein was in GDB for a particular occurrence of the bug, and said that it seemed that control was being returned to Loader::Host::didFinishLoading() with a freed Loader::Host(). I just guessed that this might be the problem from all the information given. 103631 5 darin 2008-12-28 12:59:18 -0800 OK. 103636 6 zwarich 2008-12-28 13:51:06 -0800 Landed in r39494. 26275 2008-12-28 04:31:52 -0800 2008-12-28 11:05:52 -0800 Proposed patch reentrant.diff text/plain 6228 zwarich SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM5NDg5 KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsNDAgQEAKKzIwMDgtMTIt MjggIENhbWVyb24gWndhcmljaCAgPGN3endhcmljaEB1d2F0ZXJsb28uY2E+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQnVnIDIzMDA2OiBNYW55IExv YWRlcjo6SG9zdCBtZW1iZXIgZnVuY3Rpb25zIG5vdCBzYWZlIHRvIHVzZSByZWVudHJhbnRseQor ICAgICAgICA8aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIzMDA2Pgor ICAgICAgICA8cmRhcjovL3Byb2JsZW0vNjIxNjEwNj4KKworICAgICAgICBNYW55IExvYWRlcjo6 SG9zdCBtZW1iZXIgZnVuY3Rpb25zIHNldCBtX3Byb2Nlc3NpbmdSZXNvdXJjZSB0byB0cnVlIHdo ZW4gdGhleQorICAgICAgICBiZWdpbiBwcm9jZXNzaW5nIGEgcmVzb3VyY2UgYW5kIHNldCBpdCB0 byBmYWxzZSB3aGVuIHRoZXkgYXJlIGRvbmUuIFRoYW5rcyB0bworICAgICAgICBKYXZhU2NyaXB0 IGFuZCB0aGUgd2ViIGluc3BlY3RvciwgYWxtb3N0IGFueXRoaW5nIGNhbiBoYXBwZW4gZHVyaW5n IHRoZQorICAgICAgICBwcm9jZXNzaW5nIG9mIGEgcmVzb3VyY2UsIGluY2x1ZGluZyB0aGVzZSBm dW5jdGlvbnMgYmVpbmcgY2FsbGVkIHJlZW50cmFudGx5LAorICAgICAgICB3aGljaCBpcyB1bnNh ZmUgZHVlIHRvIHRoaXMgd2F5IG9mIHVzaW5nIG1fcHJvY2Vzc2luZ1Jlc291cmNlLgorCisgICAg ICAgIFRoaXMgY2FuIHRoZW9yZXRpY2FsbHkgY2F1c2UgYSBMb2FkZXI6Okhvc3QgdG8gYmUgdXNl ZCBhZnRlciBpdCBpcyBmcmVlZCwKKyAgICAgICAgYmVjYXVzZSB3aGVuIExvYWRlcjo6c2VydmVk UGVuZGluZ1JlcXVlc3RzKCkgaXMgY2FsbGVkLCBpdCB3aWxsIGZyZWUgSG9zdHMKKyAgICAgICAg dGhhdCBoYXZlIG1fcHJvY2Vzc2luZ1Jlc291cmNlIHNldCB0byBmYWxzZS4KKworICAgICAgICBU byBmaXggdGhpcywgd2UgcmVwbGFjZSBtX3Byb2Nlc3NpbmdSZXNvdXJjZSB3aXRoIG1fbnVtUmVz b3VyY2VzUHJvY2Vzc2luZywKKyAgICAgICAgd2hpY2ggaXMgaW5jcmVtZW50ZWQgYW5kIGRlY3Jl bWVudGVkIHVzaW5nIGEgaGVscGVyIG9iamVjdCwgUHJvY2Vzc2luZ1Jlc291cmNlLgorCisgICAg ICAgIFRoZXJlIGFyZSBubyBvY2N1cnJlbmNlcyBvZiBjcmFzaGVzIGNhdXNlZCBieSB0aGlzIGJ1 ZyB0aGF0IGFyZSByZXByb2R1Y2libGUKKyAgICAgICAgYnkgbXVsdGlwbGUgcGVvcGxlLCBidXQg dGhpcyBmaXhlcyB0aGUgcHJvYmxlbSBvZiBtX3Byb2Nlc3NpbmdSZXNvdXJjZSBiZWluZworICAg ICAgICBzZXQgdG8gZmFsc2Ugd2hpbGUgYSBIb3N0IGlzIHN0aWxsIGFsaXZlLgorCisgICAgICAg ICogbG9hZGVyL2xvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpMb2FkZXI6Okhvc3Q6Okhv c3QpOgorICAgICAgICAoV2ViQ29yZTo6TG9hZGVyOjpIb3N0OjpkaWRGaW5pc2hMb2FkaW5nKTog Q2hhbmdlIHRvIHVzZSBQcm9jZXNzaW5nUmVzb3VyY2UKKyAgICAgICAgaW5zdGVhZCBvZiBtYW51 YWxseSBzZXR0aW5nIG1fcHJvY2Vzc2luZ1Jlc291cmNlLgorICAgICAgICAoV2ViQ29yZTo6TG9h ZGVyOjpIb3N0OjpkaWRGYWlsKTogRGl0dG8uCisgICAgICAgIChXZWJDb3JlOjpMb2FkZXI6Okhv c3Q6OmRpZFJlY2VpdmVEYXRhKTogRGl0dG8uCisgICAgICAgICogbG9hZGVyL2xvYWRlci5oOgor ICAgICAgICAoV2ViQ29yZTo6TG9hZGVyOjpIb3N0OjpQcm9jZXNzaW5nUmVzb3VyY2U6OlByb2Nl c3NpbmdSZXNvdXJjZSk6IEFkZGVkLgorICAgICAgICAoV2ViQ29yZTo6TG9hZGVyOjpIb3N0OjpQ cm9jZXNzaW5nUmVzb3VyY2U6On5Qcm9jZXNzaW5nUmVzb3VyY2UpOiBBZGRlZC4KKyAgICAgICAg KFdlYkNvcmU6OkxvYWRlcjo6SG9zdDo6cHJvY2Vzc2luZ1Jlc291cmNlKTogQ2hhbmdlIHRvIHVz ZSBtX251bVJlc291cmNlc1Byb2Nlc3NpbmcKKyAgICAgICAgaW5zdGVhZCBvZiBqdXN0IGdldHRp bmcgbV9wcm9jZXNzaW5nUmVzb3VyY2UuCisKIDIwMDgtMTItMjggIERtaXRyeSBUaXRvdiAgPGRp bWljaEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCklu ZGV4OiBsb2FkZXIvbG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBsb2FkZXIvbG9hZGVyLmNwcAko cmV2aXNpb24gMzk0ODcpCisrKyBsb2FkZXIvbG9hZGVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAt MTgxLDcgKzE4MSw3IEBAIHZvaWQgTG9hZGVyOjpjYW5jZWxSZXF1ZXN0cyhEb2NMb2FkZXIqIGQK IExvYWRlcjo6SG9zdDo6SG9zdChjb25zdCBBdG9taWNTdHJpbmcmIG5hbWUsIHVuc2lnbmVkIG1h eFJlcXVlc3RzSW5GbGlnaHQpCiAgICAgOiBtX25hbWUobmFtZSkKICAgICAsIG1fbWF4UmVxdWVz dHNJbkZsaWdodChtYXhSZXF1ZXN0c0luRmxpZ2h0KQotICAgICwgbV9wcm9jZXNzaW5nUmVzb3Vy Y2UoZmFsc2UpCisgICAgLCBtX251bVJlc291cmNlc1Byb2Nlc3NpbmcoMCkKIHsKIH0KIApAQCAt MjgzLDcgKzI4Myw3IEBAIHZvaWQgTG9hZGVyOjpIb3N0OjpkaWRGaW5pc2hMb2FkaW5nKFN1YnIK ICAgICBpZiAoaSA9PSBtX3JlcXVlc3RzTG9hZGluZy5lbmQoKSkKICAgICAgICAgcmV0dXJuOwog ICAgIAotICAgIG1fcHJvY2Vzc2luZ1Jlc291cmNlID0gdHJ1ZTsKKyAgICBQcm9jZXNzaW5nUmVz b3VyY2UgcHJvY2Vzc2luZ1Jlc291cmNlKHRoaXMpOwogCiAgICAgUmVxdWVzdCogcmVxdWVzdCA9 IGktPnNlY29uZDsKICAgICBtX3JlcXVlc3RzTG9hZGluZy5yZW1vdmUoaSk7CkBAIC0zMTMsOCAr MzEzLDYgQEAgdm9pZCBMb2FkZXI6Okhvc3Q6OmRpZEZpbmlzaExvYWRpbmcoU3VicgogICAgIHBy aW50ZigiSE9TVCAlcyBDT1VOVCAlZCBSRUNFSVZFRCAlc1xuIiwgdS5ob3N0KCkubGF0aW4xKCku ZGF0YSgpLCBtX3JlcXVlc3RzTG9hZGluZy5zaXplKCksIHJlc291cmNlLT51cmwoKS5sYXRpbjEo KS5kYXRhKCkpOwogI2VuZGlmCiAgICAgc2VydmVQZW5kaW5nUmVxdWVzdHMoKTsKLSAgICAKLSAg ICBtX3Byb2Nlc3NpbmdSZXNvdXJjZSA9IGZhbHNlOwogfQogCiB2b2lkIExvYWRlcjo6SG9zdDo6 ZGlkRmFpbChTdWJyZXNvdXJjZUxvYWRlciogbG9hZGVyLCBjb25zdCBSZXNvdXJjZUVycm9yJikK QEAgLTMzMCw3ICszMjgsNyBAQCB2b2lkIExvYWRlcjo6SG9zdDo6ZGlkRmFpbChTdWJyZXNvdXJj ZUxvCiAgICAgaWYgKGkgPT0gbV9yZXF1ZXN0c0xvYWRpbmcuZW5kKCkpCiAgICAgICAgIHJldHVy bjsKIAotICAgIG1fcHJvY2Vzc2luZ1Jlc291cmNlID0gdHJ1ZTsKKyAgICBQcm9jZXNzaW5nUmVz b3VyY2UgcHJvY2Vzc2luZ1Jlc291cmNlKHRoaXMpOwogICAgIAogICAgIFJlcXVlc3QqIHJlcXVl c3QgPSBpLT5zZWNvbmQ7CiAgICAgbV9yZXF1ZXN0c0xvYWRpbmcucmVtb3ZlKGkpOwpAQCAtMzU3 LDggKzM1NSw2IEBAIHZvaWQgTG9hZGVyOjpIb3N0OjpkaWRGYWlsKFN1YnJlc291cmNlTG8KICAg ICBkb2NMb2FkZXItPmNoZWNrRm9yUGVuZGluZ1ByZWxvYWRzKCk7CiAKICAgICBzZXJ2ZVBlbmRp bmdSZXF1ZXN0cygpOwotCi0gICAgbV9wcm9jZXNzaW5nUmVzb3VyY2UgPSBmYWxzZTsKIH0KIAog dm9pZCBMb2FkZXI6Okhvc3Q6OmRpZFJlY2VpdmVSZXNwb25zZShTdWJyZXNvdXJjZUxvYWRlciog bG9hZGVyLCBjb25zdCBSZXNvdXJjZVJlc3BvbnNlJiByZXNwb25zZSkKQEAgLTQzNCwxMiArNDMw LDExIEBAIHZvaWQgTG9hZGVyOjpIb3N0OjpkaWRSZWNlaXZlRGF0YShTdWJyZXMKICAgICBpZiAo cmVzb3VyY2UtPmVycm9yT2NjdXJyZWQoKSkKICAgICAgICAgcmV0dXJuOwogICAgIAotICAgIG1f cHJvY2Vzc2luZ1Jlc291cmNlID0gdHJ1ZTsKKyAgICBQcm9jZXNzaW5nUmVzb3VyY2UgcHJvY2Vz c2luZ1Jlc291cmNlKHRoaXMpOwogICAgIAogICAgIGlmIChyZXNvdXJjZS0+cmVzcG9uc2UoKS5o dHRwU3RhdHVzQ29kZSgpIC8gMTAwID09IDQpIHsKICAgICAgICAgLy8gVHJlYXQgYSA0eHggcmVz cG9uc2UgbGlrZSBhIG5ldHdvcmsgZXJyb3IuCiAgICAgICAgIHJlc291cmNlLT5lcnJvcigpOwot ICAgICAgICBtX3Byb2Nlc3NpbmdSZXNvdXJjZSA9IGZhbHNlOwogICAgICAgICByZXR1cm47CiAg ICAgfQogCkBAIC00NTEsOCArNDQ2LDYgQEAgdm9pZCBMb2FkZXI6Okhvc3Q6OmRpZFJlY2VpdmVE YXRhKFN1YnJlcwogICAgICAgICByZXNvdXJjZS0+ZGF0YShjb3BpZWREYXRhLnJlbGVhc2UoKSwg dHJ1ZSk7CiAgICAgfSBlbHNlIGlmIChyZXF1ZXN0LT5pc0luY3JlbWVudGFsKCkpCiAgICAgICAg IHJlc291cmNlLT5kYXRhKGxvYWRlci0+cmVzb3VyY2VEYXRhKCksIGZhbHNlKTsKLSAgICAKLSAg ICBtX3Byb2Nlc3NpbmdSZXNvdXJjZSA9IGZhbHNlOwogfQogICAgIAogdm9pZCBMb2FkZXI6Okhv c3Q6OmNhbmNlbFBlbmRpbmdSZXF1ZXN0cyhSZXF1ZXN0UXVldWUmIHJlcXVlc3RzUGVuZGluZywg RG9jTG9hZGVyKiBkb2NMb2FkZXIpCkluZGV4OiBsb2FkZXIvbG9hZGVyLmgKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gbG9hZGVyL2xvYWRlci5oCShyZXZpc2lvbiAzOTQ4NykKKysrIGxvYWRlci9sb2FkZXIuaAko d29ya2luZyBjb3B5KQpAQCAtNjUsOSArNjUsMjcgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogICAg ICAgICAgICAgdm9pZCBzZXJ2ZVBlbmRpbmdSZXF1ZXN0cyhQcmlvcml0eSBtaW5pbXVtUHJpb3Jp dHkgPSBMb3cpOwogICAgICAgICAgICAgdm9pZCBjYW5jZWxSZXF1ZXN0cyhEb2NMb2FkZXIqKTsK ICAgICAgICAgICAgIGJvb2wgaGFzUmVxdWVzdHMoKSBjb25zdDsKLSAgICAgICAgICAgIGJvb2wg cHJvY2Vzc2luZ1Jlc291cmNlKCkgY29uc3QgeyByZXR1cm4gbV9wcm9jZXNzaW5nUmVzb3VyY2U7 IH0KLSAgICAgICAgCisKKyAgICAgICAgICAgIGJvb2wgcHJvY2Vzc2luZ1Jlc291cmNlKCkgY29u c3QgeyByZXR1cm4gbV9udW1SZXNvdXJjZXNQcm9jZXNzaW5nICE9IDA7IH0KKwogICAgICAgICBw cml2YXRlOgorICAgICAgICAgICAgY2xhc3MgUHJvY2Vzc2luZ1Jlc291cmNlIHsKKyAgICAgICAg ICAgIHB1YmxpYzoKKyAgICAgICAgICAgICAgICBQcm9jZXNzaW5nUmVzb3VyY2UoSG9zdCogaG9z dCkKKyAgICAgICAgICAgICAgICAgICAgOiBtX2hvc3QoaG9zdCkKKyAgICAgICAgICAgICAgICB7 CisgICAgICAgICAgICAgICAgICAgIG1faG9zdC0+bV9udW1SZXNvdXJjZXNQcm9jZXNzaW5nKys7 CisgICAgICAgICAgICAgICAgfQorCisgICAgICAgICAgICAgICAgflByb2Nlc3NpbmdSZXNvdXJj ZSgpCisgICAgICAgICAgICAgICAgeworICAgICAgICAgICAgICAgICAgICBtX2hvc3QtPm1fbnVt UmVzb3VyY2VzUHJvY2Vzc2luZy0tOworICAgICAgICAgICAgICAgIH0KKworICAgICAgICAgICAg cHJpdmF0ZToKKyAgICAgICAgICAgICAgICBIb3N0KiBtX2hvc3Q7CisgICAgICAgICAgICB9Owor CiAgICAgICAgICAgICB2aXJ0dWFsIHZvaWQgZGlkUmVjZWl2ZVJlc3BvbnNlKFN1YnJlc291cmNl TG9hZGVyKiwgY29uc3QgUmVzb3VyY2VSZXNwb25zZSYpOwogICAgICAgICAgICAgdmlydHVhbCB2 b2lkIGRpZFJlY2VpdmVEYXRhKFN1YnJlc291cmNlTG9hZGVyKiwgY29uc3QgY2hhciosIGludCk7 CiAgICAgICAgICAgICB2aXJ0dWFsIHZvaWQgZGlkRmluaXNoTG9hZGluZyhTdWJyZXNvdXJjZUxv YWRlciopOwpAQCAtODMsNyArMTAxLDcgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogICAgICAgICAg ICAgUmVxdWVzdE1hcCBtX3JlcXVlc3RzTG9hZGluZzsKICAgICAgICAgICAgIGNvbnN0IEF0b21p Y1N0cmluZyBtX25hbWU7CiAgICAgICAgICAgICBjb25zdCBpbnQgbV9tYXhSZXF1ZXN0c0luRmxp Z2h0OwotICAgICAgICAgICAgYm9vbCBtX3Byb2Nlc3NpbmdSZXNvdXJjZTsKKyAgICAgICAgICAg IGludCBtX251bVJlc291cmNlc1Byb2Nlc3Npbmc7CiAgICAgICAgIH07CiAgICAgICAgIHR5cGVk ZWYgSGFzaE1hcDxBdG9taWNTdHJpbmdJbXBsKiwgSG9zdCo+IEhvc3RNYXA7CiAgICAgICAgIEhv c3RNYXAgbV9ob3N0czsK