Bug 72227 – [GTK] improve platformSetDefersLoading

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

[Dan Winship]

ResourceHandleSoup currently implements defersLoading by calling soup_session_pause_message()/soup_session_unpause_message(), which is bad because that API sucks, and because SoupHTTPInputStream also uses it internally, and ResourceHandleSoup's use of it interferes with libsoup's, and complicates SoupHTTPInputStream because it has to deal with receiving input even when it thought the message was paused.

This patch fixes that. It's built on top of the bug 71611 patches, although I think it could be rebased away from that pretty easily if we wanted to land it before that.

This patch also makes defersLoading work for non-http URIs, which didn't work before... I looked through the skipped tests list, but I couldn't find any that looked like they were being skipped because of that. Maybe we need a new one?

[Dan Winship]

Created attachment 114861 [details]
Fix platformDefersLoading

[Martin Robinson]

Comment on attachment 114861 [details]
Fix platformDefersLoading

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

I really like how this patch removes a lot of hacks for using soup_session_(un)pause_message. A couple comments follow.

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

You'll need to remove the OOPS here for the cq to land this patch. You can just say something like, "No new tests. This is covered by existing tests."

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:564
>          return;

It seems like there's nothing now to do in this method if defersLoading == true? If so, you can do an early return right away and avoid all the other checking later.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:567
> +    // Requests that haven't yet been sent
> +    if (!d->m_cancellable) {

I think it makes sense to actually remove the comment and add a helper that makes this check explicit. You might do something like static bool requestStarted(...). It makes sense to have a canonical idea of when the request has started.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:578
> +    // Do not pause or unpause operations that have not reached sendRequestCallback yet.
> +    // If m_defersLoading is true at that point, we'll pause.
>      if (!d->m_inputStream)
>          return;

This check existed only because calling soup_session_pause_message at the wrong time led to CRITICAL warnings. I'm pretty sure you can remove it competely now.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:582
> +        GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult;
> +        d->m_deferredResult = 0;

I think you can just do: RefPtr<GAsyncResult> asyncResult = adoptGRef(d->m_deferredResult.leakRef());

[Martin Robinson]

Philippe can you verify that "emulating" a message pause still fulfills the needs of the GStreamer backend.

[Philippe Normand]

(In reply to comment #3)
> Philippe can you verify that "emulating" a message pause still fulfills the needs of the GStreamer backend.

I tested the patches, haven't found any regression media playback-wise, I did some functional testing and tried the layout tests as well.

[Dan Winship]

(In reply to comment #2)
> > +        No new tests. (OOPS!)
> 
> You'll need to remove the OOPS here for the cq to land this patch. You can just say something like, "No new tests. This is covered by existing tests."

Yeah, that was intentional:

> This patch also makes defersLoading work for non-http URIs, which
> didn't work before... I looked through the skipped tests list, but
> I couldn't find any that looked like they were being skipped because
> of that. Maybe we need a new one?


> It seems like there's nothing now to do in this method if defersLoading == true?

> This check existed only because calling soup_session_pause_message at the wrong time led to CRITICAL warnings. I'm pretty sure you can remove it competely now.

Ah, yeah. Cruft from earlier versions of the patch.

> > +        GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult;
> > +        d->m_deferredResult = 0;
> 
> I think you can just do: RefPtr<GAsyncResult> asyncResult = adoptGRef(d->m_deferredResult.leakRef());

OK. But is that actually nicer? :)

Is http://www.webkit.org/coding/RefPtr.html out of date? Or does GRefPtr intentionally not work like RefPtr? I originally tried

    GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult.release();

but GRefPtr doesn't have release() (and the docs on the RefPtr page say that leakRef() only exists on PassRefPtr)

[Martin Robinson]

(In reply to comment #5)

> Is http://www.webkit.org/coding/RefPtr.html out of date? Or does GRefPtr intentionally not work like RefPtr? I originally tried
> 
>     GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult.release();
>
> but GRefPtr doesn't have release() (and the docs on the RefPtr page say that leakRef() only exists on PassRefPtr)

Oh. I think that GRefPtr is missing the release method? That you needed it here indicates that it should probably have it. Do you mind adding it with your patch and using it? GRefPtr just lags behind RefPtr feature-wise.

[Dan Winship]

(In reply to comment #6)
> Oh. I think that GRefPtr is missing the release method? That you needed it here indicates that it should probably have it. Do you mind adding it with your patch and using it? GRefPtr just lags behind RefPtr feature-wise.

Hm... RefPtr.release() returns a PassRefPtr, but there's no GPassRefPtr.

The docs say "If ownership and lifetime are guaranteed, a local variable can be a raw pointer." So I just did that (with leakRef).

[Dan Winship]

Created attachment 115169 [details]
updated patch

[Dan Winship]

oh, that's rebased to master btw, so it's landable

[Martin Robinson]

Comment on attachment 115169 [details]
updated patch

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

Looks good to me, except that I think you are leaking the asyncResult.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:750
> +    // We only need to take action here to UN-defer loading

Nit: Missing a period.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:759
>          return;

Is it actually possible for m_cancellable to be non-null and d->m_soupRequest to be null? If so it'd be good to leave a small comment here explaining when and if not, it'd be better to ASSERT(d->m_soupRequest) instead.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:763
> +        GAsyncResult* asyncResult = d->m_deferredResult.leakRef();

I think you actually need to call adoptGRef here because the naked assignment adds another reference and the pointer leaks. Sorry for the confusion.

[Dan Winship]

Created attachment 115382 [details]
updated updated patch

updated for previous comments in bugzilla and on IRC. (I ended up going with your original suggestion for the leakref thing.)

[WebKit Review Bot]

Comment on attachment 115382 [details]
updated updated patch

Clearing flags on attachment: 115382

Committed r100466: <http://trac.webkit.org/changeset/100466>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.