After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 587907 - body-less responses seem to not get content-sniffed/got-chunk/got-body emitted
body-less responses seem to not get content-sniffed/got-chunk/got-body emitted
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: HTTP Transport
2.27.x
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2009-07-06 17:37 UTC by Gustavo Noronha (kov)
Modified: 2009-07-10 17:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix, not yet ready for pushing (7.95 KB, patch)
2009-07-10 12:43 UTC, Gustavo Noronha (kov)
needs-work Details | Review
fix for the hangs on chunked encoding (3.35 KB, patch)
2009-07-10 13:33 UTC, Gustavo Noronha (kov)
committed Details | Review

Description Gustavo Noronha (kov) 2009-07-06 17:37:35 UTC
After WebKitGTK+ started using the new content sniffing feature in soup three layout tests started failing, and the cause seems to be that 412 responses are not getting a content-sniffed emission. This is the output of debugging calls I added to got-headers, got-chunk, and content-sniffed handlers in our soup backend:

+** (DumpRenderTree:6435): DEBUG: got-headers: 412
+** (DumpRenderTree:6435): DEBUG: got-headers: 304
+** (DumpRenderTree:6435): DEBUG: got-headers: 200
+** (DumpRenderTree:6435): DEBUG: content-sniffed: 200
+** (DumpRenderTree:6435): DEBUG: sniffed
+** (DumpRenderTree:6435): DEBUG: got-chunk: 200

As you can see, 412 and 304 do not get content-sniffed, nor got-chunk. This may have been caused by the new content sniffing code, but it is likely that it is just being exposed by the fact that I now rely on content-sniffed to report headers, instead of relying on got-headers.
Comment 1 Dan Winship 2009-07-06 19:15:32 UTC
There is no special handling for status 412 anywhere; if got-sniffed is not emitted for that test, it has something to do with the exact specific response returned in that test, and not anything to do with it being a 412.

304, OTOH, is one of the magic responses that never has a response body.

It appears that the check after got_body in io_read() is broken; io->delayed_chunk_data will only be set if it has read some data, so if there is no response body (either because the status code never has a response body or just because the server didn't send a response), then we don't emit got-sniffed. 
It needs to be checking io->delay_got_chunks first.

Presumably the 412 response in this test has "Content-Length: 0", and so it also triggers the bug. Assuming this is the case, we should add a test to tests/sniffing-test for this (0-length response, via both Content-Length and chunked).
Comment 2 Gustavo Noronha (kov) 2009-07-10 12:43:15 UTC
Created attachment 138187 [details] [review]
fix, not yet ready for pushing

I say this is not yet ready to be pushed because I found another problem while working on the tests for this problem, and I believe that needs to go into a commit before this one (which will have a small conflict with the patch).
Comment 3 Gustavo Noronha (kov) 2009-07-10 13:33:48 UTC
Created attachment 138190 [details] [review]
fix for the hangs on chunked encoding

Let me abuse this bug a little bit, posting also this fix here =).
Comment 4 Dan Winship 2009-07-10 14:32:57 UTC
(In reply to comment #2)
> Created an attachment (id=138187) [edit]
> fix, not yet ready for pushing

There are a few problems here, and now that I'm looking more carefully at the existing code, I see a few more problems there too. Among other things, you can't safely refer to io->whatever in between a signal emission and the RETURN_IF_CANCELLED_OR_PAUSED. I'm going to fix up this patch; feel free to commit the chunked fixes.
Comment 5 Gustavo Noronha (kov) 2009-07-10 14:46:15 UTC
(In reply to comment #4)
> There are a few problems here, and now that I'm looking more carefully at the
> existing code, I see a few more problems there too. Among other things, you
> can't safely refer to io->whatever in between a signal emission and the
> RETURN_IF_CANCELLED_OR_PAUSED. I'm going to fix up this patch; feel free to
> commit the chunked fixes.

Hmm, OK. Thanks for looking. 

Comment 6 Dan Winship 2009-07-10 17:25:29 UTC
should be fixed in master