628832 - The Radio Player on the web Bassdrive doesn't work on Firefox 4

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[djlvzqrlni]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

When you enter in the web and push the button web player to listen the radio, in the new window when yo select Play there isn't music. In Firefox 3.6.X this works correctly.

Reproducible: Always

Steps to Reproduce:
1.Enter in the web http://www.bassdrive.com/v2/
2.Select Web Player
3.In the new window, push the play button.

Comment 1

[Alice0775 White]

This problem also happens on ubuntu.

Regression window(m-c cached hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/5ae1f2fa0d9f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101202 Firefox/4.0b8pre ID:20101202133058
Fails:
http://hg.mozilla.org/mozilla-central/rev/f1a5bea1d022
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101202 Firefox/4.0b8pre ID:20101202170354
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5ae1f2fa0d9f&tochange=f1a5bea1d022

Comment 2

[Alice0775 White]

In local build,

build from f1a5bea1d022 : fails
build from 37aefdd76a22 : fails
build from 3f004b291c65 : works

So, This is regressed by 37aefdd76a22	Michal Novotny — bug 363109 - body of HTTP 304 response is treated as a HTTP/0.9 response to subsequent HTTP request. r=biesi, sr=bz, a=blocker

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: pcap of audio stream

this is the http flow in question as a pcap - the response is just a streamed audio document without an HTTP header, but it does have something that kind of looks like one.. several CRLF terminated name-value pairs and then an empty CRLF line. (but there is no HTTP response line).. perhaps that is confusing things.

after that header is streamed mp3 without a fin (and of course no content-length or chunked as there is no http response header)

ICY 200 OK
icy-notice1:<BR>This stream requires <a href="http://www.winamp.com/">Winamp</a><BR>
icy-notice2:SHOUTcast Distributed Network Audio Server/Linux v1.9.8<BR>
icy-name:Bassdrive - Worldwide Drum and Bass Radio
icy-genre:Techno
icy-url:http://www.bassdrive.com
content-type:audio/mpeg
icy-pub:1
icy-br:128
[empty line]
data

Comment 4

[Boris Zbarsky [:bzbarsky]]

"ICY 200 OK", eh?  :(

Comment 5

[Patrick McManus [:mcmanus]]

I think I know what is going on here - we set disallow09 on the connection info for the shoutcast server based on having received a valid HTTP/1.0 in a previous transaction. (the previous transaction is for flash's crossdomain.xml file).

Then in a subsequent transaction to the same host for the streaming mp3 we get a .9 response - since disallow09() has been called we just keep reading in the data infinitely looking for a response header. Even after closing the popup FF continues to download (and still not play anything).

This is why if you try and just put the mp3 url into the browser after a clean startup (without the flash) it all runs fine.

two things to fix here imo:

* disallow09 should not be on mConnInfo - it is a property of the nshttpconnection, not the host

* even when disallow09 is in effect - we shouldn't look forever. Tolerating some junk before the response headers is reasonable, searching through an infinite stream is meaningless (HTTP/1 will appear eventually even in random data).

I'll try and fix it.

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: 628832-http09-recognition-v1

* makes disallow09 a property of the TCP connection, not the server, which is how it is described in the original bug

* when looking through arbitrary amounts of junk looking for "HTTP/1", give up after a generous 128KB of crud.. the use case for this is basically skipping over content in 304 (or similar responses) that carry a mesasge even though they are illegal.. these messages aren't going to be that large and in the case of an open-ended response we need to call it a day somewhere.

This passes the xpcshell tests as well as the tests Michael laid out in https://bugzilla.mozilla.org/show_bug.cgi?id=363109#c14

fwiw - I'm not sure the "I've seen HTTP >= 1 so I won't see 0.9 logic now" really holds water, but it remains in place here. for instance, what stops us from doing the crossdomain.xml request, receiving a keep-alive 1.0 level response and then sending the mp3 request down the same pipe which presumably would generate the 0.9 response? The assumption is a 0.9 server is always a 0.9 server - but we're seeing here that isn't true, and truthfully I don't think there really are any 0.9 servers left - just a bunch of gatewayed/cgi/plugin resources that dump data on the wire. In this case the main server doesn't do keep alive so we're cool, but I don't see why it couldn't.

I wanted to code this so that when we hit the 128KB limit we revert to calling the response 0.9 from the beginning, but we cannot do that because while we process 128KB we don't buffer it all and so the beginning of the stream has been lost.

It might be best to, in a separate > ff4.0 bug, decide if we can tolerate searching for HTTP/1 in just a bufferable amount of space (8KB?) and then call the response-header-not-found case HTTP/0.9. That would be closer to the FF 3.6 behavior (which essentially declared the search space to be however much was in the first call to parseheaders())

Comment 7

[Boris Zbarsky [:bzbarsky]]

So...  I think the patch in bug 363109 really did mean to pin the HTTP version for the server, not for the connection.  But yes, just doing it for the connection may make sense.

> these messages aren't going to be that large 

How do we know?  The way 304 responses with data happen most commonly is that PHP will dump the entire contents of the page into the 304 response for that page, by default.  :(

> fwiw - I'm not sure the "I've seen HTTP >= 1 so I won't see 0.9 logic now"
> really holds water

I'd be just fine with backing out the fix we landed for bug 363109 and falling back on my preferred fix for that bug: if a 304 is followed by garbage bytes, kill the connection...  But you seemed to have issues with the resulting performance.

> The assumption is a 0.9 server is always a 0.9 server - but we're seeing here
> that isn't true

Indeed.

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to comment #7)
> So...  I think the patch in bug 363109 really did mean to pin the HTTP version
> for the server, not for the connection.  But yes, just doing it for the
> connection may make sense.
> 

I think so - the point of the patch was essentially to skip the illegal body between a 304 (or similar) and the next http response.. so what you want to mark is that you have already seen the first event in that sequence. Its more the state of the connection than a property of the server.

> How do we know?  The way 304 responses with data happen most commonly is that
> PHP will dump the entire contents of the page into the 304 response for that
> page, by default.  :(

well, we can't know for certain of course because any data is illegal - thinking too hard about where to draw the limit on a buggy server just drives you mad :)

but I think you basically back me up - the contents of the PHP page are likely to be text or some other kind of markup which generally will be "not too large" - especially compared with the open ended streaming mp3 this case shows. My definition of not too large is 128KB, which is a lot of markup, but we don't buffer it all so the number can be bigger if you like.. just something less than infinite.

Comment 9

[Patrick McManus [:mcmanus]]

> I think so - the point of the patch was essentially to skip the illegal body
> between a 304 (or similar) and the next http response.. so what you want to
> mark is that you have already seen the first event in that sequence. Its more
> the state of the connection than a property of the server.
> 

Thinking about this more - maybe instead of "disallow 0.9" we want to just mark "last transaction was no-content" and only do the new search the stream beyond the first read() behavior in that case. That's more conservative than what is currently going on and still addresses the original bug.

I can rearrange the patch to do that if you like that more. I think I like that more. It would also catch the case of a server responding with a body to a HEAD request, which I know I have seen from time to time, if FF ever generates HEAD.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I like that more too.  Let's do that.

We stopped using HEAD, period, precisely because servers were so broken for it.

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: 628832-http09-recognition-v2

update based on comments 9 and 10

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 507266 [details] [diff] [review]
628832-http09-recognition-v2

s/LastTransactionNoContent/LastTransactionExpectedNoContent/ perhaps?

>+#define MAX_RESP_HEADER_JUNK (1024 * 128)

MAX_INVALID_RESPONSE_BODY_SIZE ?

mHeaderSkipped should perhaps be mInvalidResponseBytesRead, with a better comment?

If we really cared, we could buffer up the invalid stuff and then fall back on HTTP 0.9... I'm not sure I really care.

r=me with those nits.

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: 628832-http09-recognition-v3 r=bzbarsky

reflects comment 12

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 507439 [details] [diff] [review]
628832-http09-recognition-v3 r=bzbarsky

a=me, but please s/SZ/SIZE/.  The two letters you're saving are not worth the readability loss!

Comment 15

[Patrick McManus [:mcmanus]]

(In reply to comment #14)
>
> a=me, but please s/SZ/SIZE/.  The two letters you're saving are not worth the
> readability loss!

it s/SZ/SIZE/ adds another line to the comparison thanks to the 80 column rule - which to me is a much bigger readability problem than recognizing a common convention.

would you still like it changed?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, please.  Ignore the 80-column rule in this case!

Comment 17

[Johnny Stenback (:jst)]

This seems blocker worthy, even.

Comment 18

[Patrick McManus [:mcmanus]]

Attachment: http09-recognition-v4 r=bzbarsky a=blocking-2.0

updated per comment 14/16

Comment 19

[Patrick McManus [:mcmanus]]

http://hg.mozilla.org/mozilla-central/rev/9d99616ce506

Comment 21

[Matt Wilkinson]

I have verified that this bug is fixed on Firefox 4 Beta 11 on Linux x86_64.

Comment 22

[Michal Novotny [:michal]]

(In reply to comment #9)
> Thinking about this more - maybe instead of "disallow 0.9" we want to just mark
> "last transaction was no-content" and only do the new search the stream beyond
> the first read() behavior in that case. That's more conservative than what is
> currently going on and still addresses the original bug.

Unfortunately, the problem isn't limited to no-content reply. See bug 375701.