Bug 44158 – [soup] implement ResourceHandle::platformSetDefersLoading

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

[Philippe Normand]

as in subject, using soup_session_pause_message and soup_session_unpause_message

[Philippe Normand]

Created attachment 64680 [details]
proposed patch

[Gustavo Noronha (kov)]

Comment on attachment 64680 [details]
proposed patch

I think this is a good thing to have, since other platforms support it, but do we have an idea of what this gains us? Is there a way to test it? It looks like we want to also check whether the message should start deferred: the constructor has a boolean: ResourceHandle(const ResourceRequest&, ResourceHandleClient*, bool defersLoading, bool shouldContentSniff);. Mac checks m_defersLoading when a new message is queued, and pauses it, it seems. I'll say r- for the lack of checking if we should start deferred, but it would be great if we could know why we are implementing this before we do.

[Martin Robinson]

A quick grep shows that this is used throughout WebCore including extensively in PluginStream code to avoid bugs with plugins. It will also soon be used to pause/resume the download in WebKitWebSourceGStreamer.

[Martin Robinson]

I should also mention that I originally suggested to pnormand that he use this instead of reimplementing the functionality behind #ifdefs in https://bugs.webkit.org/show_bug.cgi?id=41512 .

[Philippe Normand]

Oh right, I forgot about this one.

Not sure I have time today but monday for sure I will revise the patch. Thanks kov for this first look at the code.

[Philippe Normand]

Created attachment 65105 [details]
proposed patch

About a test that would cover this I was thinking that maybe bug 42152
could be a good candidate but not sure yet. Opinions?

[Martin Robinson]

Yes, that's an excellent idea. If you implement those things, pass the test and then add them to this patch I will r+ it. Thanks!

[Philippe Normand]

(In reply to comment #7)
> Yes, that's an excellent idea. If you implement those things, pass the test and then add them to this patch I will r+ it. Thanks!

Filed a patch there but I only had to add the required DRT stuff to make the test pass, so not sure anymore it is a good test to test this patch ;)

[Philippe Normand]

Created attachment 65917 [details]
proposed patch

Added a check for d->m_msg before using it and a note about GIO

[Philippe Normand]

The WebCore/manual-tests/xhr-failure-behind-alert.html test triggers the platformSetDefersLoading() code path but in that case d->m_msg is NULL because no connection to the server was made (because none is running at 127.0.0.1:7 ...) ;)

[Dan Winship]

Comment on attachment 65917 [details]
proposed patch

>      soup_session_queue_message(session, d->m_msg, finishedCallback, handle);
>  
> +    if (d->m_defersLoading)
> +        soup_session_pause_message(session, d->m_msg);

This won't work; pause/unpause are sort of lame, and only work once I/O has actually started (eg, after the message has been assigned to a connection). If you want the message to be deferred right away, you need to just not queue it.

[Philippe Normand]

Comment on attachment 65917 [details]
proposed patch

Unmarking for review, I think it'd be better to wait the patch from bug 44261 to land so we can implement a better patch for this bug.

[Philippe Normand]

Created attachment 71174 [details]
updated patch

As Dan suggested, in startHttp I avoid sending the requested if it's
initially deferred.

[Philippe Normand]

Created attachment 71176 [details]
updated patch

[Martin Robinson]

Comment on attachment 71176 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71176&action=review

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:739
> +    // Only supported for http(s) transfers. Something similar would
> +    // probably be needed for data transfers done with GIO.

This comment is inaccurate now that data URLs are handled by soup, I think. :)

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:746
> +    else
> +        soup_session_unpause_message(defaultSession(), d->m_soupMessage.get());

I think this presents a problem. The time between the creation of m_soupRequest and m_soupMessage is an asynchronous call. If setDefersLoading(...) was called while that call was in progress, the message would not be paused properly. Perhaps there should be a companion call to soup_session_pause_message if the message is m_defersLoading is true in sendRequestCallback.

[Philippe Normand]

(In reply to comment #15)
> (From update of attachment 71176 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71176&action=review
> 
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:739
> > +    // Only supported for http(s) transfers. Something similar would
> > +    // probably be needed for data transfers done with GIO.
> 
> This comment is inaccurate now that data URLs are handled by soup, I think. :)
> 

Well I think it's still accurate ;) m_soupMessage is created only in startHttp(). startData and the data uri loading doesn't use it, afaics.

> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:746
> > +    else
> > +        soup_session_unpause_message(defaultSession(), d->m_soupMessage.get());
> 
> I think this presents a problem. The time between the creation of m_soupRequest and m_soupMessage is an asynchronous call. If setDefersLoading(...) was called while that call was in progress, the message would not be paused properly. Perhaps there should be a companion call to soup_session_pause_message if the message is m_defersLoading is true in sendRequestCallback.

Hum, that's right indeed. Will send a new patch, thanks for reviewing :)

[Martin Robinson]

This should be fixed since r67722. Closing.

[Philippe Normand]

How come a style bot commit have fixed this bug?

[Martin Robinson]

(In reply to comment #18)
> How come a style bot commit have fixed this bug?

Sorry, wrong bug.

[Philippe Normand]

Created attachment 74669 [details]
updated patch

[Martin Robinson]

Comment on attachment 74669 [details]
updated patch

Great.

[Philippe Normand]

Committed r72647: <http://trac.webkit.org/changeset/72647>

[Philippe Normand]

This patch broke the build (yes I checked here before landing :/). Rollout is in progress, see bug 50009

[WebKit Review Bot]

http://trac.webkit.org/changeset/72647 might have broken GTK Linux 32-bit Release

[Philippe Normand]

Created attachment 75664 [details]
updated patch

The tests were failing because loadResourceSynchronously() was
creating a deferred ResourceHandle.

[Martin Robinson]

Comment on attachment 75664 [details]
updated patch

Nice catch.

[Philippe Normand]

Committed r73357: <http://trac.webkit.org/changeset/73357>