Bug 42529

Summary: [Qt] Qt WebKit updates view on HTTP 204 response
Product: WebKit Reporter: Alex Plotnick <shrike>
Component: Page LoadingAssignee: Luiz Agostini <luiz>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, commit-queue, eric, kenneth, menard, peter.hartmann, shrike, simon.fraser, webkit.review.bot
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Fix Qt WebKit HTTP 204 response
none
patch
benjamin: review-
patch
none
patch
none
patch
kling: review-
patch
kenneth: review+, commit-queue: commit-queue-
Archive of layout-test-results from cr-jail-4 none

Description Alex Plotnick 2010-07-18 16:28:05 PDT
Created attachment 61919 [details]
Fix Qt WebKit HTTP 204 response

According to RFC 2616 section 10.2.5, a response of "204 No Content" should not cause a user agent to update its document view. Qt WebKit incorrectly displays a blank page. The attached patch seems to fix the problem. I lifted the solution from the corresponding Chromium class.
Comment 1 Benjamin Poulain 2011-01-14 11:36:02 PST
Arg. A hidden patch :(

Your patch was not seen by anyone for 2 reason:
1) you did not use the template to report the bug, so it was not triaged. See http://trac.webkit.org/wiki/QtWebKitBugs
2) you did not ask for review when attaching the patch. See http://trac.webkit.org/wiki/QtWebKitContrib

I suggest you to update the patch, create a changelog entry, and resubmit with "r?".
Comment 2 Luiz Agostini 2011-03-31 10:53:38 PDT
Created attachment 87763 [details]
patch
Comment 3 Benjamin Poulain 2011-03-31 10:57:11 PDT
Comment on attachment 87763 [details]
patch

A test would be nice.
Comment 4 Benjamin Poulain 2011-03-31 11:04:00 PDT
Comment on attachment 87763 [details]
patch

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

A test would be nice.

> Source/WebKit/qt/ChangeLog:8
> +        Ignoring http responses which have status code equal to 204 or 205.

I suggest to give the code name in the changelog so people don't have to look by themself what are those code for.

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1184
> +    if (statusCode == 204 || statusCode == 205) {
> +        // FIXME: a 205 response requires that the requester reset the document view.

Shouldn't 205 just be ignored then so the view is reloaded?

Call me crazy but what about?:
enum {
   HttpStatusNoContent = 204
}
...
if (statusCode == HttpStatusNoContent, etc)
Comment 5 Kenneth Rohde Christiansen 2011-03-31 11:27:00 PDT
> Call me crazy but what about?:
> enum {
>    HttpStatusNoContent = 204
> }
> ...
> if (statusCode == HttpStatusNoContent, etc)

I was complaining about something like that some days ago. I'm actually pretty sure that such a thing already exists in WebKit or at least used to.
Comment 6 Luiz Agostini 2011-03-31 13:24:48 PDT
(In reply to comment #4)
> (From update of attachment 87763 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87763&action=review
> 
> A test would be nice.

I need to find a way to test it.

> 
> > Source/WebKit/qt/ChangeLog:8
> > +        Ignoring http responses which have status code equal to 204 or 205.
> 
> I suggest to give the code name in the changelog so people don't have to look by themself what are those code for.

ok

> 
> > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1184
> > +    if (statusCode == 204 || statusCode == 205) {
> > +        // FIXME: a 205 response requires that the requester reset the document view.
> 
> Shouldn't 205 just be ignored then so the view is reloaded?

The 205 response must be ignored in the sense that its content (that should be empty) will not be shown. But the forms in the current content should be cleared as well.

RFC 2616 says:

    10.2.6 205 Reset Content

    The server has fulfilled the request and the user agent SHOULD reset the document view which
    caused the request to be sent. This response is primarily intended to allow input for actions
    to take place via user input, followed by a clearing of the form in which the input is given
    so that the user can easily initiate another input action. The response MUST NOT
    include an entity.

> 
> Call me crazy but what about?:
> enum {
>    HttpStatusNoContent = 204
> }
> ...
> if (statusCode == HttpStatusNoContent, etc)

what about this?

    switch (response.httpStatusCode()) {

    case 205: // Reset Content
        // FIXME: a 205 response requires that the requester reset the document view.

    case 204: // No Content
        callPolicyFunction(function, PolicyIgnore);
        return;

    }
Comment 7 Luiz Agostini 2011-03-31 13:26:25 PDT
(In reply to comment #5)
> > Call me crazy but what about?:
> > enum {
> >    HttpStatusNoContent = 204
> > }
> > ...
> > if (statusCode == HttpStatusNoContent, etc)
> 
> I was complaining about something like that some days ago. I'm actually pretty sure that such a thing already exists in WebKit or at least used to.

I did not find such a thing.
Comment 8 Luiz Agostini 2011-04-01 10:09:49 PDT
Created attachment 87876 [details]
patch
Comment 9 Benjamin Poulain 2011-04-06 07:38:20 PDT
Comment on attachment 87876 [details]
patch

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

I stand by the enum for the Http response code, it is harder to get an enum out of sync with the value, and I hate magic numbers in general :)

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1182
> +

whitespace?

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1185
> +

You should add // Fallthrough,  and have fixme separately somehow. I have the feeling someone will think a "break;" is missing here :)

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1189
> +

whitespace?
Comment 10 Luiz Agostini 2011-04-08 11:41:30 PDT
Created attachment 88848 [details]
patch
Comment 11 WebKit Review Bot 2011-04-08 11:44:05 PDT
Attachment 88848 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:278:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Luiz Agostini 2011-04-08 11:46:28 PDT
Created attachment 88850 [details]
patch
Comment 13 Andreas Kling 2011-04-08 12:12:39 PDT
Comment on attachment 88850 [details]
patch

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

> LayoutTests/ChangeLog:8
> +        Responses which have status code equal to 204 should be ignored.

Oops! We're also changing the behavior of 205 responses without a test or mention.

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:609
>      if (m_webFrame && m_frame->page())
> -        emit loadFinished(m_loadError.isNull());
> +        emit loadFinished(m_loadError.isNull() || m_loadError.isCancellation());

This looks vaguely unrelated, what's this change about, and how can we test it?

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:281
> +    enum HTTPStatusCodes {
> +        NoContent = 204,
> +        ResetContent = 205
> +    };

This enum should be called HTTPStatusCode, and I think we should put it in a central location, something like Source/WebCore/platform/network/HTTPStatusCode.h
There are a number of places in WebKit that would look nicer without such numeric constants inline.
I also think the values should be prefixed with HTTP, i.e HTTPNoContent, HTTPResetContent.
Comment 14 Luiz Agostini 2011-04-08 12:31:28 PDT
(In reply to comment #13)
> (From update of attachment 88850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88850&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Responses which have status code equal to 204 should be ignored.
> 
> Oops! We're also changing the behavior of 205 responses without a test or mention.

This change log is in LayoutTests and the test that I am adding is just about 204. That is why the comment is just about 204.
I did not make tests to 205 because we do not implement its specified behavior fully.

The other change log mentions 205.

> 
> > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:609
> >      if (m_webFrame && m_frame->page())
> > -        emit loadFinished(m_loadError.isNull());
> > +        emit loadFinished(m_loadError.isNull() || m_loadError.isCancellation());
> 
> This looks vaguely unrelated, what's this change about, and how can we test it?

If a request is sent to the server and it returns to the client a 204 the client screen will not be updated but no error occurred. Why would loadFinished notify an error.
It is actually been tested. Without this change the layout test that this patch adds times out. :)

> 
> > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:281
> > +    enum HTTPStatusCodes {
> > +        NoContent = 204,
> > +        ResetContent = 205
> > +    };
> 
> This enum should be called HTTPStatusCode, and I think we should put it in a central location, something like Source/WebCore/platform/network/HTTPStatusCode.h
> There are a number of places in WebKit that would look nicer without such numeric constants inline.
> I also think the values should be prefixed with HTTP, i.e HTTPNoContent, HTTPResetContent.

ok.
Comment 15 Luiz Agostini 2011-04-08 12:33:08 PDT
the tests names seem to be wrong. 204 and 205 are not errors!
Comment 16 Luiz Agostini 2011-05-03 17:24:30 PDT
Created attachment 92177 [details]
patch
Comment 17 Alexis Menard (darktears) 2011-05-03 17:27:51 PDT
Comment on attachment 92177 [details]
patch

Looks nicer. Now I think you can post a mail on webkit-dev to see if some people are interested to use the enum in their port. A least to made them aware.
Comment 18 Kenneth Rohde Christiansen 2011-05-04 01:44:53 PDT
Comment on attachment 92177 [details]
patch

LGTM
Comment 19 WebKit Commit Bot 2011-05-04 11:34:27 PDT
Comment on attachment 92177 [details]
patch

Rejecting attachment 92177 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2

Last 500 characters of output:
imerredirect-subframeload.html -> failed
.
http/tests/navigation/useragent.php -> failed
.
http/tests/navigation/window-open-adds-history-item2.html -> failed
.
http/tests/navigation/window-open-adds-history-item.html -> failed
http/tests/plugins .
http/tests/plugins/cross-frame-object-access.html -> failed

Exiting early after 20 failures. 22720 tests run.
643.46s total testing time

22700 test cases (99%) succeeded
20 test cases (<1%) had incorrect layout
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8571085
Comment 20 WebKit Commit Bot 2011-05-04 11:34:31 PDT
Created attachment 92289 [details]
Archive of layout-test-results from cr-jail-4

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-4  Port: Mac  Platform: Mac OS X 10.6.7
Comment 21 Simon Fraser (smfr) 2011-05-04 14:14:19 PDT
This change may have caused the 400+ new failures on the Leopard bot:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r85783%20(31877)/results.html Please investigate.
Comment 22 WebKit Review Bot 2011-05-04 15:07:45 PDT
http://trac.webkit.org/changeset/85774 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
http/tests/navigation/response204.html
Comment 23 Luiz Agostini 2011-05-05 09:48:51 PDT
(In reply to comment #21)
> This change may have caused the 400+ new failures on the Leopard bot:
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r85783%20(31877)/results.html Please investigate.

I was expecting the new test (http/tests/navigation/response204.html) to fail in some platforms but it should fail alone. It seems that for Mac and Win if the new test fail  then tons of other tests fail as well.

The new test was skipped (r85784) for Mac, Win and GTK. Only Qt and Chromium are running it.
Comment 24 Luiz Agostini 2011-05-05 09:50:05 PDT
commited. http://trac.webkit.org/changeset/85774