Bug 24263 – [GTK] ref ResourceHandle until we are finished with it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Xan Lopez]

Add a ref to the ResourceHandle in startHttp so we can keep it alive untill all callbacks have been executed, and unref it when soup tells us it's done with the SoupMessage (in finishedCallback). Fixes a number of crashes when calling didReceiveData whould crash because of the handle dying.

Ideally we'd like to do this automatically with RefPtr I guess, but I'm not sure how to apply that here.

[Xan Lopez]

Created attachment 28121 [details]
refsouphandle.patch

Patch. And thanks to kov for the joint debugging session! :)

[Xan Lopez]

Created attachment 28122 [details]
refsouphandlev2.patch

Slightly improve comment text.

[Holger Freyther]

Comment on attachment 28122 [details]
refsouphandlev2.patch

r=me with two strings attached:

  - If possible I would like to see the adopRef in the finishedCallback (after this patch).
  - we need to find out if finishedCallback will always be called or if we just created a memory leak. the consensus on #webkit-gtk was that it is better to leak than to crash (for this release). We need to clean this up after the release...


> @@ -262,30 +262,32 @@ static void finishedCallback(SoupSession *session, SoupMessage* msg, gpointer da
>  
>      ResourceHandleClient* client = handle->client();
>      if (!client)
> -        return;
> +        goto exit;

this goto might be avoided with a RefPtr::adoptRef early on. I'm not quite sure, it would be nice if you could investiage that.


>  
>      d->m_msg = static_cast<SoupMessage*>(g_object_ref(msg));
> +    ref(); // balanced by a deref() in finishedCallback, which should always run

maybe you could move the comment above the line?
>      soup_session_queue_message(session, d->m_msg, finishedCallback, ma

[Christian Dywan]

2009-02-28  Xan Lopez  <[e-mail]>

        Reviewed by Holger Freyther.

        https://bugs.webkit.org/show_bug.cgi?id=24263
        [GTK] ref ResourceHandle until we are finished with it

        Add a ref to the ResourceHandle in startHttp so we can keep it
        alive untill all callbacks have been executed, and unref it when
        soup tells us it's done with the SoupMessage (in
        finishedCallback). Fixes a number of crashes when calling
        didReceiveData whould crash because of the handle dying.

        * platform/network/soup/ResourceHandleSoup.cpp:
        (WebCore::finishedCallback):
        (WebCore::ResourceHandle::startHttp):

Adjusted the comment and used adoptRef as suggested and discussed on IRC.

[Xan Lopez]

*** Bug 24196 has been marked as a duplicate of this bug. ***

[Xan Lopez]

Well, since all the points Holger raised were addressed (some by mistake in the same patch :)), I think we can close this.