Bug 25243 – Crash when data:// loads are cancelled

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

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).

[Gustavo Noronha (kov)]

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(-)

[Xan Lopez]

(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?

[Gustavo Noronha (kov)]

(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.

[Xan Lopez]

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 :)

[Gustavo Noronha (kov)]

(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!