Bug 88087 – [SOUP] WebSoupRequestManager should handle loading errors and zero-length replies

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

When an error happens while loading the resource, the WebSoupRequestManager doesn't notice it and the ui process keeps reading more data from the input stream. Zero-length replies are considered errors by WebSoupRequestManager in this moment.

[Carlos Garcia Campos]

Created attachment 145292 [details]
Patch

[Martin Robinson]

Comment on attachment 145292 [details]
Patch

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

> Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:50
> +        // If the struct contains a null request is because the request failed.

Nit: request is -> request, it is

> Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:65
> +    GSimpleAsyncResult* releaseResult()

It would make sense for this to return a GRefPtr to avoid a leak.

> Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:147
> +    if (!data)
> +        return;

Above you ASSERT data and here you return. Is there a situation where you expect data to be null here?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 145292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145292&action=review
> 
> > Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:50
> > +        // If the struct contains a null request is because the request failed.
> 
> Nit: request is -> request, it is

Ok.

> > Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:65
> > +    GSimpleAsyncResult* releaseResult()
> 
> It would make sense for this to return a GRefPtr to avoid a leak.

The only user of that method already adopts the ref, so it's not leaked.

> > Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:147
> > +    if (!data)
> > +        return;
> 
> Above you ASSERT data and here you return. Is there a situation where you expect data to be null here?

Yes, it's unlikely but possible. If there are more than one chunk messages sent by the UI process and the first one fails. The data is removed from the map, and a message is sent to the UI process to stop sending more chunk messages. The next chunk message already sent will be silently ignored if there's no data for for it in the request map.

[Martin Robinson]

(In reply to comment #3)

> > It would make sense for this to return a GRefPtr to avoid a leak.
> The only user of that method already adopts the ref, so it's not leaked.

Returning a RefPtr here would make it less likely for someone to misuse the new reference.
 
> Yes, it's unlikely but possible. If there are more than one chunk messages sent by the UI process and the first one fails. The data is removed from the map, and a message is sent to the UI process to stop sending more chunk messages. The next chunk message already sent will be silently ignored if there's no data for for it in the request map.

That makes sense. Do you mind putting that in a comment before the early return?

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > > It would make sense for this to return a GRefPtr to avoid a leak.
> > The only user of that method already adopts the ref, so it's not leaked.
> 
> Returning a RefPtr here would make it less likely for someone to misuse the new reference.

fair enough.

> > Yes, it's unlikely but possible. If there are more than one chunk messages sent by the UI process and the first one fails. The data is removed from the map, and a message is sent to the UI process to stop sending more chunk messages. The next chunk message already sent will be silently ignored if there's no data for for it in the request map.
> 
> That makes sense. Do you mind putting that in a comment before the early return?

Sure.

[Carlos Garcia Campos]

Committed r119695: <http://trac.webkit.org/changeset/119695>