[webkit-reviews] review denied: [Bug 60875] [SOUP] Abnormal operation if server sends 5xx status code without HTTP body : [Attachment 93736] add details on ChangeLog

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 60875: [SOUP] Abnormal operation if server sends 5xx status code without
HTTP body
https://bugs.webkit.org/show_bug.cgi?id=60875

Attachment 93736: add details on ChangeLog
https://bugs.webkit.org/attachment.cgi?id=93736&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93736&action=review

Do you have an example of a site this breaks? Ideally this change would come
with a test case. Is the issue that when there is a 5xx error and no body, soup
doesn't fail the load and it should? What do other network stacks do?

> Source/WebCore/ChangeLog:23
> +	   Handle status code 5xx after receiving HTTP body
> +	      - Background
> +	       1) Webkit soup port premises that server always sends error page
in case of status code 5xx.
> +	       2) But, sometimes there is no HTTP body for error page. 
> +	       3) In that case, Webkit tries to download the HTTP response even
if there is no HTTP body. (abnormal operation)
> +	      - Modified algorithm;
> +	       1) libsoup sends status code 5xx (server error)
> +	       2) Webkit soup port orders to accumulate HTTP body chunks to
libsoup.
> +	       3) Webkit soup port ignores gotHeadersCallback,
contentSniffedCallback and gotChunkCallback.
> +	       4) Webkit soup port checks there is HTTP body or not on
sendRequestCallback and;
> +		   4-1) if there is HTTP body, calls didReceiveResponse()
> +		   4-2) if there is no HTTP body, calls didFail() - prevent the
abnormal operation
> +	      - With this modification;
> +	       1) If server sends error page with HTTP body, Browser will show
the received error page.
> +	       2) If server sends no error page, that is, no HTTP body, Browser
will show its own error page from each port.
> +

This seems okay, but typically ChangeLogs are in paragraph form. Take a look at
trac.webkit.org for inspiration.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:411
> +	       || ((message &&
SOUP_STATUS_IS_SERVER_ERROR(message->status_code)) && (!message->response_body
|| !message->response_body->length))

Do you mind splitting this off into a helper function called something like
statusIsServerErrorAndHaveNotReceivedBody?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:422
> +	   || ((message && SOUP_STATUS_IS_SERVER_ERROR(message->status_code))
&& (!message->response_body || !message->response_body->length))) {

You can also use it here.