Server sent events, no reconnection

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
6 years ago
10 days ago

People

(Reporter: medikoo+mozilla.org, Assigned: wfernandom2004)

Tracking

21 Branch
mozilla36
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [bugday-20131002])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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)

Updated

6 years ago
Blocks: 338583

Updated

6 years ago
Whiteboard: [bugday-20131002]

Comment 1

5 years ago
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

Comment 2

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

Comment 3

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

Updated

5 years ago
OS: Mac OS X → All
(Assignee)

Updated

5 years ago
Assignee: nobody → wfernandom2004
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

5 years ago
Posted patch v1 (obsolete) — Splinter Review
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...
Attachment #8438844 - Flags: review?(bugs)
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.
Attachment #8438844 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

5 years ago
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.)
(Assignee)

Comment 9

5 years ago
Posted patch v2 (obsolete) — Splinter Review
Attachment #8438844 - Attachment is obsolete: true
Attachment #8439640 - Flags: review?(bugs)
(Assignee)

Comment 10

5 years ago
Posted patch v2.1 (obsolete) — Splinter Review
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.
Attachment #8439640 - Attachment is obsolete: true
Attachment #8439640 - Flags: review?(bugs)
Attachment #8439656 - Flags: review?(bugs)
Comment on attachment 8439656 [details] [diff] [review]
v2.1

Someone from Necko team should look at EventSource::OnStopRequest error code usage.
Attachment #8439656 - Flags: review?(jduell.mcbugs)
Attachment #8439656 - Flags: review?(bugs)
Attachment #8439656 - Flags: review+

Comment 12

5 years ago
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.
Attachment #8439656 - Flags: review?(jduell.mcbugs) → review?(hurley)

Comment 13

5 years ago
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?
Attachment #8439656 - Flags: review?(hurley) → review?(honzab.moz)
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
Attachment #8439656 - Flags: review?(honzab.moz) → review-

Updated

5 years ago
Flags: needinfo?(wfernandom2004)
(Assignee)

Comment 16

5 years ago
>  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?
Flags: needinfo?(honzab.moz)
(Assignee)

Updated

5 years ago
Flags: needinfo?(wfernandom2004)
(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)
Flags: needinfo?(honzab.moz)

Updated

5 years ago
Flags: needinfo?(wfernandom2004)
(Assignee)

Comment 18

4 years ago
Posted patch v2.2Splinter Review
Honza, it is reviewed according your comments.
Attachment #8439656 - Attachment is obsolete: true
Flags: needinfo?(wfernandom2004)
Attachment #8519419 - Flags: review?(honzab.moz)
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.
Attachment #8519419 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 20

4 years ago
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.
Attachment #8519419 - Flags: checkin?(bugs)
Attachment #8519419 - Flags: checkin?(bugs) → checkin+
https://hg.mozilla.org/mozilla-central/rev/6dbd7d78e775
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Comment 24

4 years ago
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!
(Assignee)

Comment 26

4 years ago
You are very welcome! :)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.