User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3 Build Identifier: Remote Events are regular DOM events that are created by a remote server and dispatched locally. A full specification and an example can be found on the WHATWG site: http://www.whatwg.org/specs/web-apps/current-work/#scs-server-sent Remote events is a #6 priority for the XUL development team http://wiki.mozilla.org/XUL:Priority_List Reproducible: Always

This is my first non-trivial contribution to Mozilla, so I'm sure I've violated some coding standards. Please let me know what I can do to make this a better patch.

==> DOM Events

Clayton: any chance you can attach the whole thing as an uncompressed patch to mozilla code? I can help you with that privately, if needed.

(In reply to comment #1) > Created an attachment (id=222661) [edit] > Implementation of Remote Events. Several source files, a patch, and > documentation included > > This is my first non-trivial contribution to Mozilla, so I'm sure I've violated > some coding standards. Please let me know what I can do to make this a better > patch. > r= jst@mozilla.org

Finally updated this patch. The patch is made from the trunk, and now includes the new files and the patch all in a single text file. There are significant bugs, but it works for the simple example I've concocted. I'll post that demo shortly. BTW, if you're searching for how to add new files to your patch without having CVS write access, it is described at the bottom of this page: http://developer.mozilla.org/en/docs/Creating_a_patch

There is a README file in the top directory which explains how to run the demo.

Once the patch is ready for reviews, please request one from the module owner. See http://www.mozilla.org/hacking/life-cycle.html and http://www.mozilla.org/owners.html

Clayton, is the patch you posted on 9-26 ready to be reviewed?

many minor, mostly cosmetic changes from the previous patch. I checked out code today (11-30-06), applied the patch, and built a clean firefox successfully, so I think it's ready for review.

This readme should help developers who want to finish/expand this feature. It takes a top-down high level look at the changes I made.

(In reply to comment #9) > Clayton, is the patch you posted on 9-26 ready to be reviewed? > The new one (11-30) is. I've marked it as such.

Comment on attachment 247037 [details] [diff] [review] Patch as of 11-30-06 review+ would mean review is granted... you probably want to set review? and put in the email address of one of the DOM owners/peers.

(In reply to comment #13) > (From update of attachment 247037 [details] [diff] [review] [edit]) > review+ would mean review is granted... you probably want to set review? and > put in the email address of one of the DOM owners/peers. > Thanks Boris. New territory for me.

I've been looking through these changes and I've got a bunch of comments. Some nits, some changes requested and some questions that need to be answered before we can take this into the tree. My first review pass basically only took me through the Mozilla changes, not the meat of the new code (i.e. not nsDOMRemoteEventSource.{cpp,h} in detail), and given the size and nature of this change it'll take a couple of iterations, so be prepared :) First a couple of high-level things: Indentation: Keep with the style around the code you're changing, and never *ever* use tabs, use spaces. I've commented on some of the indentation issues in the patch in my review feedback (that I'll attach). Static variables: In mozilla code you can't use statics of types that have ctors/dtors, so the code in nsDOMRemoteEventSource that uses static nsCStrings needs to change to not use static nsCStrings. Static const char * etc is fine, so I'd recommend looking at using something like that instead (or char[] and come up with some way to have the compiler tell an nsDependentCString about the length on construction to avoid strlen()'s all over the place). I mention in my review comments that we need to figure out the trusted/untrusted event thing. What that means is that every event carries a bit that says whether or not the event is trusted. If an event is trusted it either came from a direct user action (i.e. a mouse move or click etc), or it came from trusted code reacting to something that happened in the browser. Untrusted events are events that are synthesized by untrusted script (e.g. script on a webpage). So we need to decide what that means for remote events. My initial thought is that remote events are all untrusted since we don't know whether the server that's sending the event is trusted or not. This especially matters for server sent events that translate into standard DOM events. Another thing that we need to figure out is what event sources to we permit a page to use. Generally a HTTP page can only access data from another HTTP page from the same domain (document.domain gives the pages some level of control over this). If we don't do this we're exposing our users to attacks from the public internet to steal data from HTTP servers behind the users firewall (aka the Princeton attack). To prevent that we'd need to limit event sources to same origin. That's what I got for now, I'd suggest we continue by figuring out the above (including the review comments I'm about to attach), make a new patch that reflects that and go from there. The review comments I'm about to attach are in the form of a diff against a tree with the patch applied.

I had some concerns regarding when we remove the source. There are three scenarios that come to mind: 1. A source is attached to an element in a displayed document. The user then navigates away from the page and the document is destroyed. In this case it seems clear that the source should be removed from the element. 2. A source is attached to an element in a displayed document. The user then navigates away from the page but the document is put in the fast-back cache and is therefore not destroyed. Should the listener be removed? And if so, should it be reattached if the user navigates back? 3. A source is attached to an element in a document created by the DOMParser. When should the source be removed from this element? The more I think about it, the more I think the solution is to make the source not hold a strong reference to the element and simply stop the source as soon as the element dies.

Not holding a strong reference won't solve the fast-back issue though. If we don't do anything what will happen is that events will fire on the suspended-life document and handlers in there will execute. However many will probably fail to execute properly since the window is in suspended-life state. Additionally, if the eventhandler were to modify the DOM we'd remove the document from the fast-back cache. A few possible solutions are: A) The simplest solution would be to simply not put documents with remote sources in the fast-back cache. B) Another solution would be to buffer all incoming events and fire them if/when the user returns to the page. C) Yet another solution would be to drop the network connection when the user leaves the page and reconnect when the user goes back. I sort of like this solution since this way the server will know not to send more events and we won't get a torrent of events when the user goes back as in B).

Johnny, thanks for the review. I'll get back to you with changes, not immediately. I'm on vacation until mid-December.

Hi, I'm continuing this implementation. This patch isn't working yet. However, what I've done until now is: * Updating the old patch in order to let it up-to-date * Adding support for the event-source tag (also, I added an target attribute) * Replacing the nsIDOMRemoteEvent interface with the nsIDOMMessageEvent interface When it be working I intend: * implementing a nsIDOMMessageListener (in order to provide the addEventListener("onmessage", function(){}) entry) * implementing a nsEventSourceManager class (I think, similar to the nsEventListenerManager class), that will replace the nsDOMRemoteEventSource class

(In reply to comment #20) > When it be working I intend: > * implementing a nsIDOMMessageListener (in order to provide the > addEventListener("onmessage", function(){}) entry Why? We already have nsIDOMEventTarget which has add/removeEventListener method and nsIDOMEventListener which is implemented by the event listeners (and because the interface has |function| keyword, Javascript functions can be used as nsIDOMEventListener objects) > * implementing a nsEventSourceManager class (I think, similar to the > nsEventListenerManager class), that will replace the nsDOMRemoteEventSource > class Well, I guess nsEventSourceManager won't be too similar to nsEventListenerManager. nsEventListenerManager stores all the event listeners, nsEventSourceManager is about handling network connections or something. Btw, could you call it something else than nsEventSourceManager, since nsEventListenerManager is occasionally called ELM, nsEventStateManager is ESM. Perhaps nsRemoteEventSourceManager, so it could be called RESM. (Well, depends on what the class is going to do.) Note, in some cases the old patch uses strange (and even wrong) coding conventions. But read the comments by Jonas and Johnny.

So what is the compelling use case for this feature? I.e. do we really want this feature in firefox? I've heard that mobile might want this, is that the case? And if so, why?

Of course you want this feature. It makes an entire class of web application possible that isn't super feasible (without extensive server-side work) without it. The basic problem is that Firefox has no way of receiving notifications from a server about events taking place outside of the client's local computer without doing constant polling. Polling means that the server has to batch out-going events by a session, where-as with the server-sent events feature a server can identify a client by connection (which is way easier to implement in certain cases). It relieves latency since there is no polling interval or reconnect penalty for one-off pending event connections. Some services try to do this today by simply using a "never-ending" page load, but this has several problems. For one, Firefox will continue accumulating the page content, even though old content is never referenced again, requiring a periodic reconnect cycle to avoid eating up the user's memory. It's also a pain in the ass to get Firefox to handle those events, since you have to add tags and spaces and padding to get Firefox to "process" the content, which you have to do using script tags in said content since there is no way of getting guaranteed "onUpdate" events. There are tons of excellent uses for this feature. Chat servers are one of the most often mentioned. It allows instant notification of new chat events from chat peers in the channel, and AJAX can be used for posting new comments from the client. Stock ticker sites could use this to pump out an initial set of stock prices and then stream in real-time stock updates to the client without AJAX polling delays. Online games can be written entirely in HTML/CSS/JavaScript, which is currently not possible for a number of games as they need very efficient and polling-free updates. (There is another class of game that needs more efficient client->server notifications... but those can be done today with AJAX + HTTP/1.1 Keep-Alive behavior. They would be better served by the HTML5 "Connection" object feature, though, IMO.) Web mail services could make obvious use of the feature. Ecommerce management interfaces could make use of it. I've worked on a number of intranet apps that could really have used the feature. In short, there are just a lot of placese that would be better off paying the resource cost of an open socket rather than the resource costs of processing full-fledged HTTP requets (and possible TCP connection handshakes).

Ah, I didn't realize that this was the spec defined in the current HTML5 drafts. There has been several more complicated ideas out there that I have been much less interested in implementing.

However we should check with the mobile team that this is something that they can use.

(In reply to comment #21) > Why? We already have nsIDOMEventTarget which has add/removeEventListener > method and nsIDOMEventListener which is implemented by the event listeners > (and because the interface has |function| keyword, Javascript functions > can be used as nsIDOMEventListener objects) ok, I've checked and I agree. > Well, I guess nsEventSourceManager won't be too similar to > nsEventListenerManager. > nsEventListenerManager stores all the event listeners, nsEventSourceManager > is about handling network connections or something. I'm not expressing myself very clearly about the nsEventSourceManager. I tried to write about its structure, don't about what it should do. > > Btw, could you call it something else than nsEventSourceManager, since > nsEventListenerManager is occasionally called ELM, nsEventStateManager is ESM. > Perhaps nsRemoteEventSourceManager, so it could be called RESM. > (Well, depends on what the class is going to do.) > > Note, in some cases the old patch uses strange (and even wrong) coding > conventions. But read the comments by Jonas and Johnny. > I've renamed it to nsRemoteEventSourceManager. Also, while I'm implementing I'm trying to apply these comments. I've finished to add the eventsource tag successfully. Now I'm implementing this class(nsRemoteEventSourceManager) using the old nsEventSource class. If you wish to see my current patch, write me back and then I submit it.

Any updates here?

Yes, there is. I'm posting my current work. I started testing it yesterday. There are still problems, however I think it is ready for code reviews, etc. Wellington.

Hi, I'm posting the last patch I've sent, however updated. It is now tested and working. In order to finish this bug it needs: * Implementing the reconnection time * Adding support for Mouse, Keyboard, etc sent events I will start doing it next week.

Here there are my tests. Notes about it: * the patch has been created using the FIREFOX_3_0_1_RELEASE cvs branch * I've tested only on Linux (Ubuntu). * The server is a php script. The zip contents should be available inside "http://localhost/tests/". Wellington.

Hi, I've finished the work. It's ready for reviews. If you find out some problem, please write me back. Wellington.

(In reply to comment #30) > * the patch has been created using the FIREFOX_3_0_1_RELEASE cvs branch You should use trunk instead ( http://developer.mozilla.org/en/Mozilla_Source_Code_(Mercurial) ), which is where new features are integrated.

Ok, I will use this repository: http://hg.mozilla.org/mozilla-central/. When I finish updating the patch I post it again here. Wellington. (In reply to comment #33) > (In reply to comment #30) > > * the patch has been created using the FIREFOX_3_0_1_RELEASE cvs branch > > You should use trunk instead ( > http://developer.mozilla.org/en/Mozilla_Source_Code_(Mercurial) ), which is > where new features are integrated. >

Hi, this patch is up-to-date using the mercury trunk. I've tested and it is working. Please, disregard the test1.html instruction about the garbage collector. This issue doesn't happen any more. I've forgotten to removed it. Thanks.

Comment on attachment 335297 [details] my tests If possible, tests should be converted to mochitests http://developer.mozilla.org/en/Mochitest

Olli, this patch has the mochitest. Also it contains a correction that I've found out. When calling removeEventSource inside a event listener that has been dispatched by the same eventsource (which is being removed) there was a error. I've checked this for review instead the last one. Wellington.

This is so huge patch, that it will take few iterations to review it. Generically the patch looks great, but few things to change and more tests are needed. I haven't yet reviewed the stream parsing at all. Btw, I'm planning to test whether Server-Sent Events could be useful for testing. Some changes will be needed for that, but the idea is to first record event stream and play it back via server.

The tests should contain also some problematic cases when the input is in fact invalid.

ok Olli, I will work on your comments. However I have some questions: 1) -- >> XXXAssign the result to some local nsCOMPtr<nsIRemoteEventSourceManager> >> XXXand only if the method succeeds .swap() the value with aResult. Doing it the nsContentUtils won't need to have support for creating and managing RemoteEventSourceManagers any more, because only the nsGenericElement uses it. Can I remove the RemoteEventSourceManager support from nsContentUtils? 2) -- Should I remove all printf debugs, or only those that you have listed? 3) -- >>XXXShouldn't change the attribute value. element.getAttribute("src") should >>return the same value as what was used when .setAttribute(...) was called When an eventsource url is empty the spec says to use the "undefined" string. But if you want I change it. 4) -- >>XXXThe latest version doesn't have attribute 'target', I think - >>whatwg.org is down while I'm writing this. Yes, the spec hasn't this attribute. But if you want I remove it too. Also, some answers: 1) -- +nsTArray<nsRemoteEventSourceManager*>* nsRemoteEventSourceManager::sInstances = nsnull; +nsCStringHashSet* nsRemoteEventSourceManager::sIgnoreSrcList = nsnull; XXXWhat are these two? These two are used in order to don't add event sources that have already had some troubles, following the spec: * invalid http response code * denied requests * content-types others than text/event-stream 2) -- >>XXXIs this right? Comparing to "null" I think it is right, because, for example, the onmousemove events have relatedTarget, however others mouse events don't.

(In reply to comment #40) > Doing it the nsContentUtils won't need to have support for creating and > managing RemoteEventSourceManagers any more, because only the nsGenericElement > uses it. Can I remove the RemoteEventSourceManager support from nsContentUtils? "Any object that implements the EventTarget interface must also implement the RemoteEventTarget interface. " So also attribute nodes and textnodes etc should support the interface. > 2) -- > Should I remove all printf debugs, or only those that you have listed? All. Or if you really want to keep some, use #ifdef DEBUG_your_local_username > 3) -- > >>XXXShouldn't change the attribute value. element.getAttribute("src") should > >>return the same value as what was used when .setAttribute(...) was called > > When an eventsource url is empty the spec says to use the "undefined" string. > But if you want I change it. Where does it says so? I don't think 'current declared event source set to "undefined".' means that attribute value should be modified. > Yes, the spec hasn't this attribute. But if you want I remove it too. Remove, please. > > +nsTArray<nsRemoteEventSourceManager*>* nsRemoteEventSourceManager::sInstances > = nsnull; > +nsCStringHashSet* nsRemoteEventSourceManager::sIgnoreSrcList > = nsnull; > XXXWhat are these two? > > These two are used in order to don't add event sources that have already had > some troubles, following the spec: > * invalid http response code > * denied requests > * content-types others than text/event-stream You could add a comment to the code. > > 2) -- > >>XXXIs this right? Comparing to "null" > > I think it is right, because, for example, the onmousemove events have > relatedTarget, however others mouse events don't. But is that specified somewhere? Would it make more sense to check that if there isn't value for relatedTarget, it means that it is null.

Olli, I've updated the patch with your comments. I've added more tests, too. I think it is good for the 2nd phase of review. Wellington

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) >- // Check img src scheme >+ if (aTag == eHTMLTag_eventsource) >+ return NS_ERROR_ILLEGAL_VALUE; >+ >+ // Check img src scheme 2 extra spaces before '//' > ~EventListenerManagerMapEntry() > { >- NS_ASSERTION(!mListenerManager, "caller must release and disconnect ELM"); >+ NS_ASSERTION(!mListenerManager, >+ "caller must release and disconnect ELM"); > } Why this change? >+nsContentUtils::TraverseManagers(nsINode *aNode, >+ nsCycleCollectionTraversalCallback &cb) >+{ >+ if (sEventListenerManagersHash.ops) { >+ EventListenerManagerMapEntry *entry = >+ static_cast<EventListenerManagerMapEntry *> >+ (PL_DHashTableOperate(&sEventListenerManagersHash, aNode, >+ PL_DHASH_LOOKUP)); >+ if (PL_DHASH_ENTRY_IS_BUSY(entry)) { >+ NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "[via hash] mListenerManager"); >+ cb.NoteXPCOMChild(entry->mListenerManager); >+ } >+ } >+ >+ if (sRemoteEventSourceManagersHash.ops) { >+ RemoteEventSourceManagerMapEntry *entry = >+ static_cast<RemoteEventSourceManagerMapEntry *> >+ (PL_DHashTableOperate(&sRemoteEventSourceManagersHash, aNode, >+ PL_DHASH_LOOKUP)); >+ if (PL_DHASH_ENTRY_IS_BUSY(entry)) { >+ NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "[via hash] mRemoteEventSourceManager"); >+ cb.NoteXPCOMChild(entry->mRemoteEventSourceManager); >+ } > } > } I think it would be better to have separate Traverse for ELM and RESM. ELMs are far more common than RESMs so it doesn't make sense to check sRemoteEventSourceManagersHash every time there is an ELM. >+ nsresult rv = NS_NewRemoteEventSourceManager(getter_AddRefs(mRemoteEventSourceManager)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = mRemoteEventSourceManager->Init(NodePrincipal(), static_cast<nsIDocument*>(this)); You could simplify this a bit and remove SetRemoteEventsTarget if Init() would take RemoteEventsTarget as a parameter. Same also elsewhere where RESM is created. > nsresult >+nsGenericElement::GetRemoteEventSourceManager(PRBool aCreateIfNotFound, >+ nsIRemoteEventSourceManager** aResult) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIRemoteEventSourceManager> manager; >+ >+ rv = nsContentUtils::GetRemoteEventSourceManager(this, aCreateIfNotFound, getter_AddRefs(manager)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ manager.swap(*aResult); >+ >+ return NS_OK; >+} Why not just return nsContentUtils::GetRemoteEventSourceManager(this, aCreateIfNotFound, aResult); >+NS_IMETHODIMP >+nsDOMEventRTTearoff::RemoveEventSource(const nsAString& aSrc) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIRemoteEventSourceManager> manager; >+ >+ rv = mContent->GetRemoteEventSourceManager(PR_FALSE, getter_AddRefs(manager)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (!manager) >+ return NS_OK; >+ >+ nsCOMPtr<nsIDOMHTMLEventSourceElement> eventsourceHtmlTag(do_QueryInterface(mContent, &rv)); >+ if (!eventsourceHtmlTag || NS_FAILED(rv)) { >+ rv = manager->RemoveEventSource(aSrc); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else { >+ nsAutoString htmlSrc; >+ >+ rv = eventsourceHtmlTag->GetSrc(htmlSrc); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (htmlSrc.IsEmpty()) { >+ rv = manager->RemoveEventSource(aSrc); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else { >+ //we need resolve the absolute uri of aSrc and htmlSrc >+ //and if equals to, we should clear the src attribute >+ >+ nsCOMPtr<nsIURI> baseURI = mContent->GetBaseURI(); >+ >+ nsCOMPtr<nsIURI> aSrcURI; >+ rv = NS_NewURI(getter_AddRefs(aSrcURI), aSrc, nsnull, baseURI); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCAutoString specURISrc; >+ rv = aSrcURI->GetSpec(specURISrc); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIURI> htmlSrcURI; >+ rv = NS_NewURI(getter_AddRefs(htmlSrcURI), htmlSrc, nsnull, baseURI); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCAutoString specURIHtmlSrc; >+ rv = htmlSrcURI->GetSpec(specURIHtmlSrc); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (!specURIHtmlSrc.Equals(specURISrc)) { >+ rv = manager->RemoveEventSource(aSrc); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else { >+ rv = eventsourceHtmlTag->SetSrc(EmptyString()); //this will call manager->RemoveEventSource(aSrc) >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ } >+ } >+ >+ return rv; >+} I don't think this is doing the right thing. If the element isn't <eventsource>, aSrc should still be resolved using baseuri. And also, remember that same uri may have been registered multiple times. And, does the spec say that if there is element <eventsource src="http://foo.com">, and someone calls eventsourceelement.removeEventSource("http://foo.com"), the attribute should be cleared from the element? I don't think so. >diff --git a/content/events/public/nsIRemoteEventSourceManager.h b/content/events/public/nsIRemoteEventSourceManager.h Note to myself - not reviewed. >+++ b/content/events/src/nsRemoteEventSourceManager.cpp Not reviewed yet. >diff --git a/content/events/src/nsRemoteEventSourceManager.h b/content/events/src/nsRemoteEventSourceManager.h Not reviewed yet >+// >+//This one will cause the mochitest 338583/Test 3 (#3) and Test 3 (#7) to fail. >+//If using this, those tests would get eventsource_tag.src == {document url} >+//instead of the empty string... >+// >+//NS_IMPL_URI_ATTR(nsHTMLEventSourceElement, Src, src) >+// >+ >+NS_IMPL_STRING_ATTR(nsHTMLEventSourceElement, Src, src) Yes, currently HTML5 says the .src is the content attribute value, not the resolved absolute URI. >+ >+PRBool >+nsHTMLEventSourceElement::ParseAttribute(PRInt32 aNamespaceID, >+ nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult) >+{ >+ if (aNamespaceID == kNameSpaceID_None) { >+ if (aAttribute == nsGkAtoms::src) { >+ nsresult rv; >+ static const char* kWhitespace = " \n\r\t\b"; >+ nsAutoString srcBefore, srcNow; >+ >+ rv = GetSrc(srcBefore); >+ NS_ENSURE_SUCCESS(rv, PR_FALSE); >+ >+ srcNow = nsContentUtils::TrimCharsInSet(kWhitespace, aValue); >+ >+ nsCOMPtr<nsIDOMRemoteEventTarget> remEvtTarget; >+ >+ remEvtTarget = do_QueryInterface(static_cast<nsGenericHTMLElement*>(this), &rv); >+ NS_ENSURE_SUCCESS(rv, PR_FALSE); >+ >+ if (!srcBefore.IsEmpty()) { >+ // remEvtTarget->RemoveEventSource(srcBefore); -> this would let us in a infinite loop, so... >+ >+ nsCOMPtr<nsIRemoteEventSourceManager> manager; >+ rv = GetRemoteEventSourceManager(PR_FALSE, getter_AddRefs(manager)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (manager) { >+ rv = manager->RemoveEventSource(srcBefore); >+ NS_ENSURE_SUCCESS(rv, PR_FALSE); >+ } >+ } >+ >+ if (!srcNow.IsEmpty()) { >+ rv = remEvtTarget->AddEventSource(srcNow); >+ NS_ENSURE_SUCCESS(rv, PR_FALSE); >+ } >+ >+ aResult.SetTo(srcNow); >+ >+ return PR_TRUE; >+ } >+ } >+ >+ return nsGenericHTMLElement::ParseAttribute(aNamespaceID, aAttribute, aValue, >+ aResult); >+} You need to handle also the case when src attribute is removed from the element. And the value of src attribute shouldn't change because of URI parsing or anything. (Just got an idea for RESM lifetime handling - would it make sense if it was ELM which owns RESM. That way all the objects which have ELM could easily have RESM. The RESM objects would all be in a hashtable, where ELM objects would be keys. Does this make sense to you?)

>>I don't think this is doing the right thing. >>If the element isn't <eventsource>, aSrc should still be resolved using baseuri. I think it isn't necessary because manager->RemoveEventSource already does it (using its mPrincipal). >>And also, remember that same uri may have been registered multiple times. When an eventsource is registered multiple times removeEventSource will only remove the first one, following the spec: "If the same URI has been registered multiple times, removing it must remove only one instance of that URI for each invocation of the removeEventSource() method." >>And, does the spec say that if there is element <eventsource src="http://foo.com">, and someone calls eventsourceelement.removeEventSource("http://foo.com"), the attribute should be cleared from the element? I don't think so. Hmmm, I think you are right. Then, I will change the removeEventSource behaviour. When removing an eventsource equals to the src attr, it won't remove the last one(when there will be only one remaining eventsource). Is it ok?

>>(Just got an idea for RESM lifetime handling - would it make sense if it was >> ELM which owns RESM. The RESM lifetime handling is following the ELM. In my tests I've verified that the destructors are being called correctly. Like you have written here, because ELMs are far more common than RESMs most of the RESMs owned by the ELMs will be null. So I think it's better using the nsContentUtils.ccp/sRemoteEventSourceManagersHash hashtable, as it is. >>That way all the objects which have ELM could easily have >> RESM. The RESM objects would all be in a hashtable, where ELM objects >> would be keys. Does this make sense to you?) However, if you are saying to use a static hashtable in nsEventListenerManager.cpp (like in nsContentUtils.ccp) without RESM refs in the ELM instances this makes sense. So, if you want I change this. >>>>diff --git b/content/events/public/nsIRemoteEventSourceManager.h >>Note to myself - not reviewed. >>>>+++ b/content/events/src/nsRemoteEventSourceManager.cpp >>Not reviewed yet. >>>>diff --git b/content/events/src/nsRemoteEventSourceManager.h >>Not reviewed yet I've started doing this changes. However I will wait you finish this revisions in order to upload the new patch. Wellington.

(In reply to comment #45) > > However, if you are saying to use a static hashtable in > nsEventListenerManager.cpp (like in nsContentUtils.ccp) without RESM refs in > the ELM instances this makes sense. So, if you want I change this. The hashtable can be anywhere, and I didn't mean the ELM has any refs to RESM. Just that ELM could be the key in the hashtable, where RESM is the value.

Though, maybe it is better to handle RESMs similarly to ELMs. Easier to understand and all...

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) Have you tested this all with bfcache? What happens when page goes to cache? What happens when page is again activated? > // QueryInterface implementation for nsDOMAttribute > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsDOMAttribute) > NS_INTERFACE_MAP_ENTRY(nsIDOMAttr) > NS_INTERFACE_MAP_ENTRY(nsIAttribute) > NS_INTERFACE_MAP_ENTRY(nsINode) nsDOMAttribute should support remote event target interface. Add also a test for that. > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsGenericDOMDataNode) > NS_INTERFACE_MAP_ENTRY(nsIContent) > NS_INTERFACE_MAP_ENTRY(nsINode) > NS_INTERFACE_MAP_ENTRY(nsPIDOMEventTarget) nsGenericDOMDataNode should support remote event target interface. Add also a test for that. > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsGenericElement) > NS_INTERFACE_MAP_ENTRY(nsIContent) > NS_INTERFACE_MAP_ENTRY(nsINode) > NS_INTERFACE_MAP_ENTRY(nsPIDOMEventTarget) > NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOM3Node, new nsNode3Tearoff(this)) > NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMNSElement, new nsNSElementTearoff(this)) > NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMEventTarget, >+ nsDOMEventRTTearoff::Create(this)) >+ NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMRemoteEventTarget, > nsDOMEventRTTearoff::Create(this)) > NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOM3EventTarget, > nsDOMEventRTTearoff::Create(this)) > NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMNSEventTarget, > nsDOMEventRTTearoff::Create(this)) > NS_INTERFACE_MAP_ENTRY_TEAROFF(nsISupportsWeakReference, > new nsNodeSupportsWeakRefTearoff(this)) > NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMNodeSelector, nsIDOMRemoteEventTarget is least frequently used. Add it to the end of list. >@@ -1179,16 +1182,20 @@ nsDOMEvent::GetEventPopupControlState(ns > break; > } > } > break; > case NS_XUL_COMMAND_EVENT : > if (nsEventStateManager::IsHandlingUserInput()) { > abuse = openControlled; > } >+ break; >+ case NS_MESSAGE_EVENT : >+ abuse = openControlled; >+ break; Why this? Scripts shouldn't be able to use message event to open windows.

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) >+nsresult >+nsXHREventTarget::GetRemoteEventSourceManager(PRBool aCreateIfNotFound, >+ nsIRemoteEventSourceManager** aResult) >+{ >+ return NS_ERROR_NOT_IMPLEMENTED; > } All event targets should support remote event targets

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) >+#define RESM_DEFAULT_BUFFER_SIZE 8192 >+#define RESM_MAX_RECONNECTION_TIME_VALUE \ >+ PR_IntervalToMilliseconds(PR_BIT(8 * sizeof(PRIntervalTime) - 1)) I don't understand this. Why 8? Why sizeof(PRIntervalTime)? >+//nsIRequestObserver will be the unambiguous nsISupports base for nsEventSource >+ >+#define RESM_EVENTSOURCE_CAST_FROM_ISUPPORTS(_expr) \ >+ (static_cast<nsEventSource*>(static_cast<nsIRequestObserver*>(_expr))) >+ if (!innerWindow) { >+ if (!gFrozenEventSources->AppendElement(oThis)) { >+ NS_WARNING("Couldn't append oThis in gFrozenEventSources"); >+ return; >+ } >+ oThis->Freeze(); >+ } else if (innerWindow->IsFrozen()) { >+ if (!gFrozenEventSources->AppendElement(oThis)) { >+ NS_WARNING("Couldn't append oThis in gFrozenEventSources"); >+ return; >+ } >+ oThis->Freeze(); ... >+nsresult >+nsEventSource::TryThaw() Ah, there is some code for bfcache. >+ while (true /* == !streamIsFinished*/ ) { >+ PRUnichar buffer[RESM_DEFAULT_BUFFER_SIZE]; This uses stack pretty heavily. Some systems don't have large stack. (Symbian used to have 8KB)

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) >+//we should test: >+// 1) the addEventSource for window, document, eventsource tag and some generic html tag(document.body is enough) >+// 2) the removeEventSource for window, document, eventsource tag and some generic html tag(document.body is enough) >+// 3) the eventsource's src attribute and its refletion with the removeEventSource method >+// 4) regular dom events >+// 5) invalid eventsources Maybe there could be a test where server sends lots of different kinds of events, and somewhere in middle of the test the remote event target is removed. >+//in order to test (5) >+// a) re-add the eventsource tag in the document >+// b) dns error test >+// c) protocol file:/// test >+// d) protocol javascript: test >+// e) wrong Content-Type test >+// f) bad http response code test >+// g) message eventsource without a data test >+// h) eventsource with invalid NCName char in the event field test Perhaps more tests with wrong data - something which is almost right, but might break RESM event stream parser. >+NS_IMPL_ADDREF_INHERITED(nsHTMLEventSourceElement, nsGenericElement) >+NS_IMPL_RELEASE_INHERITED(nsHTMLEventSourceElement, nsGenericElement) inherited from nsGenericHTMLElement. > if (nodeType == eHTMLTag_img || nodeType == eHTMLTag_frame >- || nodeType == eHTMLTag_input) // elements with 'SRC=' >+ || nodeType == eHTMLTag_input || nodeType == eHTMLTag_eventsource) // elements with 'SRC=' > AddBaseTagInfo(content); Too long line. >@@ -1096,17 +1096,18 @@ nsHTMLParanoidFragmentSink::AddAttribute > } > > if (nodeType == eHTMLTag_a || > nodeType == eHTMLTag_form || > nodeType == eHTMLTag_img || > nodeType == eHTMLTag_map || > nodeType == eHTMLTag_q || > nodeType == eHTMLTag_blockquote || >- nodeType == eHTMLTag_input) { >+ nodeType == eHTMLTag_input || >+ nodeType == eHTMLTag_eventsource) { > AddBaseTagInfo(aContent); > } Should we actually disable src attribute on eventsource when using nsHTMLParanoidFragmentSink. Feed handler seems to use paranoidsink. Should also SanitizingSerializer be modified. >diff --git a/dom/public/nsDOMClassInfoID.h b/dom/public/nsDOMClassInfoID.h >--- a/dom/public/nsDOMClassInfoID.h >+++ b/dom/public/nsDOMClassInfoID.h >@@ -102,16 +102,17 @@ enum nsDOMClassInfoID { > eDOMClassInfo_HTMLBaseFontElement_id, > eDOMClassInfo_HTMLBodyElement_id, > eDOMClassInfo_HTMLButtonElement_id, > eDOMClassInfo_HTMLDListElement_id, > eDOMClassInfo_HTMLDelElement_id, > eDOMClassInfo_HTMLDirectoryElement_id, > eDOMClassInfo_HTMLDivElement_id, > eDOMClassInfo_HTMLEmbedElement_id, >+ eDOMClassInfo_HTMLEventSourceElement_id, > eDOMClassInfo_HTMLFieldSetElement_id, > eDOMClassInfo_HTMLFontElement_id, > eDOMClassInfo_HTMLFormElement_id, > eDOMClassInfo_HTMLFrameElement_id, > eDOMClassInfo_HTMLFrameSetElement_id, > eDOMClassInfo_HTMLHRElement_id, > eDOMClassInfo_HTMLHeadElement_id, > eDOMClassInfo_HTMLHeadingElement_id, You should add to the end of list. Same thing in nsDOMClassInfo.cpp

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) >+ JSContext* mJSContext; Do you really need this and is it ensured that this never points to a deleted object? Would it be possible to get it from mOwner->mTarget (which should always implement nsPIDOMEventTarget, I think)?

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) >+ rv = nsContentUtils::GetSecurityManager()-> >+ CheckLoadURIWithPrincipal(mOwner->mPrincipal, >+ mSrc, >+ aCheckURIFlags); Should you perhaps have NS_CheckContentLoadPolicy. See what script loading or object loading do.

Comment on attachment 337736 [details] [diff] [review] final patch (review - 1st phase) >+ nsCOMPtr<nsIPrincipal> documentPrincipal = nsnull; documentPrincipal? Call it just principal. >+//the nsEventSource::FillDOMXXXEventDefaults methods use the DOM2-Events spec >+//when there isn't any information there: >+// PRBool fields -> PR_FALSE >+// aCancelable -> PR_TRUE >+// PRUint32, PRUint16 and PRInt32 -> 0 >+// Strings -> "" >+// nsIDOMAbstractView** aView -> the owner's innerWindow >+// nsIXXX** -> nsnull Is this all defined properly in HTML 5? >+ *aBubbles = PR_FALSE; >+ *aCancelable = PR_TRUE; I think *aBubbles = PR_TRUE; would be better default. >+ case 'c': >+ { >+ char third_char; >+ if (mEventName.Length() < 3) >+ break; >+ third_char = (char)mEventName.CharAt(2); Why this third_char thing is needed? Similar also elsewhere. The first char optimization is probably enough. Other way to implement this would be to use hashtable eventname->initializationfunction and initialize such hashtable lazyly. >+ nsCOMPtr<nsIDOMMessageEvent> messageEvent; >+ nsCOMPtr<nsIDOMKeyEvent> keyboardEvent; >+ nsCOMPtr<nsIDOMPageTransitionEvent> pageTransitionEvent; >+ nsCOMPtr<nsIDOMXULCommandEvent> xulCommandEvent; >+ nsCOMPtr<nsIDOMMouseEvent> mouseEvent; >+ nsCOMPtr<nsIDOMMutationEvent> mutationEvent; >+ nsCOMPtr<nsIDOMUIEvent> uiEvent; Define these whenever you use them >+ mDOMEventBoolFields.Get(NS_LITERAL_STRING("bubbles"), &bubbles); >+ mDOMEventBoolFields.Get(NS_LITERAL_STRING("cancelable"), &cancelable); You could use NS_NAMED_LITERAL_STRING to get fields from has table. >+ //then, we dispatch >+ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ rv = nsEventDispatcher::DispatchDOMEvent(evtRealTarget, nsnull, domEvent, >+ nsnull, >+ &status); Could you make sure (in tests) that event is actually untrusted when dispatched. (It should be, but better to have a test for that too.) >+ >+ if (!mOwner) >+ return nsCString(); >+ >+ if (!mOwner->mDocument || !mOwner->mPrincipal) >+ return nsCString(); You could combine those checks: NS_ENSURE_TRUE(mOwner && mOwner->mDocument && mOwner->mPrincipal, nsCString()); >+ nsCOMArray<nsISupports> mEventSources; Why not nsTArray<nsRefPtr<nsEventSource> > ? Then you shouldn't need to use that a bit ugly macro Need to still go through parsing...

> Should you perhaps have NS_CheckContentLoadPolicy. Wait. We're only calling this if !mXSiteEnabled? That's wrong. If you're trying to enforce same-origin you probably want to be calling CheckMayLoad(), and if you don't call CheckMayLoad() you MUST call CheckLoadURI. No options about it. And yes, this could use a NS_CheckContentLoadPolicy. Is there a reason we're forcing all channels involved to be HTTP? It should be clearly documented, and made more explicit than just checking the rv from QI (esp. since said rv might be going away with outparamdel). I'm not sure why permanent redirects are handled differently from other kinds in this code. Why do we care? What's with the whitelist of response codes in OnStartRequest? Do we really want to do something here with an explicit 401 response? That doesn't make much sense to me... You should never really see the other response codes you're checking from here. Do you just want nsIHttpChannel.requestSucceeded? While I'm here, EqualsLiteral, please, not Equals(NS_LITERAL_CSTRING(...)). Why are we treating DNS errors differently from other errors in OnStopRequest? Is there a reason for manual if cascades instead of switches in ParseCharacter? You shouldn't be handing out inner window references to content script. If this code does that, please change it to use the outer window.

>+ nsCOMPtr<nsIPrincipal> documentPrincipal = nsnull; Drop the "= nsnull" part, please.

>> The hashtable can be anywhere, and I didn't mean the ELM has any refs to RESM. >> Just that ELM could be the key in the hashtable, where RESM is the value. -- (In reply to comment #47) > Though, maybe it is better to handle RESMs similarly to ELMs. Easier to > understand and all... Well... Then I will maintain it as it is (similarly to ELMs), ok?

(In reply to comment #57) > Well... Then I will maintain it as it is (similarly to ELMs), ok? Ok

(In reply to comment #48) > (From update of attachment 337736 [details] [diff] [review]) > Have you tested this all with bfcache? What happens when page goes to cache? > What happens when page is again activated? Yes, I have. You can see what happen here (from 2:36 to 2:50): http://br.youtube.com/watch?v=iGdPJYtMgSM > nsDOMAttribute should support remote event target interface. > Add also a test for that. > > nsGenericDOMDataNode should support remote event target interface. > Add also a test for that. Ok. However the nsDOMAttribute's nsIDOMEventTarget support isn't working. Comments and Text nodes(nsGenericDOMDataNode) are working. I've tested: var event = document.createEvent("Events"); var attr = document.body.getAttributeNode('bgcolor'); event.initEvent("anEvent", true, true); ---> //atr.addEventListener exists, //but it will throw an exception when called attr.addEventListener('anEvent', function() { alert('ok'); }, true); attr.dispatchEvent(event); So, it won't be possible do attr.addEventListener('message') when testing.

(In reply to comment #59) > Ok. However the nsDOMAttribute's nsIDOMEventTarget support isn't working. Ah, ok, then you can't add the test. I'll fix DOMAttr's event handling.

(In reply to comment #50) > >+#define RESM_MAX_RECONNECTION_TIME_VALUE \ > >+ PR_IntervalToMilliseconds(PR_BIT(8 * sizeof(PRIntervalTime) - 1)) > I don't understand this. Why 8? Why sizeof(PRIntervalTime)? > I've copied this "PR_BIT(8 * sizeof(PRIntervalTime) - 1)" from nsGlobalWindow.cpp and added the PR_IntervalToMilliseconds call, which nsGlobalWindow does, too. I think it is the numeric limit.

You should probably copy the comment referencing DELAY_INTERVAL_LIMIT too, then. Better yet, that stuff should move to nsITimer.idl in a {%C++ block and just be referenced from these various places. That can happen as a followup if the comments happen now.

(In reply to comment #51) > Perhaps more tests with wrong data - something which is almost right, but > might break RESM event stream parser. Following the spec I see just one possibility that could break the RESM event stream parser. This one is when the event field contains an invalid NCName. When there are invalid UTF-8 sequences they are replaced. I don't see another case. Have you thought some else test case? > Should we actually disable src attribute on eventsource when using > nsHTMLParanoidFragmentSink. Feed handler seems to use paranoidsink. > Should also SanitizingSerializer be modified. > Sorry, I don't know if I have understood. Do you want that the eventsource tag and its src attribute can be used when using paranoidsink, ins't it?

(In reply to comment #54) > >+//the nsEventSource::FillDOMXXXEventDefaults methods use the DOM2-Events spec > >+//when there isn't any information there: > >+// PRBool fields -> PR_FALSE > >+// aCancelable -> PR_TRUE > >+// PRUint32, PRUint16 and PRInt32 -> 0 > >+// Strings -> "" > >+// nsIDOMAbstractView** aView -> the owner's innerWindow > >+// nsIXXX** -> nsnull > Is this all defined properly in HTML 5? No, it isn't. In order to implement default field values I have used all information that the DOM2-Events spec has provided about it. However it wasn't enough, because there is, in that spec, only default values for cancelable, bubbles and other few fields. So, I've used these listed values. > >+ *aBubbles = PR_FALSE; > >+ *aCancelable = PR_TRUE; > I think *aBubbles = PR_TRUE; would be better default. ok. > >+ if (!mOwner) > >+ return nsCString(); > >+ > >+ if (!mOwner->mDocument || !mOwner->mPrincipal) > >+ return nsCString(); > You could combine those checks: > NS_ENSURE_TRUE(mOwner && mOwner->mDocument && mOwner->mPrincipal, nsCString()); ok. I've done in this way because someone in somewhere (or some book) has said me (I don't know if he was right or not...) that testing a pointer and dereferencing it inside a same expression could be dangerous.

(In reply to comment #64) > ok. I've done in this way because someone in somewhere (or some book) has said > me (I don't know if he was right or not...) that testing a pointer and > dereferencing it inside a same expression could be dangerous. That was misinformation or a misunderstanding -- in C and C++ (and other languages based on C), the short-circuiting logical connectives && and || have a guaranteed left to right order of evaluation. In other expressions, order of evaluation is not specified, so it is not safe to use a possibly null pointer without a prior test (if condition, && or ||, ?:, etc.). /be

(In reply to comment #61) > (In reply to comment #50) > > >+#define RESM_MAX_RECONNECTION_TIME_VALUE \ > > >+ PR_IntervalToMilliseconds(PR_BIT(8 * sizeof(PRIntervalTime) - 1)) > > I don't understand this. Why 8? Why sizeof(PRIntervalTime)? > > > > I've copied this "PR_BIT(8 * sizeof(PRIntervalTime) - 1)" from > nsGlobalWindow.cpp and added the PR_IntervalToMilliseconds call, which > nsGlobalWindow does, too. I think it is the numeric limit. Is it the right limit? Do you use a macro like this one from xpcom/threads/nsTimerImpl.h? // Is interval-time t less than u, even if t has wrapped PRIntervalTime? #define TIMER_LESS_THAN(t, u) ((t) - (u) > DELAY_INTERVAL_LIMIT) For more on why this works, see bug 138791 comment 38 et seq. /be

> >+ return nsCString(); Er... why not EmptyCString() here? For that matter, why isn't this function just using an out param for the string, in the usual way?

(In reply to comment #55) > Is there a reason we're forcing all channels involved to be HTTP? It should be > clearly documented, and made more explicit than just checking the rv from QI > (esp. since said rv might be going away with outparamdel). It is because the spec has defined event streams only to http. This implementation uses the http headers "Last-Event-ID", "Cache-Control" and "Accept". Also, it uses the http response codes. > What's with the whitelist of response codes in OnStartRequest? Do we really > want to do something here with an explicit 401 response? That doesn't make > much sense to me... You should never really see the other response codes > you're checking from here. Do you just want nsIHttpChannel.requestSucceeded? ok. > Why are we treating DNS errors differently from other errors in OnStopRequest? Because the spec says "DNS errors must be considered fatal, and cause the user agent to not open any connection", so we can't just abort the request. > Is there a reason for manual if cascades instead of switches in ParseCharacter? No, it is only aesthetic. If you want I can use switches inside switches. > You shouldn't be handing out inner window references to content script. If > this code does that, please change it to use the outer window. No, this code has only references to nsIDocument ans nsIPrincipal. It gets the Window as it needs from the nsIDocument reference.

> Is it the right limit? Do you use a macro like this one from > xpcom/threads/nsTimerImpl.h? > > // Is interval-time t less than u, even if t has wrapped PRIntervalTime? > #define TIMER_LESS_THAN(t, u) ((t) - (u) > DELAY_INTERVAL_LIMIT) > No, I don't use. I use only the "<" operator, as the nsGlobalWindow does.

(In reply to comment #69) > > Is it the right limit? Do you use a macro like this one from > > xpcom/threads/nsTimerImpl.h? > > > > // Is interval-time t less than u, even if t has wrapped PRIntervalTime? > > #define TIMER_LESS_THAN(t, u) ((t) - (u) > DELAY_INTERVAL_LIMIT) > > > > No, I don't use. I use only the "<" operator, as the nsGlobalWindow does. Then you're not handling wrap-around, possibly. If nsGlobalWindow has a bug here it should be filed separately. /be

(In reply to comment #70) > Then you're not handling wrap-around, possibly. If nsGlobalWindow has a bug > here it should be filed separately. > > /be ok, I will use a macro.

> > Do you just want nsIHttpChannel.requestSucceeded? > ok. I'm not sure what that means. What does the spec say here? What's a reasonable behavior? > Because the spec says "DNS errors must be considered fatal, and cause the user > agent to not open any connection", so we can't just abort the request. So just to make sure I understand, if I hit a DNS error once, we will never try to hit that server again until the page reloads? > No, it is only aesthetic. If you want I can use switches inside switches. It it's more readable, please do. I'm pretty sure compilers can do a better job on it. >> You shouldn't be handing out inner window references to content script. If >> this code does that, please change it to use the outer window. >No, this code has only references to nsIDocument ans nsIPrincipal. It gets the >Window as it needs from the nsIDocument reference. I thought you said you set the inner window as the nsIDOMAbstractView on the event, though...

Please schedule a security review of this feature (we do for all new features) here: https://wiki.mozilla.org/Firefox3.1/Security#Pending_Reviews

(In reply to comment #72) > > > Do you just want nsIHttpChannel.requestSucceeded? > > ok. > > I'm not sure what that means. What does the spec say here? What's a > reasonable behavior? Actually, all response codes should be checked because the spec defines a set of http response codes that can be received. In case of another response code, the eventsource should be prevented from fetching(see below). Then, considering this, should I replace with nsIHttpChannel.requestSucceeded? > So just to make sure I understand, if I hit a DNS error once, we will never try > to hit that server again until the page reloads? The code prevents the eventsource's URI, not its server (domain). When it happens all similar eventsources are removed and destroyed and that URI is blocked permanently, until the application (browser) is finished. > > > No, it is only aesthetic. If you want I can use switches inside switches. > > It it's more readable, please do. I'm pretty sure compilers can do a better > job on it. ok, so I will maintain the ifs inside the switches (I think it is more readable).

> the spec defines a set of http response codes that can be received. I assume you mean http://www.whatwg.org/specs/web-apps/current-work/#connecting-to-an-event-stream ? Going through the things it lists: 200: no problems 201, 202, 203, 206: you will likely never see this on your end, but in any case, this should succeed per spec. 204, 205: These will automatically have no data, so you should be fine, no? Worth double-checking with a broken server that sends a body with a 204 or 205. 3xx: You will only see these in your code if there is no Location: header or if there is some other error redirecting. So if you get a 3xx here it needs to be treated like 404. 301 special-casing comment: Why are you copying specs instead of URI objects in ProcessRedirection? Don't do that. 401, 407: necko handles this already; you won't see these So what that leaves are 2xx codes for xx > 06. The spec requires these to be treated as errors and "cause the user agent to stop trying to process this event source" (whatever that means; please raise the issue of defining that with the spec author). I think this is a mistake in the spec; there is no reason to prohibit all future 2xx codes. Assuming this mistake is fixed, GetRequestSucceeded() will give you the right answer. If it's not, then you'd have to check for just the "allowed" 2xx codes. > When it happens all similar eventsources are removed and destroyed and that > URI is blocked permanently, until the application (browser) is finished. So on a transient error we enter a state where the browser has to be restarted for things to work right again? That makes no sense, and if the spec requires it the spec needs changing. In general, these specs are works in progress and are pretty much guaranteed to have bugs; part of implementing them (and of reviewing the patches) should be looking hard at them and deciding whether what they call for even makes sense, not just blindly implementing it.

(In reply to comment #73) > Please schedule a security review of this feature (we do for all new features) > here: > > https://wiki.mozilla.org/Firefox3.1/Security#Pending_Reviews it is done.

>> Assuming this mistake is fixed GetRequestSucceeded() will give you the right answer. If it's not, then you'd > have to check for just the "allowed" 2xx codes. > ok, then I will check both. In the second case I will block the eventsource (see below), ok? > So on a transient error we enter a state where the browser has to be restarted > for things to work right again? That makes no sense, and if the spec requires > it the spec needs changing. Hmm, then what do I do? Should I don't care about it (fetching the eventsources whenever) or should I block until the page reloads?

> ok, then I will check both. As I said that's something to do _only_ if the editor won't change the spec here. I think the spec as written is silly. > Hmm, then what do I do? Get the spec to define this behavior clearly, taking into account the issues I raised? In the meantime, do nothing special for DNS errors?

Hi, Here it is the patch updated. Using the spec editor answers, that he has written to me and Boris, I've decided to use nsIHttpChannel.requestSucceeded. Also, I've moved the nsTimerImpl numeric limits stuff to nsITimer.idl and I am using its comparator macro. I've used this changeset as base: changeset: 20590:f31b66fd4192 user: Marco Bonardo <mak77@bonardo.net> date: Fri Oct 17 06:12:53 2008 -0400 summary: Bug 459934 - should lazy-load places autocomplete statements I'm awaiting the dveditz answer in order to schedule the security review. I think this patch is good for the 3rd review phase, if still needed. Wellington.

I've found out there are some new events in the current changeset. This patch is up-to-date and has minor changes. Wellington.

I updated the spec. http://html5.org/tools/web-apps-tracker?from=2358&to=2359

The new spec text doesn't say that you stop processing on network errors, right?

I am not sure, because, in the spec: "If such a resource (with the correct MIME type) completes loading (i.e. the entire HTTP response body is received or the connection itself closes), the user agent should request the event source resource again after a delay equal to the reconnection time of the event source. This doesn't apply for the error cases that are listed below." and, after, below: "Any other HTTP response code not listed here or network error (e.g. DNS errors) must be ignored." It isn't clear if either the error should be ignored (and so don't stop processing the resource, retrying after the reconnection time) or if the resource should be ignored completely. As it is now, the patch has been modified in order to do what Ian had answered by email: "It just means "do nothing" (including not automatically retrying). If the event source is reregistered, then it should go through the same process again; the error response doesn't block that URL for all time." Wellington.

> It isn't clear if either the error should be ignored Please raise that with the working group in question so that the spec is clarified....

(In reply to comment #84) > > It isn't clear if either the error should be ignored > > Please raise that with the working group in question so that the spec is > clarified.... ok, I've written to him...

Comment on attachment 343812 [details] [diff] [review] final patch (review - 2nd phase) > nsContentUtils::TraverseListenerManager(nsINode *aNode, > nsCycleCollectionTraversalCallback &cb) > { >- if (!sEventListenerManagersHash.ops) { >- // We're already shut down, just return. >- return; >- } >- >- EventListenerManagerMapEntry *entry = >- static_cast<EventListenerManagerMapEntry *> >- (PL_DHashTableOperate(&sEventListenerManagersHash, aNode, >- PL_DHASH_LOOKUP)); >- if (PL_DHASH_ENTRY_IS_BUSY(entry)) { >- NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "[via hash] mListenerManager"); >- cb.NoteXPCOMChild(entry->mListenerManager); >+ if (sEventListenerManagersHash.ops) { >+ EventListenerManagerMapEntry *entry = >+ static_cast<EventListenerManagerMapEntry *> >+ (PL_DHashTableOperate(&sEventListenerManagersHash, aNode, >+ PL_DHASH_LOOKUP)); >+ if (PL_DHASH_ENTRY_IS_BUSY(entry)) { >+ NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "[via hash] mListenerManager"); >+ cb.NoteXPCOMChild(entry->mListenerManager); >+ } >+ } >+} Why this change? > void > nsContentUtils::RemoveListenerManager(nsINode *aNode) > { > if (sEventListenerManagersHash.ops) { > EventListenerManagerMapEntry *entry = > static_cast<EventListenerManagerMapEntry *> > (PL_DHashTableOperate(&sEventListenerManagersHash, aNode, > PL_DHASH_LOOKUP)); > if (PL_DHASH_ENTRY_IS_BUSY(entry)) { > nsCOMPtr<nsIEventListenerManager> listenerManager; >+ > listenerManager.swap(entry->mListenerManager); >+ > // Remove the entry and *then* do operations that could cause further > // modification of sEventListenerManagersHash. See bug 334177. > PL_DHashTableRawRemove(&sEventListenerManagersHash, entry); > if (listenerManager) { > listenerManager->Disconnect(); Don't make unnecessary whitespace changes. > void > nsGenericDOMDataNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent) > { >+ nsContentUtils::RemoveRemoteEventSourceManager(this); >+ this->UnsetFlags(NODE_HAS_REMOTEEVENTSOURCEMANAGER); >+ Does HTML 5 really specify this behavior? > nsGenericElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) > { > NS_PRECONDITION(aDeep || (!GetCurrentDoc() && !GetBindingParent()), > "Shallow unbind won't clear document and binding parent on " > "kids!"); >+ >+ nsContentUtils::RemoveRemoteEventSourceManager(this); >+ this->UnsetFlags(NODE_HAS_REMOTEEVENTSOURCEMANAGER); Same here. >-#define NS_EVENT_TEAROFF_CACHE_SIZE 4 >+#define NS_EVENT_TEAROFF_CACHE_SIZE 5 Why this change? > class nsXMLHttpRequestUpload : public nsXHREventTarget, > public nsIXMLHttpRequestUpload >@@ -229,16 +237,17 @@ public: > mOwner = aOwner; > mScriptContext = aScriptContext; > } > virtual ~nsXMLHttpRequestUpload(); > NS_DECL_ISUPPORTS_INHERITED > NS_FORWARD_NSIXMLHTTPREQUESTEVENTTARGET(nsXHREventTarget::) > NS_FORWARD_NSIDOMEVENTTARGET(nsXHREventTarget::) > NS_FORWARD_NSIDOMNSEVENTTARGET(nsXHREventTarget::) >+ NS_FORWARD_NSIDOMREMOTEEVENTTARGET(nsXHREventTarget::) Why is this needed for upload but not for XMLHttpRequest? >+nsHTMLEventSourceElement::ParseAttribute(PRInt32 aNamespaceID, >+ nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult) Why ParseAttribute and not BeforeSetAttr Will continue... sorry for the delay

> Why this change? > > Don't make unnecessary whitespace changes. > Sorry... I will fix it. > > void > > nsGenericDOMDataNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent) > > { > >+ nsContentUtils::RemoveRemoteEventSourceManager(this); > >+ this->UnsetFlags(NODE_HAS_REMOTEEVENTSOURCEMANAGER); > >+ > Does HTML 5 really specify this behavior? > No, it doesn't. Because you wrote "You need to handle also the case when src attribute is removed from the element." I thought it should make sense for genericElement and genericDOMDataNode (like for attributes). If you want I take away it. > >-#define NS_EVENT_TEAROFF_CACHE_SIZE 4 > >+#define NS_EVENT_TEAROFF_CACHE_SIZE 5 > Why this change? > Because I've added the remote event target interface to nsDOMEventRTTearoff I thought that could be good another cache entry in that class. If you want I take away it too. > > class nsXMLHttpRequestUpload : public nsXHREventTarget, > > public nsIXMLHttpRequestUpload > >+ NS_FORWARD_NSIDOMREMOTEEVENTTARGET(nsXHREventTarget::) > Why is this needed for upload but not for XMLHttpRequest? > I'm following the nsIDOMEventTarget model. In this case XMLHttpRequest gets its nsIDOMRemoteEventTarget implementation from nsXHREventTarget. > >+nsHTMLEventSourceElement::ParseAttribute(PRInt32 aNamespaceID, > >+ nsIAtom* aAttribute, > >+ const nsAString& aValue, > >+ nsAttrValue& aResult) > Why ParseAttribute and not BeforeSetAttr > ok.

(In reply to comment #73) > Please schedule a security review of this feature (we do for all new features) > here: > > https://wiki.mozilla.org/Firefox3.1/Security#Pending_Reviews hi, dveditz hasn't anwered my email about the security review. May I schedule this for next week? Wellington.

So I started actually looking at the HTML5 spec here. Why exactly are we using a principal URI as the base URI for event source URI resolution? Why are we doing string compares instead of nsIURI::Equals() compares in RemoveEventSource? When creating nsEventSource objects, is there a reason you're not storing the pointer in an nsRefPtr? It looks like you're calling methods on an object with a refcount of 0, which is a bad idea. I'm a little confused by the direct includes of nsScriptSecurityManager.h in content code. How does that work, exactly?

(In reply to comment #89) > Why exactly are we using a principal URI as the base URI for event source URI resolution? Why are we > doing string compares instead of nsIURI::Equals() compares in > RemoveEventSource? Hmmm, I've found out that I could get the principal from nsRemoteEventSourceManager::mDocument. I will use nsIDocument::GetBaseURI for event source URI resolution. Also I will use nsIURI::Equals(), I hadn't seen it before, sorry. > When creating nsEventSource objects, is there a reason you're not storing the > pointer in an nsRefPtr? It looks like you're calling methods on an object with > a refcount of 0, which is a bad idea. The nsEventSource objects are stored inside a nsTArray<nsRefPtr<nsEventSource> >. However you are right, when they are created I only use a pointer. I will fix it. > I'm a little confused by the direct includes of nsScriptSecurityManager.h in > content code. How does that work, exactly? I've included nsScriptSecurityManager.h in order to use nsScriptSecurityManager::ReportError. If you prefer I can use JS_ReportError, instead. Wellington.

only correcting the look of my answer: > When creating nsEventSource objects, is there a reason you're not storing the > pointer in an nsRefPtr? It looks like you're calling methods on an object with > a refcount of 0, which is a bad idea. The nsEventSource objects are stored inside a nsTArray<nsRefPtr<nsEventSource>>. However you are right, when they are created I only use a pointer. I will fix it.

(In reply to comment #90) > I've included nsScriptSecurityManager.h in order to use > nsScriptSecurityManager::ReportError. If you prefer I can use JS_ReportError, > instead. Do you actually need that? Could returning NS_ERROR_DOM_SECURITY_ERR be enough? Or if you want to report something to console, nsContentUtils::ReportToConsole? nsScriptSecurityManager::ReportError sets an exception to the context. How does it work currently if several InitChannel fails here (if that is possible)? + PRUint32 count, i; + count = mEventSources.Length(); + for (i = 0; i < count; ++i) { + mEventSources[i]->ResetChannel(); + mEventSources[i]->InitChannel(); + }

> I will use nsIDocument::GetBaseURI for event source URI resolution. That sounds great. Note that it should be possible to write a test for this that will catch the fact that we were using the wrong base URI. > I've included nsScriptSecurityManager.h in order to use > nsScriptSecurityManager::ReportError. I understand that. I'm just surprised that it compiles and runs in all configurations (including on Windows, without libxul, etc), and not convinced that this is guaranteed to actually be the case. That said, comment 92 is correct: setting these errors on the JSContext is a little weird. What's the desired actual behavior here? Should this error reporting cause scripts to throw exceptions?

(In reply to comment #92) > nsScriptSecurityManager::ReportError sets an exception to the context. > How does it work currently if several InitChannel fails here (if that is > possible)? This doesn't happen right now because in the current implementation everywhere nsRemoteEventSourceManager::Init is used it is called only once by instance, after its instance is created (so there isn't any eventsources yet). However I haven't thought about it. There is no prohibition that this method can be used later. In this case I don't know what could happen... Because the spec doesn't say to throw exception I think it is better just report something to console and return NS_ERROR_DOM_SECURITY_ERR like you have said. Do you agree?

Comment on attachment 343812 [details] [diff] [review] final patch (review - 2nd phase) >+nsEventSource::CreateDOMEvent(nsIDOMEvent** aDOMEvent, nsIAtom** aAtom) Seems like the spec has changed here. Step 3 is "create an event that uses the MessageEvent interface". So that is the event interface to use always, just the type of the event may change depending on the "event name" buffer.

(In reply to comment #96) > (From update of attachment 343812 [details] [diff] [review]) > >+nsEventSource::CreateDOMEvent(nsIDOMEvent** aDOMEvent, nsIAtom** aAtom) > Seems like the spec has changed here. Step 3 is > "create an event that uses the MessageEvent interface". > So that is the event interface to use always, just the type of the > event may change depending on the "event name" buffer. I've just seen the spec and you are right. If changing this it will remove support for all other dom events, like Mouse, Keyboard, etc and its specific fields. Wellington. ps: I am in irc(#shiretoko) just now, using the nick wellington.

Hi, It's here the new patch with the review comments. Only I haven't implemented the Access-Control support, because I need to look at the XMLHttpRequest better and then implement it in resm. While I implement that you can review this patch. When I finish the Access-Control support I upload it here again. Wellington.

This patch has Access-Control support. Wellington.

So how come the GetPrincipal function here is so COM-y? It's not like it can really fail or return null, so it should probably just be a function that takes no arguments and returns an nsIPrincipal* (and should be called Principal()).

(In reply to comment #100) > So how come the GetPrincipal function here is so COM-y? It's not like it can > really fail or return null, so it should probably just be a function that takes > no arguments and returns an nsIPrincipal* (and should be called Principal()). ok.

it is just the last one, however merged with the trunk.

Comment on attachment 247037 [details] [diff] [review] Patch as of 11-30-06 Clearing review on age-old patch.

The following tests fail (at least on 64 bit linux) with the latest patch. browser-chrome: browser_gestureSupport.js mochitest: /tests/docshell/test/navigation/test_bug386782.html /tests/editor/composer/test/test_bug384147.html Maybe just some small merge problem, like missing "," in nsDOMEvent.cpp

But I'll fix that when reviewing, testing.

(In reply to comment #104) > Maybe just some small merge problem, like missing "," in nsDOMEvent.cpp Adding the missing ',' fixed browser_gestureSupport.js, but not docshell nor editor test.

This patch and the missing ',' is needed to make tests working.

One thing to improve is the freeze/thaw handling. Might be better to get notification from the window when that happens. Then you wouldn't need to check if window is frozen in many places and RefreshGlobalAndStaticData() wouldn't have to start the timer.

This updated path has the last comments. I've added an "stress factor" in the mochitest for slow machines. Also, I've improved the freeze/thaw behaviour. Wellington.

I found an issue in channel redirects, which started to occur when I put the access-control suport. This patch has this correction and one or two small changes. Sorry because uploading again.. Wellington.

Comment on attachment 359247 [details] [diff] [review] final-patch-2009-01-27 (4th review) >+NS_IMETHODIMP >+nsDOMAttribute::RemoveEventSource(const nsAString& aSrc) >+{ >+ nsCOMPtr<nsIRemoteEventSourceManager> manager; >+ nsresult rv = GetRemoteEventSourceManager(PR_TRUE, getter_AddRefs(manager)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ return manager->RemoveEventSource(aSrc); Couldn't you call GetRemoteEventSourceManager(PR_FALSE, getter_AddRefs(manager)) and then RemoveEventSource only if manager != nsnull >+nsRemoteEventSourceManager::GetLoadGroup(nsILoadGroup **aLoadGroup) >+{ >+ NS_ENSURE_STATE(mDocument); >+ NS_ENSURE_ARG_POINTER(aLoadGroup); >+ *aLoadGroup = mDocument->GetDocumentLoadGroup().get(); // already_AddRefed >+ >+ return NS_OK; >+} I think this method could return already_AddRefed<nsILoadGroup>. (No need for the out param) >+nsRemoteEventSourceManager::GetPrincipal(nsIPrincipal **aPrincipal) >+{ >+ NS_ENSURE_STATE(mDocument); >+ NS_ENSURE_ARG_POINTER(aPrincipal); >+ >+ NS_ADDREF(*aPrincipal = mDocument->NodePrincipal()); >+ >+ return NS_OK; >+} This method could return already_AddRefed<nsIPrincipal>. >+nsRemoteEventSourceManager::GetBaseURI(nsIURI **aBaseURI) >+{ >+ NS_ENSURE_STATE(mDocument); >+ NS_ENSURE_ARG_POINTER(aBaseURI); >+ >+ nsresult rv; >+ >+ *aBaseURI = nsnull; >+ >+ //first we try from nsIContent::GetBaseURI() of mTarget >+ nsCOMPtr<nsIContent> contentTarget; >+ if (mTarget) >+ contentTarget = do_QueryInterface(mTarget); >+ if (contentTarget) >+ *aBaseURI = contentTarget->GetBaseURI().get(); //already_AddRefed >+ if (*aBaseURI) >+ return NS_OK; >+ >+ //next we try from mDocument->GetBaseURI() >+ *aBaseURI = mDocument->GetBaseURI(); >+ if (*aBaseURI) { >+ NS_ADDREF(*aBaseURI); >+ return NS_OK; >+ } >+ >+ //otherwise we try from the doc's principal >+ nsCOMPtr<nsIPrincipal> principal; >+ rv = GetPrincipal(getter_AddRefs(principal)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = principal->GetURI(aBaseURI); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (*aBaseURI) { >+ NS_ADDREF(*aBaseURI); >+ return NS_OK; >+ } This leaks. principal->GetURI addrefs, and then you re-addref. >+nsHTMLEventSourceElement::~nsHTMLEventSourceElement() >+{ >+} No need for this destructor > DOM_CLASSINFO_MAP_BEGIN(Element, nsIDOMElement) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMElement) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSElement) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) >+ DOM_CLASSINFO_MAP_ENTRY(nsIDOMRemoteEventTarget) > DOM_CLASSINFO_MAP_ENTRY(nsIDOM3Node) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMNodeSelector) > DOM_CLASSINFO_MAP_END > > DOM_CLASSINFO_MAP_BEGIN(Attr, nsIDOMAttr) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMAttr) > DOM_CLASSINFO_MAP_ENTRY(nsIDOM3Node) > DOM_CLASSINFO_MAP_ENTRY(nsIDOM3Attr) > DOM_CLASSINFO_MAP_END I think Attr should also have DOM_CLASSINFO_MAP_ENTRY(nsIDOMRemoteEventTarget) I think either jst or sicking could superreview.

This patch is merged with the trunk and has the last comments. > I think Attr should also have > DOM_CLASSINFO_MAP_ENTRY(nsIDOMRemoteEventTarget) > I've added. I hadn't added before because nsIDOMEventTarget wasn't working in Attr. (I've tested, and it isn't working yet). Wellington.

I started a thread on the whatwg list about reducing the IMHO way too great complexity of this feature. I don't at all understand why we need an <eventsource> element, or why RemoteEventTarget needs to be implemented by all EventTargets. Sorry for springing this on you so late, but I hadn't looked at this feature in the spec before and it was just brought to my attention the other day how complex this feature is (as demonstrated by the patch in this bug. 258Kb is a lot) If you have opinions, such as you think i'm crazy as a loon, please head over to the whatwg list and comment there.

I may agree that for web development it might be easier to have just <eventsource> which implements RemoteEventTarget. Though, it is pretty easy to make all the event targets to implement RemoteEventTarget too. For testing purposes I've been thinking about extending this all so that we can record event streams and play back them from server. That may or may not need that each event target implements RemoteEventTarget.

(In reply to comment #115) > I may agree that for web development it might be easier to have just > <eventsource> which implements RemoteEventTarget. Actually, my proposal is to not have <eventsource> at all, but rather a dedicated object that listens to server streams. I don't understand the purpose of having this be an element at all. See full proposal at http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-February/018578.html Ideally comments should go to there too, though i'll forward any concerns expressed in this bug. > Though, it is pretty > easy to make all the event targets to implement RemoteEventTarget too. That's a bad reason to add a feature. > For testing purposes I've been thinking about extending this all so > that we can record event streams and play back them from server. > That may or may not need that each event target implements RemoteEventTarget. I really don't think we should expose features to web content just to make internal testing easier for us. We can always add separate hacks for that. Most likely RemoteEventTarget will never be a full solution here anyway since we wouldn't want that to be able to dispatch trusted 'click' events for example.

(In reply to comment #116) > I really don't think we should expose features to web content just to make > internal testing easier for us. We can always add separate hacks for that. Oh, right. I didn't mean that we should expose that event stream thing to web. > Most > likely RemoteEventTarget will never be a full solution here anyway since we > wouldn't want that to be able to dispatch trusted 'click' events for example An early version of the patch did have a partial support for that.

Hi, The spec has changed. This patch has the modifications. Because EventSource has become similar to XHR I've tried to let the common stuff in a common-base class. Well.. who should review it? Wellington.

Why is the nsEventTargetWrapperCache stuff still needed? Is there anything else that can be cut away now that the spec is much simpler? I don't think we should architect for remote events being applied to more EventTargets in the future. If that happens we can always change the code then. Not sure if you are doing this or not, just wanted to ask.

(In reply to comment #119) > Why is the nsEventTargetWrapperCache stuff still needed? This class is new. It has the common stuff that I told. EventSource and XHR inherits from this. I think you were telling about the old nsRemoteEventTarget or nsRemoteEventSourceManager. > Is there anything else that can be cut away now that the spec is much simpler? > I don't think we should architect for remote events being applied to more > EventTargets in the future. Yes, all HTML and remote events for events target has been removed. This patch is much simpler than the previous one. Wellington.

I am admittedly a little surprised that there is any advantages in code sharing between XHR and the server-sent events code. However I haven't looked at the code in detail so it might definitely make sense. Wouldn't mind doing the sr here though.

(In reply to comment #121) > I am admittedly a little surprised that there is any advantages in code sharing > between XHR and the server-sent events code. I would expect that having a base class for XHR, WebSockets and server-sents might be pretty useful.

Comment on attachment 365701 [details] [diff] [review] stable patch (new spec) See the comments. I hope I didn't miss any changes comparing to the earlier patches.

Spec moved. New url is http://dev.w3.org/html5/eventsource/

Here is the patch with the comments. Wellington.

hmm, a small change I missed when I uploaded. Wellington.

I hate to be the one to throw a stick in the wheel here, but the more I think about this less clear it is to me that we actually want to see this code land in the Firefox source tree, especially already. The big reasons for doing this initially was to benefit from this on mobile devices, being able to do server push w/o the need to keep a TCP connection open for as long as the user keeps a webpage open etc. It's been almost 3 years since this effort was started and I've seen 0 progress on that front wrt this spec. And I hear from our mobile team that there's not very strong interest in them investing in doing the backend work for non-TCP connections for this purpose. I also have yet to hear anyone else sign up for that for Mozilla eihter. So I'm questioning whether taking this is in the best interest for Mozilla. I realize that this a cleaner way for doing server push to clients (compared to iframe hacks, XHR, XHR with multipar-mixed-replace), but I'm not convinced that's a good enough argument for taking this given the amount of code, the need for someone to actively maintain this feature going forward, the instability of the spec (afaik at least), and the increased attack surface we'll get from adding this feature to Firefox. I also realize that Wellington (and Clayton early on) has spent a fair bit of time working on this, and it's my fault for not paying attention to this bug for a long time and not bringing this up sooner. What do others think about taking this feature into the tree?

disclaimer: I'm not a developer/contributor to Mozilla, just a consumer of its technologies. re 128: iframe hacks are a pain because the developer is required to periodically reload the page (requiring a bit code in the browser and quite a bit more code in most servers to deal with this) because otherwise the iframe "original source" consumes too much memory. The XHR multipart-mixed-replace stuff would be nice if it actually still worked. It stopped working in Firefox 3 iirc. If that behavior was returned, standardized, and pushed into Opera (and a miracle happens and IE supported it too) then server-sent dom events would indeed lose much (though not all) of their appeal. If WebSockets (I detest the current spec, but that's a different issue) were supported by Firefox it would also near completely eliminate the need for server-sent dom events, as those allow pretty much the same thing except they allow the client to talk back. Which would make a whole class of web apps that are currently impossible to implement a reality, e.g. realtime games that can't deal with the overhead of full HTTP requests for every single update.

Multipart XHR worked just fine last I checked. If you have a testcase that shows otherwise, please cc me on the bug.

(In reply to comment #129) > The XHR multipart-mixed-replace stuff would be nice if it actually still > worked. It stopped working in Firefox 3 iirc. If that behavior was returned, > standardized, and pushed into Opera (and a miracle happens and IE supported it > too) then server-sent dom events would indeed lose much (though not all) of > their appeal. Unless we have a reason to think that IE would implement server-sent DOM events, but not multipart-mixed-replaced, I don't think that is an argument for implementing server-sent events. > If WebSockets (I detest the current spec, but that's a different issue) were > supported by Firefox it would also near completely eliminate the need for > server-sent dom events, as those allow pretty much the same thing except they > allow the client to talk back. Which would make a whole class of web apps > that are currently impossible to implement a reality, e.g. realtime games > that can't deal with the overhead of full HTTP requests for every single > update. I really think we should implement WebSockets, for the reasons that you state above. So if that solves the same use cases then I don't think we need server-sent DOM events. The main reasons i've seen for server-sent DOM events is to allow for connection-less push, over for example SMS or iPhone-push. However until that is possible I don't know of much value that server-sent DOM events provide over XHR and WebSockets.

Is the server-sent DOM events code at least helpful for implementing WebSockets? It at least solves the issue of handling persistent connections from a page and handling events. Mostly just a change to the connection handler (to use the silly WebSockets handshake instead of a regular HTTP request) and then changing the event handler to always fire a specific event, yes? I wonder also if the mobile folks are uninterested in server-sent DOM events solely because nobody except Opera actually implements it yet?

We (the mobile folks) are interested in server-set DOM events for connectionless push. Is anyone interested in implementing a connectionless backend?

(In reply to comment #132) > Is the server-sent DOM events code at least helpful for implementing > WebSockets? There is already a bug with a partial WebSockets patch. I've already implemented about 80% of the WebSockets spec. Although incomplete, it is already working. See https://bugzilla.mozilla.org/show_bug.cgi?id=472529

(In reply to comment #133) > We (the mobile folks) are interested in server-set DOM events for > connectionless push. Is anyone interested in implementing a connectionless > backend? Kaazing might be interested in implementing a connectionless backend. Feel free to contact me directly if you would like to discuss this in more detail.

(In reply to comment #131) [snip] > I really think we should implement WebSockets, for the reasons that you state > above. So if that solves the same use cases then I don't think we need > server-sent DOM events. [snip] Server-sent Events and WebSockets cover different usecases. Server-sent Events defines reliability and recoverability in the protocol specification, beyond what would make sense to do for the generic socket abstraction provided by WebSockets. Server-sent Events is also well-positioned to share event streams with the same URL across different frames, different tabs, even different user-interface windows for the same browser process. Consider the scenario where the main page for a site has a Server-sent Events stream, and includes links to navigate to other pages on the site with the same stream. If the user chooses to open those links in a different tab or window, Server-sent Events could be capable of sharing the existing event stream without incurring an additional HTTP connection. Feedback on stream sharing has already been sent to the public comments mailing list. See http://lists.w3.org/Archives/Public/public-html-comments/2009Mar/0010.html for more details. Sharing the same WebSocket connection across frames, tabs or windows just doesn't make sense, because the semantics of the shared upstream would be so confusing. IMHO, Server-sent Events and WebSockets provide solutions to two different but important usecases over HTTP, in much the same way that UDP and TCP provide solutions to two different but important usecases over IP.

(In reply to comment #136) > Server-sent Events and WebSockets cover different usecases. I agree. WebSocket is lower level and XHR and server sent events could be almost implemented using it. XHR is for web page to request something from server. Server-Sent events (and multipart XHR) is for server to push data to web page. WebSocket is to send and receive pretty raw data. > Server-sent Events is also well-positioned to share event streams with the same > URL across different frames, different tabs, even different user-interface > windows for the same browser process. But in practice streams aren't shared per spec. (In reply to comment #128) > I realize that this a cleaner way for doing server push to clients Indeed. And that is the main reason I'd like to see this or something similar in browsers. (In reply to comment #129) > The XHR multipart-mixed-replace stuff would be nice if it actually still > worked. It stopped working in Firefox 3 iirc. It works fine here on 3.0 and 3.x http://mozilla.pettay.fi/post_multipart_test.html

My impression is this: Websockets + XHR + XHR-with-multipart + JS-libraries cover the use cases for server sent DOM events fairly well, with the exception of connection-less push such as over SMS or iPhone-push. Yes, there are things that get more complicated, but I'm not convinced that reducing that complexity is worth the cost of adding this API at this time.

Vlad suggested an alternative approach: We could encourage extensions to that implement an SMS backend. One way to this would be to create a stub implementation that just implements the API, but no backend (including no http backend). We'd also create hooks so that someone can implement a backend for SMS or other push technologies. Such a stub implementation should be very small, and wouldn't commit us to any support in case no SMS implementations show up. The other way to do nothing. I believe that we already have all the hooks needed to enable extensions to expose APIs to javascript. So anyone could today create an extension that implements the EventSource API with an SMS backend.

(In reply to comment #138) > Websockets + XHR + XHR-with-multipart + JS-libraries cover the use cases for > server sent DOM events fairly well, with the exception of connection-less push > such as over SMS or iPhone-push. multipart XHR does indeed cover the use cases of server sent events quite well, but is there a specification for it, or is it implemented by anyone else than gecko.

I was asked to chime in on this issue by Mark Finkle. FWIW, I think the combination of multipart XHR (keep an existing HTTP connection open and send new chunks of data) and Web Sockets (keep long-running connections open with an explicit protocol and methodology) removes the need for Server-Sent DOM Events.

My concern is similar to Smaug's. If Opera is using Server-Sent Events, and Firefox is using Multipart XHR, with no agreed upon standard Internet Explorer may go more releases than it would otherwise without choosing one path or the other or just decide it's own ActiveX("htmlfile") transport is good enough. Maybe John has a JQuery abstraction layer that works well with the ActiveX("htmlfile") transport. Personally I have never been able to get that particular "Comet" hack to work on my server. So my concern as a developer is that if Mozilla doesn't implement Server-Sent Events it may be many more years than necessary before the average developer can add "Comet" functionality to their web apps. Obviously this is all conjecture. It just seems more likely that we will have cross browser functionality in the foreseeable future if the quickly developing browsers, Mozilla, Webkit, and Opera are on the same page. This would give IE a more compelling reason to participate.

Given that there is no uniform solution (even excluding IE, who were quick to implement querySelector and postMessage, so I hold out hope), why do you advocate a totally new API like server-sent DOM events rather than having Opera and WebKit add multipart to the XHR they already support? That would also seem to be something more likely to be adopted by IE, since again they already have an XHR implementation and are actively participating in the XHR2 work AFAIK.

If Opera and Webkit were to implement multipart XHR then my particular concern about a cross-browser standard would be resolved. This doesn't negate any of the other concerns voiced above or elsewhere though. From what I can tell, it doesn't look like any of the Comet libraries use multipart XHR by default for Mozilla due to reasons mentioned in the first two links below and instead use XHR Streaming even though the latter has it's own issues. Someone can correct me if I am wrong. Dojo: http://alex.dojotoolkit.org/2006/12/adventures-in-comet-and-multipart-mime/ Orbited: http://cometdaily.com/2007/12/11/the-future-of-comet-part-1-comet-today/ Meteor: http://meteorserver.org/browser-techniques/ There is a feature request for multipart XHR at Webkit Bugzilla, but interest in it does not appear very high as of yet. https://bugs.webkit.org/show_bug.cgi?id=14392 Opera of course has already implemented a version of Server-Sent Events. From a developer's perspective, one of the nice things about it is just how simple and straight-forward it is to use. http://labs.opera.com/news/2006/09/01/

I don't think Opera is not too interested in adding multipart support to XMLHttpRequest. I'm pretty sure we will update our Server-Sent Events implementation if there is another implementation. (E.g. the one proposed here.) As for IE interest in XHR2, I haven't really seen that. They are interested in CORS, but on the client-side they have a proprietary API called XDomainRequest since IE8.

I can see where this could be nice. "Simple" is the main order. I think this would have been taken up years ago if TCP/IP standardized on permanent address assignment. With a 64-bit permanent address, there would be potential for pushing an always-on mailing list. But that's in a perfect world. The issue on mobiles is always that teeny CPU and memory, and the load from flash and javascript. That's why this looks nice, but I doubt we'll see anything final on it from Mozilla, not ever. Of course if OBML supported more flexible partial updates and Server-sent had a large push in the Widgets API, Fennec would look foolish without a working implementation.

Comment on attachment 368411 [details] [diff] [review] stable patch reviewed Clearing review request here since we won't be taking this change, but instead take Wellington's awesome WebSockets contribution in bug 472529.

For the record, I'm not sure if we'll want this or not. We should continue to poke around more and ask web developers if and why they want it. So far I haven't heard many requests, but I guess that might change.

During my last meeting with web developers, we talked a lot about "push" mechanisms, and they know about server sent events. "It's easy to use, pretty straight forward." That's the huge asset of this mechanism. Mutlipart XHR sounds like a "side effect" of XHR, not something they want to bet in. Websockets sounds awesome, but a bit overkill. Of course, this feature could be implemented through a JS library using websockets or XHR, and it's what it's going to happen if don't ship it. Is it what we want? (honest question)

I don't want server sent events to be implemented via websockets, since websocket protocol doesn't use http (well, it does use something like http during handshake). Implementing server-sent events using multipart xhr might be better. But IMO, we should just support server sent events. Jst, Jonas, comments?

I'm not sure that the purpose of multipart XHR is to push data "à la" server sent events. I feel like using XHR to implement it looks like the "infinite iframe" hack (well, it's not that ugly). I think we can make web developer life easier with a native implementation of the server sent events. But it's not a priority since it can be easily simulated with a simple JS library.

The url in the description has gone stale. I'm basing my comments off of this: http://dev.w3.org/html5/eventsource/ The draft spec describes the behavior of server sent events in detail when the transport protocol is http. For other protocols it simply says "For non-HTTP protocols, UAs should act in equivalent ways." I can certainly see value in supporting this for web developers in general. And to that extent, supporting any transport is a good thing. In mobile contexts, supporting transports other than http will be very valuable. If, for instance, we had support for sip events, we wouldn't have to keep the socket open and could save a lot of battery by powering down the radio. So, I think we should support SSE, even if its only with http support at first so that we creating a void that third party libraries will naturally fill.

It's a bit unclear, at least to me, what the status of the this bug is. Do we want to add support for this at all, and in that case, do we want to do it for 4.0?

Anyone still working on this ? I would have loved to see this in FF4, but it seems that we'll have to wait a bit longer?

Nicholas Zakas: "Server-Sent events are a simpler alternative to Web Sockets for server push technology. Although some people seem convinced that bidirectional communication is an absolute necessity, many of the proposed use cases for Web Sockets are actually implementable using Server-Sent Events. This includes streaming stock quotes, score updates for games, bidding updates for auctions, etc. Opera 10.7 has an implementation of Server-Sent Events, and an implementation has already made it into the WebKit repository as well. Before jumping on the Web Sockets bandwagon, consider if Server-Sent Events can accomplish the same task." http://www.nczonline.net/blog/2010/10/19/introduction-to-server-sent-events/

Server-Sent Events currently work in Safari, Mobile Safari, Chrome and Opera. But sadly, not in Firefox. There is a test page at: http://sapid.sourceforge.net/ssetest/ That page has a link to an SSE emulation script that appears to work in Firefox 3.6.11, but not in FF4b6.

Thanks for the test page! Going ahead anyway now with developing a multi-player game, using SSE and canvas. Never thought FF was going to be one of the last browsers to be able to run the game. Ah well, I'm sure they'll get around it soon. ;-)

Thanks for the test page! Going ahead anyway now with developing a multi-player game, using SSE and canvas. Never thought FF was going to be one of the last browsers to be able to run the game. Ah well, I'm sure they'll get around it soon. ;-)

Are we strict with enforcing content-type in the current patch? And are we also sending an appropriate accept headers? If so I think we should go ahead and take this as that gives us forwards compatibility if the spec is expanded to support SMS in the future.

I managed to find the appropriate places in the patch, and it does indeed look like we're doing the right thing for both Accept and Content-Type. So I say lets take this! The bad news is that it's probably too late to take this for FF4. I'll have to ask around :(

If you need to update the patch to the trunk, please let me know.

Wellington: Just to clarify, the current patch is reviewed and ready to go, right? Just needs merging to tip. So unless something unforseen is discovered during the merge, the patch is ready to land?

Yes, it is reviewed and ready to go. There are two minor changes in the spec: EventSource::URL -> EventSource::url EventSource::disconnect() -> EventSource::close()

As we now have a few more days before feature freeze, can we make sure that this is assigned to someone and gets in before Beta 7?

Is there a comment in this bug somewhere that gives an overview of the pros and cons of taking this sort of change, as well as a summary of how well tested the code is for regressions (functional and performance?) I'm happy to give driving input here, but this is the first I've heard of Server-Sent DOM events, let alone the proposal that we include it as a technology supported by Firefox 4. I'm generally not a fan of haphazard late-takes like this, as they've had a strong history of causing downstream headaches for us in terms of support for privacy, security and performance issues. My instinct is to not take it, but I'd like to know what I'm giving up and why people feel this is important enough to justify the late-in-the-game risk.

I talked to Blizzard and Jonas about this a couple of days ago, but failed to comment in this bug afterwards. IMO we'd be shooting from the hip if we took this feature this late in the beta cycle, especially given the slip we've already had. I think this is a feature we should land early on in a dev cycle (like, on the trunk right after we branch for Firefox 4), and leave it bake on trunk throughout the whole cycle. So IOW, we agreed that we should not take this right now, but we should land it after we branch...

Thanks, jst. Appreciate it.

I have heard that "Fennec" is now also "Firefox"? If/when this matter is finally addressed, do we assume that mobiles will also be getting it? (As SSE takes highest precedent over WebSockets on mobile devices)

calc - no, SSE will not be available on mobile in this cycle. i share jst's risk assessment.

Thanks for clarifying, and I do entirely sympathize with your concern about late-testing changes. Though I wonder what the threat profile is for a one-way "broadcast" communication method.

Note <http://html5.org/tools/web-apps-tracker?from=5832&to=5833>, btw.

I created http://tc.labs.opera.com/apis/EventSource/ by the way which you are free to use. request-2xx.htm is however not yet updated with respect to the aforementioned change. Everything else should be okay.

Oh, and you can get the raw files via http://tc.labs.opera.com/svn/apis/EventSource/

(In reply to comment #161) > If you need to update the patch to the trunk, please let me know. Wellington, I'd like to get this landed for Firefox 5. Can you update the patch to trunk?

Yes, of course.

We should get some security folks involved ASAP too, if we plan to have it threat-modelled and ready for distribution to users in a few weeks. (I don't know if there's existing TM work on server-sent events that we can reuse, but I didn't see any references in this bug at least.)

I don't know that Firefox 5 is the right release target here, the cutoff for that is in ~3 weeks. But I agree we should get this patch updated and ready to ship soon. If I was in control here I'd probably say this is closer to Firefox 6 material (since 5 is mostly a learning exercise for the new release process), that would give Wellington some time to work on this and get it ready, and then we can land it early after the Firefox 5 pull from mozilla-central, and have some time to make sure this is ready and shippable.

Wellington, I just wanted to point out a recently added tree rule: Code should be factored in such a way such that we can disable features which cause regressions, either by backout or via a kill switch/preference. Be especially careful when landing features which depend on other new features which may be disabled. Ask mozilla.dev.planning for assistance if there are any questions. While we could always back this out, you might want to think about adding a way to disable it without back out as well.

> Be especially careful when landing features which depend on other new features > which may be disabled. Ask mozilla.dev.planning for assistance if there are any > questions. Ok. Isn't there any page or document that lists the features which may be disabled? > While we could always back this out, you might want to think about adding a way > to disable it without back out as well. Not sure, but I think that the current patch has already a preference that disables/enables this code. I'll check it.

(In reply to comment #179) > > Be especially careful when landing features which depend on other new features > > which may be disabled. Ask mozilla.dev.planning for assistance if there are any > > questions. > Ok. Isn't there any page or document that lists the features which may be > disabled? My understanding is that is a warning against relying on recently landed changes as they may be disabled as we stablize the branch in this new release model. Given that this patch was written against code checked into the tree at least 2 years ago, that doesn't really apply here.

Actually, I would say this is a decent feature to use to experiment with landing a patch which we would use flags for to enable/disable as it's pretty isolated. Wellington: Do you think you could add a pref which would disable the new feature. I.e. so that there is no way to instantiate it using either XPCOM nor the DOM. Disabling the pref also needs to disable any traces of it from the DOM. So all interface objects need to be hidden, and the constructor needs to be hidden. The pref definitely does *not* need to disable the code shuffling for nsXHREventTarget obviously as that shouldn't change any behavior. If you need help with this find me or jst on irc (or email if you prefer). We can land with this preffed off, and then we'll figure out it if makes sense to flip on for FF5 or not. But lets make that a separate discussion from the landing discussion. As for security review, my understanding is that this doesn't really expose any new capabilities and so from a security point of view is in general pretty low risk. However that is a discussion we should take separately when we discuss flipping the pref on.

(In reply to comment #181) > As for security review, my understanding is that this doesn't really expose any > new capabilities and so from a security point of view is in general pretty low > risk. However that is a discussion we should take separately when we discuss > flipping the pref on. Then it could be a quick review, but one of the points of a security review is to make sure we take time trying to think of ways that it could be broken (redirects? strange protocols? does it trigger mixed content appropriately? etc.) rather than deciding that it's low risk a priori and therefore not checking.

For enabling/disabling EventSource we could have similar to WebSocket's pref.

Given the proposed value of SSE for updating pages on mobile devices, protocol dependence could be an issue I guess. Maybe there should be a simple check on the incoming data? Since once the connection is open, there should be no further lengthy handshakes; this could be an issue if the stream passes through a compromised router?

Changes in the spec: * there is no cross-site support any more; * URL -> url, and absolute * disconnect() -> close()

Comment on attachment 523815 [details] [diff] [review] patch Some comments, but I need to do still a proper review >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPrincipal) No need for this. >+ >+ mPrincipal = nsnull; I'd never change mPrincipal once it is initialized to some value. >+ >+ nsCOMPtr<nsIJSContextStack> stack = >+ do_GetService("@mozilla.org/js/xpc/ContextStack;1"); >+ JSContext* cx = nsnull; >+ if (stack && NS_SUCCEEDED(stack->Peek(&cx)) && cx) { >+ JSStackFrame *fp = JS_GetScriptedCaller(cx, NULL); >+ if (fp) { >+ JSScript *script = JS_GetFrameScript(cx, fp); >+ if (script) { >+ mScriptFile = JS_GetScriptFilename(cx, script); >+ } >+ >+ jsbytecode *pc = JS_GetFramePC(cx, fp); >+ if (script && pc) { >+ mScriptLine = JS_PCToLineNumber(cx, script, pc); >+ } >+ } >+ >+ mWindowID = nsJSUtils::GetCurrentlyRunningCodeWindowID(cx); Nothing seems to actually use mScriptFile, mSriptLine and mWindowID >+ >+ rv = os->AddObserver(this, DOM_WINDOW_FREEZED_TOPIC, PR_FALSE); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = os->AddObserver(this, DOM_WINDOW_THAWED_TOPIC, PR_FALSE); >+ NS_ENSURE_SUCCESS(rv, rv); So does the patch not leak? I'd guess EventSource should support weakreference. >+ >+ mReconnectionTime = >+ nsContentUtils::GetIntPref("dom.server-events.default-reconnection-time", >+ MIN_RECONNECTION_TIME_VALUE*10); Why the *10? Couldn't MIN_RECONNECTION_TIME_VALUE be 10* the value in the current patch. And document what the value means. Is it ms, or s, or usec >+ if (mReadyState != nsIEventSource::CONNECTING || !PrefEnabled() || >+ aArgc != 1) { I'm not sure whether != 1 is right. perhaps > 0 would be better. Check what other browsers do. >+ >+ nsCOMPtr<nsIConverterInputStream> converter( >+ do_CreateInstance("@mozilla.org/intl/converter-input-stream;1", &rv)); 2 space indentation >+ if (aIID.Equals(NS_GET_IID(nsIAuthPrompt)) || >+ aIID.Equals(NS_GET_IID(nsIAuthPrompt2))) { >+ nsCOMPtr<nsIPromptFactory> wwatch = >+ do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // Get the an auth prompter for our window so that the parenting >+ // of the dialogs works as it should when using tabs. >+ >+ nsCOMPtr<nsIDOMWindow> window; >+ if (mOwner) { >+ window = mOwner->GetOuterWindow(); >+ } I think there should be CheckInnerWindowCorrectness() somewhere here. >+nsEventSource::PrefEnabled() >+{ >+ return nsContentUtils::GetBoolPref("dom.server-events.enabled", PR_TRUE) && >+ nsContentUtils::GetBoolPref("dom.server-events.override-security-block", >+ PR_FALSE); I think we should have just one pref "dom.server-sent_events.enabled" For the event names, I don't know why IsXMLNCNameChar check is needed. Some old version of DOM Events did require that, but not http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html Are there any tests for cases like da<flush server output and wait>ta: foobar Feel free to update the patch or wait another review.

This is done with a long lived GET, right? Assuming that is true, this should coordinate with pipelining if it lands after pipelining so that we can mark the connection as a head of line blocker and not try and pipeline behind it.

What is the bug # for pipelining?

(In reply to comment #188) > What is the bug # for pipelining? bug 603503

I was reading the pipelining code. Is nsIRequest::LOAD_POTENTIALLY_SLOW applicable to this patch?

(In reply to comment #190) > I was reading the pipelining code. Is nsIRequest::LOAD_POTENTIALLY_SLOW > applicable to this patch? yes, that is what comment 187 refers to. But it would be a dep and we don't want to make pipelining a hard dep for this feature. Thanks for reading the pipelining code!

>> + if (mReadyState != nsIEventSource::CONNECTING || !PrefEnabled() || >> + aArgc != 1) { > I'm not sure whether != 1 is right. perhaps > 0 would be better. > Check what other browsers do. WebKit does > 0. > Why the *10? Couldn't MIN_RECONNECTION_TIME_VALUE be 10* the value in the > current patch. > And document what the value means. Is it ms, or s, or usec It is ms. Although I think that 500 ms is suitable for a minimum possible reconnection time, I think it is too small for the default one. What do you think about? > yes, that is what comment 187 refers to. But it would be a dep and we don't > want to make pipelining a hard dep for this feature. Thanks for reading the > pipelining code! So I won't use that flag.

> So does the patch not leak? > I'd guess EventSource should support weakreference. No, it isn't leaking, because the observer service is holding a strong reference to the nsEventSource object. Also when the nsEventSource object is closed it removes itself from the service. However I'll change to support weakreference.

> I think there should be CheckInnerWindowCorrectness() > somewhere here. Ok, I'll add there that check. I've copied that code from nsXMLHttpRequest, so perhaps XHR also should have that check.

Olli, about bfcache, this patch handles these two use cases: 1: when the connection is alive (fetching data from the server): the page won't go to bfcache; 2: when the connection is closed (EventSource object is waiting for the reconnection timeout): both the page and the EventSource object freeze.

Guys, please, all major browsers already have support for EventSource. This bug is open since 2006. Can we expect EventSource support for FireFox 5? I really hate to tell people all the time to use another browser to use sites with EventSource.. And don't forget that this is an INFINITE stream, so please make sure that the UI doesn't display a busy indicator all the time (like Chromium does) or that the page isn't fully loaded yet (like Safari does).

(In reply to comment #196) > Guys, please, all major browsers already have support for EventSource. As far as I know, IE9 does't support server sent events. > This > bug is open since 2006. Can we expect EventSource support for FireFox 5? No, but for Firefox 6 hopefully. (I'm really late with the review. Sorry about that.) In Firefox one can use multipart XHR to achieve sent-sent events -like behavior (not the same behavior, but similar)

Comment on attachment 527963 [details] [diff] [review] patch >+ >+nsEventSource::nsEventSource() : >+ mStatus(PARSE_STATE_OFF), >+ mFrozen(PR_FALSE), >+ mErrorLoadOnRedirect(PR_FALSE), >+ mGoingToDispatchAllMessages(PR_FALSE), >+ mLastConvertionResult(NS_OK), >+ mReadyState(nsIEventSource::CONNECTING) Please initialize mScriptLine and mWindowID >+ nsCOMPtr<nsIJSContextStack> stack = >+ do_GetService("@mozilla.org/js/xpc/ContextStack;1"); >+ JSContext* cx = nsnull; >+ if (stack && NS_SUCCEEDED(stack->Peek(&cx)) && cx) { >+ JSStackFrame *fp = JS_GetScriptedCaller(cx, NULL); >+ if (fp) { >+ JSScript *script = JS_GetFrameScript(cx, fp); >+ if (script) { >+ mScriptFile = NS_ConvertUTF8toUTF16(JS_GetScriptFilename(cx, script)); >+ } >+ >+ jsbytecode *pc = JS_GetFramePC(cx, fp); >+ if (script && pc) { >+ mScriptLine = JS_PCToLineNumber(cx, script, pc); >+ } >+ } Could you please re-use nsJSUtils::GetCallingLocation >+ rv = os->AddObserver(this, DOM_WINDOW_FREEZED_TOPIC, PR_TRUE); FROZEN >+ JSAutoRequest ar(aContext); >+ >+ JSString* jsstr = JS_ValueToString(aContext, aArgv[0]); >+ if (!jsstr) { >+ return NS_ERROR_DOM_SYNTAX_ERR; >+ } >+ >+ size_t length; >+ const jschar *chars = JS_GetStringCharsAndLength(aContext, jsstr, &length); >+ if (!chars) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } There has been some change to JS API, I think. nsWebSocket::Initialize has now JS::Anchor here. >+ do { >+ srcCount = aCount - (p - aFromRawSegment); >+ outCount = 2; >+ >+ thisObject->mLastConvertionResult = >+ thisObject->mUnicodeDecoder->Convert(p, &srcCount, out, &outCount); >+ >+ if (thisObject->mLastConvertionResult == NS_ERROR_ILLEGAL_INPUT) { >+ // There's an illegal byte in the input. It's now the responsibility >+ // of this calling code to output a U+FFFD REPLACEMENT CHARACTER, advance >+ // over the bad byte and reset the decoder. >+ rv = thisObject->ParseCharacter(REPLACEMENT_CHAR); >+ NS_ENSURE_SUCCESS(rv, rv); >+ p = p + srcCount + 1; >+ thisObject->mUnicodeDecoder->Reset(); >+ } else { >+ for (PRInt32 i=0; i < outCount; ++i) { >+ rv = thisObject->ParseCharacter(out[i]); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ p = p + srcCount; >+ } >+ } while (p < end && >+ thisObject->mLastConvertionResult != NS_PARTIAL_MORE_INPUT && >+ thisObject->mLastConvertionResult != NS_OK); I wonder if this all could be implemented faster way. But could be optimized in a followup. >+ // check if the last byte was a bad one >+ if (thisObject->mLastConvertionResult == NS_ERROR_ILLEGAL_INPUT) { >+ thisObject->mLastConvertionResult = NS_OK; >+ } Add some comment *why* NS_OK is assigned to mLastConvertionResult >+ nsCOMPtr<nsIDOMEvent> event; >+ rv = NS_NewDOMEvent(getter_AddRefs(event), nsnull, nsnull); >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Failed to create the open event!!!"); >+ return; >+ } >+ >+ // it doesn't bubble, and it isn't cancelable >+ rv = event->InitEvent(NS_LITERAL_STRING("open"), PR_FALSE, PR_FALSE); >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Failed to init the open event!!!"); >+ return; >+ } >+ >+ rv = DispatchDOMEvent(nsnull, event, nsnull, nsnull); >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Failed to dispatch the open event!!!"); >+ return; >+ } You should make the event trusted. Same problem with error events. >+ function doTest() >+ { >+ netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); >+ var prefService = >+ Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefService); >+ domBranch = prefService.getBranch("dom.server-events."); >+ oldPrefVal = domBranch.getBoolPref("enabled"); >+ domBranch.setBoolPref("enabled", true); Don't use netscape.security.PrivilegeManager.enablePrivilege anymore in tests. Testing framework has now SpecialPowers object which has ways to get and set prefs. https://developer.mozilla.org/en/SpecialPowers >+ >+ // we get a good stress_factor by testing 10 setTimeouts and some float >+ // arithmetic taking my machine as stress_factor==1 (time=589) Ah, this is interesting, and I don't see any better way to do all the tests than using timers >+jsid nsDOMClassInfo::sOnopen_id = JSID_VOID; ... >+ SET_JSID_TO_STRING(sOnopen_id, cx, "onopen"); ... >+ sOnopen_id = JSID_VOID; ... >+ case 'o' : >+ return id == sOnopen_id; Why do you need these changes to nsDOMClassInfo. EventSource has its own onopen property anyway. >+nsGlobalWindow::NotifyDOMWindowFreezed(nsGlobalWindow* aWindow) { s/Freezed/Frozen/ ? >+#define NS_MESSAGE_EVENT 46 >+#define NS_OPENCLOSE_EVENT 47 Why are these needed? You don't add a new struct for message or openclose events. >+#define NS_MESSAGE_EVENT_START 4600 >+#define NS_MESSAGE (NS_MESSAGE_EVENT_START) >+ >+// Open and close events >+#define NS_OPENCLOSE_EVENT_START 4700 >+#define NS_OPEN (NS_OPENCLOSE_EVENT_START) >+#define NS_CLOSE (NS_OPENCLOSE_EVENT_START+1) >+ These need to be updated for trunk.

Comment on attachment 527963 [details] [diff] [review] patch >diff --git a/content/base/src/nsEventSource.cpp b/content/base/src/nsEventSource.cpp >new file mode 100755 >--- /dev/null >+++ b/content/base/src/nsEventSource.cpp >+/** >+ * A simple state machine used to manage the event-source's line buffer Thanks for the comment >+#define PARSE_STATE_OFF PRUint32(1) >+#define PARSE_STATE_BEGIN_OF_STREAM PRUint32(2) >+#define PARSE_STATE_BOM_WAS_READ PRUint32(3) >+#define PARSE_STATE_CR_CHAR PRUint32(4) >+#define PARSE_STATE_COMMENT PRUint32(5) >+#define PARSE_STATE_FIELD_NAME PRUint32(6) >+#define PARSE_STATE_FIRST_CHAR_OF_FIELD_VALUE PRUint32(7) >+#define PARSE_STATE_FIELD_VALUE PRUint32(8) >+#define PARSE_STATE_BEGIN_OF_LINE PRUint32(9) Can this be an enum? >+NS_IMETHODIMP >+nsEventSource::Close() >+{ >+ if (mReadyState == nsIEventSource::CLOSED) { >+ return NS_OK; >+ } >+ >+ nsCOMPtr<nsIObserverService> os = >+ do_GetService("@mozilla.org/observer-service;1"); I think the new way is to use mozilla::services? >+/** >+ * This Init method should only be called by C++ consumers. >+ */ >+NS_IMETHODIMP >+nsEventSource::Init(nsIPrincipal* aPrincipal, >+ nsIScriptContext* aScriptContext, >+ nsPIDOMWindow* aOwnerWindow, >+ const nsAString& aURL) >+{ >+ nsresult rv; Please move the declaration down to its first use, which is... >+ // get the src >+ nsCOMPtr<nsIURI> baseURI; >+ rv = GetBaseURI(getter_AddRefs(baseURI)); here. >+ NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SYNTAX_ERR); Really, syntax error? How can this fail? >+ // we observe when the window freezes and thaws >+ nsCOMPtr<nsIObserverService> os = >+ do_GetService("@mozilla.org/observer-service;1"); And here. >+NS_IMETHODIMP >+nsEventSource::Initialize(nsISupports* aOwner, >+ JSContext* aContext, >+ JSObject* aObject, >+ PRUint32 aArgc, >+ jsval* aArgv) >+{ >+ nsAutoString urlParam, protocolParam; These declarations can go a lot lower too. >+ >+ if (mReadyState != nsIEventSource::CONNECTING || !PrefEnabled() || >+ aArgc < 1) { >+ return NS_ERROR_DOM_SECURITY_ERR; >+ } Strange error. >+NS_IMETHODIMP >+nsEventSource::Observe(nsISupports* aSubject, >+ const char* aTopic, >+ const PRUnichar* aData) >+{ >+ nsresult rv; Lower, and DebugOnly<nsresult>. >+ if (strcmp(aTopic, DOM_WINDOW_FREEZED_TOPIC) == 0) { >+ rv = Freeze(); >+ NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Freeze() failed"); >+ } else { // if (strcmp(aTopic, DOM_WINDOW_THAWED_TOPIC) == 0) Assert that? >+ rv = Thaw(); >+ NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Thaw() failed"); >+ } >+NS_IMETHODIMP >+nsEventSource::OnStartRequest(nsIRequest *aRequest, >+ nsISupports *ctxt) >+{ >+ nsresult rv; >+ >+ rv = CheckHealthOfRequestCallback(aRequest); nsresult rv = CheckHealthOfRequestCallback(aRequest); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aRequest, &rv)); ... httpChannel = do_... >+ if (mErrorLoadOnRedirect || !requestSucceeded || >+ !contentType.EqualsLiteral(TEXT_EVENT_STREAM)) { If mErrorLoadOnRedirect is true, you've done quite a lot of unnecessary work. Maybe check these conditions as soon as possible? >+NS_METHOD >+nsEventSource::StreamReaderFunc(nsIInputStream *aInputStream, >+ void *aClosure, >+ const char *aFromRawSegment, >+ PRUint32 aToOffset, >+ PRUint32 aCount, >+ PRUint32 *aWriteCount) >+{ >+ nsresult rv; Lower... >+ do { >+ if (thisObject->mLastConvertionResult == NS_ERROR_ILLEGAL_INPUT) { >+ } else { >+ for (PRInt32 i=0; i < outCount; ++i) { i = 0 >+ *aWriteCount = aCount; This looks strange, not sure if it is. >+NS_IMETHODIMP >+nsEventSource::OnDataAvailable(nsIRequest *aRequest, >+ nsISupports *aContext, >+ nsIInputStream *aInputStream, >+ PRUint32 aOffset, >+ PRUint32 aCount) >+{ >+ nsresult rv; . >+NS_IMETHODIMP >+nsEventSource::OnStopRequest(nsIRequest *aRequest, >+ nsISupports *aContext, >+ nsresult aStatusCode) >+{ >+ nsresult ret; healthOfRequestResult, or something like that? >+ >+ ret = CheckHealthOfRequestCallback(aRequest); And initialize here >+ rv = DispatchCurrentMessageEvent(); // there is an empty line(CRCR) Space after 'line' >+/* /** >+ * Simple helper class that just forwards the redirect callback back >+ * to the nsEventSource. >+ */ >+class AsyncVerifyRedirectCallbackFwr: public nsIAsyncVerifyRedirectCallback A space before the ':', and Fwr is an abbreviation I've never seen before >+{ >+public: >+ AsyncVerifyRedirectCallbackFwr(nsEventSource *eventsource) aEventSource >+NS_IMETHODIMP >+nsEventSource::AsyncOnChannelRedirect(nsIChannel *aOldChannel, >+ nsIChannel *aNewChannel, >+ PRUint32 aFlags, >+ nsIAsyncVerifyRedirectCallback *callback) aCallback >+{ >+ nsresult rv; . >+nsresult >+nsEventSource::OnRedirectVerifyCallback(nsresult result) aResult >+{ >+ NS_ASSERTION(mRedirectCallback, "mRedirectCallback not set in callback"); >+ NS_ASSERTION(mNewRedirectChannel, "mNewRedirectChannel not set in callback"); If you're sure these can't be hit, please make them NS_ABORT_IF_FALSE. >+ nsresult rv; . >+ NS_ENSURE_SUCCESS(result, result); >+ >+ // update our channel >+ >+ nsCOMPtr<nsIInterfaceRequestor> notificationCallbacksNewChannel; Unused? >+NS_IMETHODIMP >+nsEventSource::GetInterface(const nsIID & aIID, >+ void **aResult) >+{ >+ nsresult rv = NS_OK; . >+ return wwatch->GetPrompt(window, aIID, reinterpret_cast<void**>(aResult)); Do you need the cast? >+// nsEventSource::nsEventSource ? >+nsEventSource::PrefEnabled() >+{ >+ return nsContentUtils::GetBoolPref("dom.server-events.enabled", PR_FALSE); Should this be cached? >+nsEventSource::GetLoadGroup() >+{ >+ if (doc) { >+ return doc->GetDocumentLoadGroup().get(); Why the .get? >+nsEventSource::GetBaseURI(nsIURI **aBaseURI) >+{ >+ nsresult rv; . >+ if (baseURI) { >+ *aBaseURI = baseURI; >+ NS_ADDREF(*aBaseURI); >+ return NS_OK; >+ } >+ >+ return NS_ERROR_UNEXPECTED; if (!baseURI) { return NS_ERROR_UNEXPECTED; } baseURI.forget(aBaseURI); return NS_OK; (Saves an addref) >+nsresult >+nsEventSource::SetupHttpChannel() >+{ >+ nsresult rv; . >+nsEventSource::InitChannelAndRequestEventSource() >+{ >+ nsresult rv; . >+nsEventSource::AnnounceConnection() >+{ >+ nsresult rv; . >+nsEventSource::ReestablishConnection() >+{ >+ nsresult rv; . >+nsEventSource::SetReconnectionTimeout() >+{ >+ nsresult rv; . >+nsEventSource::ConsoleError() >+{ >+ nsresult rv; . >+nsEventSource::FailConnection() >+{ >+ nsresult rv; . >+nsEventSource::CheckCanRequestSrc(nsIURI* aSrc) >+{ >+ nsresult rv; . >+ nsCOMPtr<nsIURI> srcToTest = aSrc ? aSrc : mSrc.get(); Lose the .get >+nsEventSource::TimerCallback(nsITimer* aTimer, void* aClosure) >+{ >+ nsresult rv; . >+nsEventSource::Thaw() >+{ >+ nsresult rv; . >+nsEventSource::DispatchCurrentMessageEvent() >+{ >+ nsAutoPtr<Message> message(new Message()); message = new Message(); >+ NS_ENSURE_TRUE(message.get(), NS_ERROR_OUT_OF_MEMORY); Drop this, new can't fail. >+ // removes the trailing LF from mData Assert that it's a LF? >+nsEventSource::DispatchAllMessageEvents() >+{ >+ nsresult rv; . >+ while (mMessagesToDispatch.GetSize() > 0) { >+ nsAutoPtr<Message> message(static_cast<Message*>(mMessagesToDispatch.PopFront())); = >+nsEventSource::SetFieldAndClear() >+{ >+ char first_char; >+ first_char = (char)mLastFieldName.CharAt(0); Why is this cast safe? Could you assert it is? Stopped here.

I uploaded this to tryserver.

There are still some failures when running Opera's testsuite.

Hmm, I'm not sure Opera's testrunner really work properly in any browser.

Comment on attachment 534322 [details] [diff] [review] patch Could you please file followup bugs for the (minor, IMHO) issues we still have with Opera's tests. (We're pretty much equal with webkit). I'll inform Anne about invalid tests.