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).
Created attachment 29549 [details] 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(-)
(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?
(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.
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 :)
(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!