[Webkit-unassigned] [Bug 44261] [GTK] Add HTTP caching support

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 30 15:19:34 PDT 2010


https://bugs.webkit.org/show_bug.cgi?id=44261


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69356|review?                     |review-
               Flag|                            |




--- Comment #98 from Martin Robinson <mrobinson at webkit.org>  2010-09-30 15:19:34 PST ---
(From update of attachment 69356)
View in context: https://bugs.webkit.org/attachment.cgi?id=69356&action=review

This patch is so very close, but I've found some issues with memory leaks. When constructing a platformRefPtr, you should use adoptPlatformRef, unless you want the reference count to increase (the default constructor calls g_object_ref_sink).

> WebCore/platform/network/ResourceHandleInternal.h:118
> +            , m_soupRequest(0)
> +            , m_requester(0)
>              , m_inputStream(0)
>              , m_cancellable(0)

Since these are now PlatformRefPtrs you don't need to initialize them. They are automatically 0.

> WebCore/platform/network/ResourceHandleInternal.h:140
> +            m_requester = webkit_soup_requester_new();

This should use adoptPlatformRef, see below for an extended note about this.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:357
> +    d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), handle->firstRequest().url().string().utf8().data(), session, &error.outPtr());

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:365
> +    if (error)
> +        return false;
> +
> +    d->m_inputStream = in;

Careful here. From the soup code it looks like it is the callers responsibility to unref the value that webkit_soup_request_send returns. Constructing a PlatformRefPtr without using adoptPlatformRef has the effect of calling g_object_ref another time on the GInputStream. 

d->m_inputStream = adoptPlatformRef(webkit_soup_request_send(d->m_soupRequest.get(), 0, &error.outPtr()));
if (error) {
    d->m_inputStream = 0;
    return false;
}

and dispense with the intermediate 'in' variable entirely.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:373
> +    d->m_cancellable = g_cancellable_new();

This should be d->m_cancellable = adoptPlatformRef(g_cancellable_new()); as well.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:436
> +static void cleanupSoupRequestOperation(ResourceHandle* handle, bool isDestroying = false)
> +{
> +    ResourceHandleInternal* d = handle->getInternal();
> +
> +    if (d->m_soupRequest) {
> +        g_object_set_data(G_OBJECT(d->m_soupRequest.get()), "webkit-resource", 0);
> +        d->m_soupRequest.clear();
> +    }

We still need to clear the m_cancellable here to ensure that the unref happens, right?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:519
> +    d->m_inputStream = in;

The same goes for this section. After this the reference count on d->m_inputStream will be 2.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:564
> +    d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), url.string().utf8().data(), session, &error.outPtr());

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:652
> +    d->m_cancellable = g_cancellable_new();

Same issue here.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:833
>      // GIO doesn't know how to handle refs and queries, so remove them

Shouldn't this comment be changed to reflect the fact that we're using soup now?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:839
> +    urlStr = url.string().utf8().data();

This might as well be:
CString urlStr = url.string().utf8().data();

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:842
> +    d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), urlStr.data(), session, &error.outPtr());

I think this should use adoptPlatformRef as well.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:851
>      d->m_cancellable = g_cancellable_new();

Same issue here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list