Bug 144738 – REGRESSION(r183861): [SOUP] Downloads are broken when using the Network Process

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

I still don't know which revision broke this, but downloads don't work when using the network process. Try to download anything in GTK MiniBrowser, the download gets stalled right after it starts. It doesn't happen when using shared secondary process model.

[Carlos Garcia Campos]

Maybe it's the time to start running unit tests with network process too.

[Carlos Garcia Campos]

When converting the main resource handle to a download, the NetworkResourceLoader is aborted, but the ResourceHandle shouldn't be cleaned up because it's still used for the download.

[Carlos Garcia Campos]

Created attachment 253103 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 253103 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-210
> +    if (m_handle && !m_didConvertHandleToDownload)
>          m_handle->setClient(nullptr);
> -        m_handle = nullptr;
> -    }

This is not right for Mac. On the Mac, the download is not a ResourceHandleClient, so the client pointer is invalid once the handle is converted to a download.

Would it be possible to follow Mac design, and not use a ResourceHandleClient for downloading? This is quite confusing, I certainly didn't expect such a difference when making the change.

[Alexey Proskuryakov]

A quick fix for you may be to change the client after calling didConvertHandleToDownload(), not before.

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 253103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253103&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-210
> > +    if (m_handle && !m_didConvertHandleToDownload)
> >          m_handle->setClient(nullptr);
> > -        m_handle = nullptr;
> > -    }
> 
> This is not right for Mac. On the Mac, the download is not a
> ResourceHandleClient, so the client pointer is invalid once the handle is
> converted to a download.
> 
> Would it be possible to follow Mac design, and not use a
> ResourceHandleClient for downloading? This is quite confusing, I certainly
> didn't expect such a difference when making the change.

What's wrong using a ResourceHandleClient for downloads? We would use libsoup directly for downloading, but we would probably ended up duplicating the ResourceHandle code.

[Carlos Garcia Campos]

(In reply to comment #5)
> A quick fix for you may be to change the client after calling
> didConvertHandleToDownload(), not before.

Not that easy, the client is private to DownloadSoup and set in Download::startWithHandle(). What we could do, though, is creating a new handle instead of reusing the existing one.

[Carlos Garcia Campos]

Or we could add isDownload() to ResourceHandleClient and do something like:

if (m_handle && m_handle->client() && !m_handle->client()->isDownload())
    m_handle->setClient(nullptr);

it will always return false for non soup ports.

[Alexey Proskuryakov]

> What's wrong using a ResourceHandleClient for downloads?

It's best to use cross-platform code in the same way on all ports. When we don't, we get bugs like this.

> What we could do, though, is creating a new handle instead of reusing the existing one.

This seems better indeed, although it still smells like something that would cause trouble due to different usage scenarios for the same cross-platform abstraction.

> Or we could add isDownload() to ResourceHandleClient and do something like:

This is a pretty gross hack. Even though downloading is the only case where we swap ResourceHandleClients at this time, ResourceHandle interface in no way enforces that. Also, the check would be super surprising to anyone who looks at cross-platform and Mac code only - the libsoup behavior is hidden pretty well.

If the first option (using custom soup based code for downloading) is not practical, I would consider adding a disconnectClient(ResourceHandleClient*) function to ResourceHandle, which would be a no-op if the argument doesn't match the current client.

However, I have a suspicion that most of the code needed to reimplement downloading would be moved, not duplicated.

[Carlos Garcia Campos]

Created attachment 253126 [details]
Updated patch

New approach using always a different ResourceHandle for downloads. This also fixes bug #144972

[Carlos Garcia Campos]

(In reply to comment #9)
> > What's wrong using a ResourceHandleClient for downloads?
> 
> It's best to use cross-platform code in the same way on all ports. When we
> don't, we get bugs like this.

But the downloads implementation in mac is not cross-platform at all, no?

> > What we could do, though, is creating a new handle instead of reusing the existing one.
> 
> This seems better indeed, although it still smells like something that would
> cause trouble due to different usage scenarios for the same cross-platform
> abstraction.
> 
> > Or we could add isDownload() to ResourceHandleClient and do something like:
> 
> This is a pretty gross hack. Even though downloading is the only case where
> we swap ResourceHandleClients at this time, ResourceHandle interface in no
> way enforces that. Also, the check would be super surprising to anyone who
> looks at cross-platform and Mac code only - the libsoup behavior is hidden
> pretty well.
> 
> If the first option (using custom soup based code for downloading) is not
> practical, I would consider adding a disconnectClient(ResourceHandleClient*)
> function to ResourceHandle, which would be a no-op if the argument doesn't
> match the current client.
> 
> However, I have a suspicion that most of the code needed to reimplement
> downloading would be moved, not duplicated.

I could try to move the soup implementation to a helper class to be used by both ResourceHandle and Downloads, but that would require a lot more time. Since this is a serious regression, I prefer to fix it ASAP and then try to do it in a different way, but I still don't see why using a ResourceHandle is a problem. ResourceHandle is a good wrapper around the libsoup details, and I think we have always used a ResourceHandle for downloads, even in WebKit1.

[Alexey Proskuryakov]

Comment on attachment 253126 [details]
Updated patch

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

> But the downloads implementation in mac is not cross-platform at all, no?

What I'm talking about is that abstractions should abstract out platform differences. We have cross-platform abstractions for ResourceHandle and ResourceHandleClient. Code that uses those should expect identical behavior on all platforms - if it's correct to set the client to null on Mac, it should be the correct thing to do everywhere.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1042
> +    return WTF::move(newHandle);

Is WTF::move needed? I thought that C++11 must move automatically in this case.

[Carlos Garcia Campos]

(In reply to comment #12)
> Comment on attachment 253126 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253126&action=review

Thanks!

> > But the downloads implementation in mac is not cross-platform at all, no?
> 
> What I'm talking about is that abstractions should abstract out platform
> differences. We have cross-platform abstractions for ResourceHandle and
> ResourceHandleClient. Code that uses those should expect identical behavior
> on all platforms - if it's correct to set the client to null on Mac, it
> should be the correct thing to do everywhere.

Ok, got it.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1042
> > +    return WTF::move(newHandle);
> 
> Is WTF::move needed? I thought that C++11 must move automatically in this
> case.

I think we can just return the RefPtr, yes.

[Carlos Garcia Campos]

Committed r184376: <http://trac.webkit.org/changeset/184376>