Bug 88087 - [SOUP] WebSoupRequestManager should handle loading errors and zero-length replies
Summary: [SOUP] WebSoupRequestManager should handle loading errors and zero-length rep...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, Soup
Depends on:
Blocks: 84133
  Show dependency treegraph
 
Reported: 2012-06-01 06:38 PDT by Carlos Garcia Campos
Modified: 2012-06-07 02:11 PDT (History)
1 user (show)

See Also:


Attachments
Patch (17.46 KB, patch)
2012-06-01 06:48 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-06-01 06:38:20 PDT
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.
Comment 1 Carlos Garcia Campos 2012-06-01 06:48:28 PDT
Created attachment 145292 [details]
Patch
Comment 2 Martin Robinson 2012-06-06 09:48:26 PDT
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?
Comment 3 Carlos Garcia Campos 2012-06-06 23:12:30 PDT
(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.
Comment 4 Martin Robinson 2012-06-06 23:30:55 PDT
(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?
Comment 5 Carlos Garcia Campos 2012-06-06 23:34:30 PDT
(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.
Comment 6 Carlos Garcia Campos 2012-06-07 02:11:29 PDT
Committed r119695: <http://trac.webkit.org/changeset/119695>