Bug 42529 – [Qt] Qt WebKit updates view on HTTP 204 response

• Component: Page Loading
• Status: RESOLVED FIXED

[Alex Plotnick]

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.

[Benjamin Poulain]

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?".

[Luiz Agostini]

Created attachment 87763 [details]
patch

[Benjamin Poulain]

Comment on attachment 87763 [details]
patch

A test would be nice.

[Benjamin Poulain]

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)

[Kenneth Rohde Christiansen]

> 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.

[Luiz Agostini]

(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;

    }

[Luiz Agostini]

(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.

[Luiz Agostini]

Created attachment 87876 [details]
patch

[Benjamin Poulain]

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?

[Luiz Agostini]

Created attachment 88848 [details]
patch

[WebKit Review Bot]

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.

[Luiz Agostini]

Created attachment 88850 [details]
patch

[Andreas Kling]

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.

[Luiz Agostini]

(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.

[Luiz Agostini]

the tests names seem to be wrong. 204 and 205 are not errors!

[Luiz Agostini]

Created attachment 92177 [details]
patch

[Alexis Menard (darktears)]

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.

[Kenneth Rohde Christiansen]

Comment on attachment 92177 [details]
patch

LGTM

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Simon Fraser (smfr)]

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.

[WebKit Review Bot]

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

[Luiz Agostini]

(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.

[Luiz Agostini]

commited. http://trac.webkit.org/changeset/85774