25243 2009-04-16 14:05:57 -0700 Crash when data:// loads are cancelled 2009-04-20 08:25:38 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) PC All RESOLVED FIXED Gtk, Soup P2 Normal --- 1 gustavo webkit-unassigned xan.lopez oldest_to_newest 117792 0 gustavo 2009-04-16 14:05:57 -0700 The plugins/return-error-from-new-stream-callback-in-full-frame-plugin.html test crashes in WebKitGTK+ (though the crash is reported as being caused by the next test, because parseDataUrl is called as an idle callback). 117793 1 29549 gustavo 2009-04-16 14:07:09 -0700 Created attachment 29549 fix plugin crash WebCore/ChangeLog | 15 +++++++++++++++ .../platform/network/soup/ResourceHandleSoup.cpp | 15 ++++++++++++++- WebKit/gtk/ChangeLog | 10 ++++++++++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 7 +++++++ 4 files changed, 46 insertions(+), 1 deletions(-) 118098 2 xan.lopez 2009-04-20 06:53:01 -0700 (In reply to comment #1) > Created an attachment (id=29549) [review] > fix plugin crash > > WebCore/ChangeLog | 15 +++++++++++++++ > .../platform/network/soup/ResourceHandleSoup.cpp | 15 ++++++++++++++- > WebKit/gtk/ChangeLog | 10 ++++++++++ > WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 7 +++++++ > 4 files changed, 46 insertions(+), 1 deletions(-) > + Properly handle cancellation of the load for data:// loads. This + fixes crashing in the followin test: + + plugins/return-error-from-new-stream-callback-in-full-frame-plugin.html Extra indentation? + ResourceHandleInternal* d = handle->getInternal(); + if (d->m_cancelled) + return false; Wrong indentation. So, all the checks are needed for the crash to go away, or you are just being extra careful? And the FrameLoaderClientGtk change is for the same bug? 118100 3 gustavo 2009-04-20 07:23:17 -0700 (In reply to comment #2) > So, all the checks are needed for the crash to go away, or you are just being > extra careful? And the FrameLoaderClientGtk change is for the same bug? No, I think we can get along (for this specific crash) with only this check: if (data.length() > 0) client->didReceiveData(handle, reinterpret_cast<const char*>(data.c + + if (d->m_cancelled) + return false; The other checks I am adding are based on previous experience with fixing loading crashers. We should never trust a load (and the objects that are bound to it) to still exist after dispatching delegates, such as didReceiveResponse. As with content sniffing, plugins add yet another layer of potential problems, because they also delay the didReceiveResponse to when they get the first data (that's why we have that check in FrameLoaderClient, and this one after didReceiveData). Since the checks are all fixing the same problem, though happening under different conditions, I think they make sense as a single commit. 118102 4 xan.lopez 2009-04-20 07:33:33 -0700 I see, that covers all but the first check, which I guess it's just an extra precaution in case the loading was cancelled before the function is executed (since it's queued as an idle)? So, r=me with those style issues fixed :) 118118 5 gustavo 2009-04-20 08:25:38 -0700 (In reply to comment #4) > I see, that covers all but the first check, which I guess it's just an extra > precaution in case the loading was cancelled before the function is executed > (since it's queued as an idle)? Right, I forgot to talk about that one. Yes, indeed, it is a safe-guard because of it being an idle =). > So, r=me with those style issues fixed :) Landed as r42670! 29549 2009-04-16 14:07:09 -0700 2009-04-20 07:34:00 -0700 fix plugin crash fix-plugin-crash.patch text/plain 3891 gustavo YTMzYTgxNTYyMTBhNGE0MmI3NWY4YTMyMmRhN2M5M2U5ZmI2Nzc0NQpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAyNzMxOTg3Li5lYTEzNTZi IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpA QCAtMSwzICsxLDE4IEBACisyMDA5LTA0LTE2ICBHdXN0YXZvIE5vcm9uaGEgU2lsdmEgIDxndXN0 YXZvLm5vcm9uaGFAY29sbGFib3JhLmNvLnVrPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0yNTI0MworICAgICAgICBDcmFzaCB3aGVuIGRhdGE6Ly8gbG9hZHMgYXJlIGNhbmNlbGxl ZAorCisgICAgICAgIFByb3Blcmx5IGhhbmRsZSBjYW5jZWxsYXRpb24gb2YgdGhlIGxvYWQgZm9y IGRhdGE6Ly8gbG9hZHMuIFRoaXMKKyAgICAgICAgZml4ZXMgY3Jhc2hpbmcgaW4gdGhlIGZvbGxv d2luIHRlc3Q6CisKKyAgICAgICAgICBwbHVnaW5zL3JldHVybi1lcnJvci1mcm9tLW5ldy1zdHJl YW0tY2FsbGJhY2staW4tZnVsbC1mcmFtZS1wbHVnaW4uaHRtbAorCisgICAgICAgICogcGxhdGZv cm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFuZGxlU291cC5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpwYXJzZURhdGFVcmwpOgorCiAyMDA5LTA0LTE2ICBKdXN0aW4gR2FyY2lhICA8anVzdGluLmdh cmNpYUBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgU2ltb24gRnJhc2VyLgpkaWZm IC0tZ2l0IGEvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VIYW5kbGVTb3Vw LmNwcCBiL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFuZGxlU291cC5j cHAKaW5kZXggOTI1MGQ2NS4uZDkwM2M5ZSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9wbGF0Zm9ybS9u ZXR3b3JrL3NvdXAvUmVzb3VyY2VIYW5kbGVTb3VwLmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3Jt L25ldHdvcmsvc291cC9SZXNvdXJjZUhhbmRsZVNvdXAuY3BwCkBAIC0zNjYsOCArMzY2LDExIEBA IHN0YXRpYyBnYm9vbGVhbiBwYXJzZURhdGFVcmwoZ3BvaW50ZXIgY2FsbGJhY2tfZGF0YSkKIHsK ICAgICBSZXNvdXJjZUhhbmRsZSogaGFuZGxlID0gc3RhdGljX2Nhc3Q8UmVzb3VyY2VIYW5kbGUq PihjYWxsYmFja19kYXRhKTsKICAgICBSZXNvdXJjZUhhbmRsZUNsaWVudCogY2xpZW50ID0gaGFu ZGxlLT5jbGllbnQoKTsKKyAgICBSZXNvdXJjZUhhbmRsZUludGVybmFsKiBkID0gaGFuZGxlLT5n ZXRJbnRlcm5hbCgpOworICAgIGlmIChkLT5tX2NhbmNlbGxlZCkKKyAgICAgIHJldHVybiBmYWxz ZTsKIAotICAgIGhhbmRsZS0+Z2V0SW50ZXJuYWwoKS0+bV9pZGxlSGFuZGxlciA9IDA7CisgICAg ZC0+bV9pZGxlSGFuZGxlciA9IDA7CiAKICAgICBBU1NFUlQoY2xpZW50KTsKICAgICBpZiAoIWNs aWVudCkKQEAgLTQwMyw2ICs0MDYsOSBAQCBzdGF0aWMgZ2Jvb2xlYW4gcGFyc2VEYXRhVXJsKGdw b2ludGVyIGNhbGxiYWNrX2RhdGEpCiAgICAgICAgIHJlc3BvbnNlLnNldFRleHRFbmNvZGluZ05h bWUoY2hhcnNldCk7CiAgICAgICAgIGNsaWVudC0+ZGlkUmVjZWl2ZVJlc3BvbnNlKGhhbmRsZSwg cmVzcG9uc2UpOwogCisgICAgICAgIGlmIChkLT5tX2NhbmNlbGxlZCkKKyAgICAgICAgICAgIHJl dHVybiBmYWxzZTsKKwogICAgICAgICAvLyBVc2UgdGhlIEdMaWIgQmFzZTY0IGlmIGF2YWlsYWJs ZSwgc2luY2UgV2ViQ29yZSdzIGRlY29kZXIgaXNuJ3QKICAgICAgICAgLy8gZ2VuZXJhbC1wdXJw b3NlIGFuZCBmYWlscyBvbiBBY2lkMyB0ZXN0IDk3ICh3aGl0ZXNwYWNlKS4KICNpZmRlZiBVU0Vf R0xJQl9CQVNFNjQKQEAgLTQyMiw4ICs0MjgsMTUgQEAgc3RhdGljIGdib29sZWFuIHBhcnNlRGF0 YVVybChncG9pbnRlciBjYWxsYmFja19kYXRhKQogICAgICAgICBkYXRhID0gZGVjb2RlVVJMRXNj YXBlU2VxdWVuY2VzKGRhdGEsIFRleHRFbmNvZGluZyhjaGFyc2V0KSk7CiAgICAgICAgIHJlc3Bv bnNlLnNldFRleHRFbmNvZGluZ05hbWUoIlVURi0xNiIpOwogICAgICAgICBjbGllbnQtPmRpZFJl Y2VpdmVSZXNwb25zZShoYW5kbGUsIHJlc3BvbnNlKTsKKworICAgICAgICBpZiAoZC0+bV9jYW5j ZWxsZWQpCisgICAgICAgICAgICByZXR1cm4gZmFsc2U7CisKICAgICAgICAgaWYgKGRhdGEubGVu Z3RoKCkgPiAwKQogICAgICAgICAgICAgY2xpZW50LT5kaWRSZWNlaXZlRGF0YShoYW5kbGUsIHJl aW50ZXJwcmV0X2Nhc3Q8Y29uc3QgY2hhcio+KGRhdGEuY2hhcmFjdGVycygpKSwgZGF0YS5sZW5n dGgoKSAqIHNpemVvZihVQ2hhciksIDApOworCisgICAgICAgIGlmIChkLT5tX2NhbmNlbGxlZCkK KyAgICAgICAgICAgIHJldHVybiBmYWxzZTsKICAgICB9CiAKICAgICBjbGllbnQtPmRpZEZpbmlz aExvYWRpbmcoaGFuZGxlKTsKZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvQ2hhbmdlTG9nIGIvV2Vi S2l0L2d0ay9DaGFuZ2VMb2cKaW5kZXggODRkMjk0NS4uOTBlMWQxZSAxMDA2NDQKLS0tIGEvV2Vi S2l0L2d0ay9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cKQEAgLTEsMyArMSwx MyBAQAorMjAwOS0wNC0xNiAgR3VzdGF2byBOb3JvbmhhIFNpbHZhICA8Z3VzdGF2by5ub3Jvbmhh QGNvbGxhYm9yYS5jby51az4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBIYW5kbGUgdGhlIGNhc2Ugd2hlbiB0aGUgcGx1Z2luIHZpZXcgaXMgZGVzdHJv eWVkIGR1cmluZworICAgICAgICBkaWRSZWNlaXZlUmVzcG9uc2UsIHRvIGF2b2lkIGNyYXNoaW5n LgorCisgICAgICAgICogV2ViQ29yZVN1cHBvcnQvRnJhbWVMb2FkZXJDbGllbnRHdGsuY3BwOgor ICAgICAgICAoV2ViS2l0OjpGcmFtZUxvYWRlckNsaWVudDo6Y29tbWl0dGVkTG9hZCk6CisKIDIw MDktMDQtMTQgIEd1c3Rhdm8gTm9yb25oYSBTaWx2YSAgPGd1c3Rhdm8ubm9yb25oYUBjb2xsYWJv cmEuY28udWs+CiAKICAgICAgICAgVW5yZXZpZXdlZC4gVXBkYXRlIGd0ay1kb2MgY29udHJvbCBh bmQgYmFzZSBzZ21sIGZpbGVzIGZvciAxLjEuNS4KZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvV2Vi Q29yZVN1cHBvcnQvRnJhbWVMb2FkZXJDbGllbnRHdGsuY3BwIGIvV2ViS2l0L2d0ay9XZWJDb3Jl U3VwcG9ydC9GcmFtZUxvYWRlckNsaWVudEd0ay5jcHAKaW5kZXggOTdjM2ViMi4uNmQ3MWFmNiAx MDA2NDQKLS0tIGEvV2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRlckNsaWVudEd0 ay5jcHAKKysrIGIvV2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRlckNsaWVudEd0 ay5jcHAKQEAgLTIwMSw2ICsyMDEsMTMgQEAgdm9pZCBGcmFtZUxvYWRlckNsaWVudDo6Y29tbWl0 dGVkTG9hZChEb2N1bWVudExvYWRlciogbG9hZGVyLCBjb25zdCBjaGFyKiBkYXRhLAogICAgICAg ICAgICAgbV9wbHVnaW5WaWV3LT5kaWRSZWNlaXZlUmVzcG9uc2UobG9hZGVyLT5yZXNwb25zZSgp KTsKICAgICAgICAgICAgIG1faGFzU2VudFJlc3BvbnNlVG9QbHVnaW4gPSB0cnVlOwogICAgICAg ICB9CisKKyAgICAgICAgLy8gRklYTUU6IFdlIG1heSB3YW50IHRvIGludmVzdGlnYXRlIHJlZmFj dG9yaW5nIG91ciBwbHVnaW4gbG9hZGluZworICAgICAgICAvLyBjb2RlIHRvIGJlIHNpbWlsYXIg dG8gbWFjJ3MuCisgICAgICAgIC8vIEFsc28sIHNlZSBodHRwOi8vdHJhYy53ZWJraXQub3JnL2No YW5nZXNldC8yNDExOC4KKyAgICAgICAgaWYgKCFtX3BsdWdpblZpZXcpCisgICAgICAgICAgICBy ZXR1cm47CisKICAgICAgICAgbV9wbHVnaW5WaWV3LT5kaWRSZWNlaXZlRGF0YShkYXRhLCBsZW5n dGgpOwogICAgIH0KIH0K