Bug 24750

Summary: [GTK] requests download instead of displaying page
Product: WebKit Reporter: Gustavo Noronha (kov) <gns>
Component: WebKitGTKAssignee: Gustavo Noronha (kov) <gns>
Status: RESOLVED FIXED    
Severity: Normal Keywords: Gtk, Soup
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
URL: http://lhv.delfi.ee/news/index.cfm?id=1146314
Attachments:
Description Flags
proposed fix
none
use soup facilities to append parameters to the content type zecke: review+

Description Gustavo Noronha (kov) 2009-03-22 14:12:23 PDT
This was initially reported in Epiphany's bug tracker: http://bugzilla.gnome.org/show_bug.cgi?id=576267.

When the server sends to or more different Content-Type headers, webkit will request that the page be downloaded instead of displaying it:

wget -S -O /dev/null 'http://lhv.delfi.ee/news/index.cfm?id=1146314'
--2009-03-22 11:16:44--  http://lhv.delfi.ee/news/index.cfm?id=1146314
Resolving lhv.delfi.ee... 90.190.151.34
Connecting to lhv.delfi.ee|90.190.151.34|:80... connected.
HTTP request sent, awaiting response... 
  HTTP/1.1 200 OK
  Connection: close
  Date: Sun, 22 Mar 2009 14:16:51 GMT
  Server: Microsoft-IIS/6.0
  Content-type: text/html
  Page-Completion-Status: Normal
  Expires: -1
  Content-Type: text/html; charset=windows-1257
  Page-Completion-Status: Normal
  Set-Cookie: CFID=45627684; path=/; 
  Set-Cookie: CFTOKEN=52268372; path=/; 
  Set-Cookie: LOG_CLIENT_ID=35258851; expires=Sun, 27-Sep-2037 00:00:00 GMT;
path=/;

This is caused by the fact that libsoup will concatenate all instances of the header, and WebCore doesn't like that.
Comment 1 Gustavo Noronha (kov) 2009-03-22 15:15:55 PDT
Created attachment 28840 [details]
proposed fix
Comment 2 Holger Freyther 2009-03-25 00:23:46 PDT
Comment on attachment 28840 [details]
proposed fix

> -    String contentType = soup_message_headers_get(msg->response_headers, "Content-Type");
> +    GHashTable *contentTypeParameters = 0;

style, '*' pleaced wrongly.



> +    if (!contentType.isEmpty() && contentType.find(",", 0)) {
> +        Vector<String> contentTypes;
> +        contentType.split(',', contentTypes);
> +        contentType = contentTypes[0];
> +    }

Interesting question. What is faster... searching the string twice or creating the Vector?


> +
> +    if (contentTypeParameters) {
> +        GHashTableIter hashTableIter;
> +        gpointer hashKey;
> +        gpointer hashValue;
> +
> +        g_hash_table_iter_init(&hashTableIter, contentTypeParameters);
> +        while (g_hash_table_iter_next(&hashTableIter, &hashKey, &hashValue)) {
> +            contentType += "; ";
> +            contentType += static_cast<char*>(hashKey);
> +            contentType += "=";
> +            contentType += static_cast<char*>(hashValue);
> +        }

You could consider using StringBuilder here. And another thing. With HTTP headers are like ASCII which makes your use of String(char*) okay, but maybe write String() around it as well to highlight that we really want this.
Comment 3 Gustavo Noronha (kov) 2009-03-25 08:11:55 PDT
Landed as r41975.

(In reply to comment #2)
> > +    if (!contentType.isEmpty() && contentType.find(",", 0)) {
> > +        Vector<String> contentTypes;
> > +        contentType.split(',', contentTypes);
> > +        contentType = contentTypes[0];
> > +    }
> 
> Interesting question. What is faster... searching the string twice or creating
> the Vector?

I've gone with creating the Vector. Since split already works correctly, and doesn't do much more than checking if a , exists in the string, I believe this is the way to go.
 
> > +        while (g_hash_table_iter_next(&hashTableIter, &hashKey, &hashValue)) {
> > +            contentType += "; ";
> > +            contentType += static_cast<char*>(hashKey);
> > +            contentType += "=";
> > +            contentType += static_cast<char*>(hashValue);
> > +        }
> 
> You could consider using StringBuilder here. And another thing. With HTTP
> headers are like ASCII which makes your use of String(char*) okay, but maybe
> write String() around it as well to highlight that we really want this.
 
I have added the String()'s, but since I am not sure of the advantages of using StringBuilder here, when compared to simply using the + operator, I have left that out for the time being.

Thanks!
Comment 4 Gustavo Noronha (kov) 2009-03-25 13:46:23 PDT
Created attachment 28944 [details]
use soup facilities to append parameters to the content type

 WebCore/ChangeLog                                  |   10 ++++++++++
 .../platform/network/soup/ResourceHandleSoup.cpp   |   10 ++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)
Comment 5 Gustavo Noronha (kov) 2009-03-25 13:53:47 PDT
Comment on attachment 28944 [details]
use soup facilities to append parameters to the content type

danw noticed that my work-around is not robust enough for parameters with spaces in their values; this patch uses the libsoup facilities, which handle these cases correctly, to append the parameters
Comment 6 Gustavo Noronha (kov) 2009-03-25 13:55:21 PDT
Reopening bug because I posted a new, related patch.
Comment 7 Holger Freyther 2009-03-30 06:08:02 PDT
Comment on attachment 28944 [details]
use soup facilities to append parameters to the content type

Okay, didn't know this method, nice cleanup.
Comment 8 Gustavo Noronha (kov) 2009-03-30 09:35:38 PDT
Landed as r42106.