User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130116 Firefox/21.0 Build ID: 20130116031003 Steps to reproduce: Event source object is configured traditional way: var source = new EventSource('/url/'); Actual results: Connects properly, but if I stop and start the server that maintains connection Firefox doesn't try to reconnect (waited at least few minutes) Expected results: It should try to reconnect in some intervals - Chrome in following case connects nearly instantly when server is back. Spotted in lastest Release (17) and Nighlty (21.0a1)

For me Firefox doesn't reconnects even immediately after killing a connection. Live basic SSE demo: http://server-sent-events-demo.herokuapp.com/ Description of another test case in detail with basic implementation source code: http://stackoverflow.com/questions/20837460/firefox-doesnt-restore-server-sent-events-connection

The bug still persists in today's Nightly 32.0a1 (2014-05-13)

And it isn't limited to Mac OS X, also happens on GNU/Linux and Windows.

I've tested in my computer, locally, playing with my Apache server. I tried to write a xpcshell automated test, in order to be able to create/start/stop a dedicated httpd server, as in netwerk/test/unit/test_xmlhttprequest.js. However, it seems like support to EventSource in SandBox has been dropped in bug 819639. I'm without any further ideas...

Hmm, the spec is vague atm. http://www.w3.org/TR/eventsource/ mentions still 500, 502, 503, 504, but http://dev.w3.org/html5/eventsource/#dom-eventsource-readystate doesn't have those, yet it has "Any other HTTP response code not listed here, as well as the cancelation of the fetch algorithm by the user agent (e.g. in response to window.stop() or the user canceling the network connection manually) must cause the user agent to fail the connection."

Comment on attachment 8438844 [details] [diff] [review] v1 Or actually, per spec 50x shouldn't reestablish connection.

Olli, should we wait the spec freeze, or just let's remove the 50x from the patch and move on?

http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#processing-model-8 is the spec we should follow. (And HTML spec is a living standard so it will never be frozen.)

I got some intermittent failures in test 3.h.1, so I increased its timeout. While reviewing it, I find a code issue there, so I fixed that as well.

Comment on attachment 8439656 [details] [diff] [review] v2.1 Someone from Necko team should look at EventSource::OnStopRequest error code usage.

Comment on attachment 8439656 [details] [diff] [review] v2.1 Review of attachment 8439656 [details] [diff] [review]: ----------------------------------------------------------------- Nick, I'm going to punt this to you for now. If you don't feel you know the laundry list of error codes here well enough, you can pass this on to Patrick or Honza.

Comment on attachment 8439656 [details] [diff] [review] v2.1 Review of attachment 8439656 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I'm definitely not up enough on the error codes to be useful here. Honza, since Patrick is out, can you take this?

I'll gladly do the review here, but I first have to learn the EventSource code.

Comment on attachment 8439656 [details] [diff] [review] v2.1 Review of attachment 8439656 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/EventSource.cpp @@ +353,5 @@ > + nsresult status; > + aRequest->GetStatus(&status); > + > + uint32_t httpStatus; > + httpChannel->GetResponseStatus(&httpStatus); move this before the if () you are using httpStatus at. @@ +478,5 @@ > + aStatusCode != NS_ERROR_REDIRECT_LOOP && > + aStatusCode != NS_ERROR_ENTITY_CHANGED && > + aStatusCode != NS_ERROR_UNKNOWN_HOST && > + aStatusCode != NS_ERROR_DNS_LOOKUP_QUEUE_FULL && > + aStatusCode != NS_ERROR_UNKNOWN_PROXY_HOST) { you should IMO have a white list (errors that allow resestablish) instead. Candidates: NS_ERROR_NET_RESET NS_ERROR_NET_TIMEOUT NS_ERROR_DNS_LOOKUP_QUEUE_FULL NS_ERROR_UNKNOWN_HOST NS_ERROR_NET_INTERRUPT NS_ERROR_NET_PARTIAL_TRANSFER

> move this before the if () you are using httpStatus at. Sorry, I didn't understand what you meant, nor the reason. > you should IMO have a white list (errors that allow resestablish) instead. Hmm, it seems like the proposed code is already a white list, isn't it? > Candidates: Ok. But your proposed list seems incomplete. For instance, why not NS_ERROR_CONNECTION_REFUSED or NS_ERROR_PROXY_CONNECTION_REFUSED?

(In reply to Wellington Fernando de Macedo from comment #16) > > move this before the if () you are using httpStatus at. > Sorry, I didn't understand what you meant, nor the reason. The code best should look like: nsresult status; rv = aRequest->GetStatus(&status); NS_ENSURE_SUCCESS(rv, rv); if (NS_FAILED(status)) { // EventSource::OnStopRequest will evaluate if it shall either reestablish // or fail the connection return NS_ERROR_ABORT; } uint32_t httpStatus; rv = httpChannel->GetResponseStatus(&httpStatus); NS_ENSURE_SUCCESS(rv, rv); if (httpStatus != 200) { DispatchFailConnection(); return NS_ERROR_ABORT; } nsAutoCString contentType; rv = httpChannel->GetContentType(contentType); NS_ENSURE_SUCCESS(rv, rv); if (!contentType.EqualsLiteral(TEXT_EVENT_STREAM)) { DispatchFailConnection(); return NS_ERROR_ABORT; } > > > you should IMO have a white list (errors that allow resestablish) instead. > Hmm, it seems like the proposed code is already a white list, isn't it? Ah!! There are '!='... OK, my bad. > > > Candidates: > Ok. But your proposed list seems incomplete. For instance, why not > NS_ERROR_CONNECTION_REFUSED or NS_ERROR_PROXY_CONNECTION_REFUSED? I would personally remove the following errors, unless you document reasons to leave them (I would actually expect good explanation for all of your choices): NS_BINDING_FAILED (this is cancel, but I may be confused here, maybe this has its place in the whitelist) NS_ERROR_NO_CONTENT NS_ERROR_CORRUPTED_CONTENT NS_ERROR_OFFLINE (seems like you want to poll for being back online. we have events for this) NS_ERROR_REDIRECT_LOOP (this is IMO not a temporary error) NS_ERROR_ENTITY_CHANGED (you will get this one only when resuming and the entity you get in 206 has changed or got 4xx) NS_ERROR_UNKNOWN_HOST (this error will probably not go away soon) NS_ERROR_UNKNOWN_PROXY_HOST (as well this one)

Honza, it is reviewed according your comments.

Comment on attachment 8519419 [details] [diff] [review] v2.2 Review of attachment 8519419 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. If you want to double check the white-list of errors is OK, ask Patrick McManus.

Comment on attachment 8519419 [details] [diff] [review] v2.2 Hi Olli, Could you please upload this to TryServer, and if everything fine, to check-in into trunk? Regards.

Wellington Fernando de Macedo, thank you for fixing this bug. Tested today on Nightly, in case of interrupted connection it works great. Just to note, after returning from offline mode it doesn't reconnect automatically. I suppose it is intended behavior.

You may want to file another bug about offline. It is a very different case technically. And yes, thanks Wellington, you're great!

You are very welcome! :)