This is not a must-have (or even a strong want-have) for the next fennec release, but at some point we'll want to make web sockets work under e10s. We'll want a separate IPDL protocol (PWebSocket) for it. Hopefully it won't be much work, as the trickier stuff (like authentication) should be similar enough to whatever we cook up for HTTP, and web socket messages should map pretty straightforwardly onto IPDL msgs or shmem.

I think it's a must-have, at least for parity with Fx4 Desktop.

I'm confused. I saw Paul demo using Web Sockets to control a presentation in another computer just a couple of weeks ago. Did e10s disable that feature?

@Lars: Yes: Error: docShell: Not Remotable Source File: chrome://global/content/bindings/browser.xml Firefox can't establish a connection to the server at ws://.../

I'll see what I can do with this.

Sorry for this naive comment. I don't really understand all the details of this discussion (for instance, I'm not really sure of what e10s is) so if my remark is stupid, please, just ignore it. If there is no plan to integrate correctly the websockets for the next stable Fennec version, could it be possible to disable them by default (network.websocket.enabled set to false)? This will give a change to applications that need server pushes to switch smoothly to another implementation method such multipart XHRs.

Just another comment that might interest you. If you are looking for incentive for adding websockets to Fennec you might find interesting watching this video. Go to the time position 7:03 and you will see the demo. Of course, everything is implemented with Firefox and Fennec (this is acknowledged at the end of the talk). Thanks to all of you. http://www.college-de-france.fr/default/EN/all/cha_inf2009/Cours_et_seminaire_du_20_janvi.jsp

> If there is no plan to integrate correctly the websockets for the next stable Fennec version, could it be possible to disable them by default (network.websocket.enabled set to false)? Yes--we should check this in now, in case there's any fennec release before we ship e10 websockets.

(In reply to comment #7) > If there is no plan to integrate correctly the websockets for the next stable > Fennec version, could it be possible to disable them by default > (network.websocket.enabled set to false)? Even with this pref change, sites still seem to think that Fennec supports web sockets. The window.WebSocket property still exists. Is this a bug?

I'm inclined to think that window.WebSocket existing is not a bug. For one thing, that's controlled by the DOMClassInfo stuff as far as I can tell, and I don't believe that's easy to hide based on a pref. For another, we check the pref during websocket initialization and throw a NS_ERROR_DOM_SECURITY_ERROR - I have no idea if this is to spec and people should be catching this and simply aren't.

Just for clarity, this is a blocker for release.

> This is not a must-have (or even a strong want-have) At the moment there is no websockets + canvas solution on Android. When there is one, it'll extend realtime network-aware UIs that would previously have needed a native applet / app to Android. There are people who "strongly want to have" it ;-) and I hope it'll be possible to ship with it.

I'm working on the implementation right now, and the bug is marked as blocking the final release of mobile Firefox. There is no need for advocacy.

This patch allows sites like http://websockets.org/echo.html and http://mrdoob.com/projects/multiuserpad to work seemingly correctly. There are still problems with the implementation (for example, cookies don't work yet), but I think it's the right architecture.

Rebased on top of bug 574897. Cookies should now work, and theoretically GetInterface for an auth prompt on the channel should also work. I'm stumped about what to do with the nsILoadContext block that bug 574897 added in GetInterface, however.

jdm, should someone be looking at this wip patch?

Comment on attachment 492835 [details] [diff] [review] WIP v2 I suppose it wouldn't hurt if jduell or anybody else knowledgeable wanted to look over it.

I could take a look, if anyone is going to do this sooner, please drop a note.

Now passes all cookie tests in bug 574897 with the help of SpecialPowers.

Comment on attachment 495639 [details] [diff] [review] Add cross-process websocket support. Alright, I think this is good to go. I've verified that no objects leak, random browsing of websockets-enabled websites works flawlessly, all websockets tests in the tree pass, and I've got a comfortable mental model of how the various proxy and concrete objects on the parent and child interact. Honza, if you're not the right person for this then please reassign.

Honza, looks like this can wait until bug 616733 is resolved one way or the other.

As I understand it, firefox is pref'ing this off for 4.0, so this shouldn't block the release anymore. If my understanding is incorrect, please leave a comment. Also, to be clear, I don't see any reason for this not to land if the patches are ready.

honza any update on this review?

Comment on attachment 495639 [details] [diff] [review] Add cross-process websocket support. This review isn't worth Honza's time. The websocket design has changed significantly in the new implementation, so I (or someone else) needs to sit down an reevaluate how best to remote them. In fact, I think I'll try to do that during the train rides I'll be taking in the next few days.

Hooray, it looks like remoting the new websockets patch should be significantly easier. We'll want a proxy listener object in the parent process that relays to the child, and some kind of proxy protocol handler in the child that relays the initial AsyncOpen to the parent, I think. I haven't sorted out all the details, but I'm hopeful. I'd be happy to put together a patch when I start full time at the end of May.

I think we're going to want to move on this sooner than the end of May, assuming that biesi will get a chance to review the main websockets patches before then, and that we want to land e10s support concurrently (I don't care personally about landing them at the same time, but I seem to remember other people feeling it was a requirement). jdm: please unassign yourself if you don't think you can get to this in a few weeks or less, and we'll find someone to do it.

Turns out that I've spent the majority of the past couple days on trains, so I'll be able to put up a proof-of-concept semi-patch in the near future. We can see where to go from there.

This patch is conceptually complete. I've manually put together a diff based off of the most recent proposed websockets patch, so it may or may not apply to any given tree. It also conceptually builds, as I've gone to fairly painstaking trouble of reading over all my code changes. If someone wants to take it any run with it from here, that'd be swell.

I got access to a real tree, so this is actually a patch that can be applied. I also moved the IPC classes into new files, because the originals were long and complicated enough already.

Now buildable, thanks to a few hours spent in the Paris office.

The attached patch represents the sum of my work. I'm not sure how much else I'll be able to do before I return on the 29th, so I'm unassigning myself. My work was at the point where I was attempting to test fennec against updated websocket tests to see what was still broken, last I recall.

Psyke! Here's a patch that passes all the websocket tests in the tree. The only remaining issues are that I haven't added any new logging, and the mochitest currently leaks like a bucket with no bottom.

You can never have too many GC methods in SpecialPowers.

Comment on attachment 535997 [details] [diff] [review] Remote websockets. This patch is the best one to date! Now with fewer leaks, fewer crashes and more logging. All mochitests also pass with flying colours. Jason, do you feel like taking this?

Comment on attachment 535997 [details] [diff] [review] Remote websockets. jdm already discussed much of this on IRC, but I've found some new things. I'll want to have a look at the next patch too. Test cases: I haven't looked at the mochitest(s), but let's make sure we cover - bogus URI in websocket constructor - valid but refused/unavailable URI - sync XHR in onmessage() (have server close WS: make sure readyState doesn't change to CLOSED during onmessage()). - test that involves an HTTP redirect (both failed and succeeded redirects would be good) >diff --git a/netwerk/build/nsNetModule.cpp b/netwerk/build/nsNetModule.cpp > >+static BaseWebSocketHandler* >+WebSocketHandlerConstructor(bool aSecure) >+{ >+ if (XRE_GetProcessType() != GeckoProcessType_Default) { >+ return new nsWebSocketHandlerChild(aSecure); >+ } Use IsNeckoChild()? Are you sure you want the check to be !GeckoProcessType_Default, or is ==GeckoProcessType_Content good enough? We're only supporting tab processes, right? In which case I'd think IsNeckoChild() should work fine. >diff --git a/netwerk/protocol/websocket/PWebSocket.ipdl b/netwerk/protocol/websocket/PWebSocket.ipdl >new file mode 100644 Your new files seem to be using DOS line-endings (I see lots of ^M in vim). Convert to unix (dos2unix is one util for doing that). Also, not all of your new files have emacs/vim modelines. >+ // Forwarded methods corresponding to methods on nsIWebSocketProtocolHandler s/nsIWebSocketProtocolHandler/nsIWebSocketListener/ >diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.cpp b/netwerk/protocol/websocket/nsWebSocketHandler.cpp >+#include "mozilla/net/NeckoChild.h" See comments on logging below. Also may need to remove #include of prlog.h below this. > #if defined(PR_LOGGING) >-static PRLogModuleInfo *webSocketLog = nsnull; >+PRLogModuleInfo *webSocketLog = nsnull; > #endif > #define LOG(args) PR_LOG(webSocketLog, PR_LOG_DEBUG, args) Hmm, we're going to want logging on when !DEBUG, which means we need to have FORCE_PR_LOG on AFICT. Look at how nsHttp.h does it in lines 43-61. I'm surprised we don't run into an error re-#defining LOG here--it's probably a warning? Anyway, I think you'll want to centralize this in BaseWebChannel.h. Make sure to compile an opt build and make sure that logging works in non-debug mode. Our logging is a real mess. >diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.h b/netwerk/protocol/websocket/nsWebSocketHandler.h > >+class BaseWebSocketHandler : public nsIWebSocketProtocol, As mentioned on IRC, rename "BaseWebSocketChannel" (per bug 664860) and move into its own .h/.cpp files. >+public: >+ BaseWebSocketHandler(); >+ >+ NS_DECL_NSIPROTOCOLHANDLER >+ >+ NS_SCRIPTABLE NS_IMETHOD QueryInterface(const nsIID & uuid, void **result NS_OUTPARAM) = 0; >+ NS_IMETHOD_(nsrefcnt ) AddRef(void) = 0; >+ NS_IMETHOD_(nsrefcnt ) Release(void) = 0; As mentioned on IRC, I don't think you need to declare methods you're inheriting from interfaces unless you're actually defining them, so these can be eliminated. >+ >+ NS_SCRIPTABLE NS_IMETHOD GetOriginalURI(nsIURI **aOriginalURI); >+ NS_SCRIPTABLE NS_IMETHOD GetURI(nsIURI **aURI); >+ NS_SCRIPTABLE NS_IMETHOD GetNotificationCallbacks(nsIInterfaceRequestor **aNotificationCallbacks); >+ NS_SCRIPTABLE NS_IMETHOD SetNotificationCallbacks(nsIInterfaceRequestor *aNotificationCallbacks); 80 columns max, please. Maybe break after NS_IMETHOD? I'm not used to having NS_SCRIPTABLE in the decls, which makes lines (and even the column of the 1st param) very long. We can probably scrap the NS_SCRIPTABLE decls--we don't use them in necko except a few scattered places, and they're just for dehydra analysis. >+ NS_SCRIPTABLE NS_IMETHOD GetSecurityInfo(nsISupports **aSecurityInfo) = 0; >+ >+ NS_SCRIPTABLE NS_IMETHOD AsyncOpen(nsIURI *aURI, >+ const nsACString &aOrigin, >+ nsIWebSocketListener *aListener, >+ nsISupports *aContext) = 0; >+ NS_SCRIPTABLE NS_IMETHOD Close() = 0; >+ NS_SCRIPTABLE NS_IMETHOD SendMsg(const nsACString &aMsg) = 0; >+ NS_SCRIPTABLE NS_IMETHOD SendBinaryMsg(const nsACString &aMsg) = 0; I think these pure virtual decls can also be eliminated. >+protected: >+ nsCOMPtr<nsIURI> mOriginalURI; >+ nsCOMPtr<nsIURI> mURI; >+ nsCOMPtr<nsIWebSocketListener> mListener; >+ nsCOMPtr<nsISupports> mContext; >+ nsCOMPtr<nsIInterfaceRequestor> mCallbacks; >+ nsCOMPtr<nsILoadGroup> mLoadGroup; >+ >+ nsCString mProtocol; >+ nsCString mOrigin; >+ >+ PRBool mEncrypted; indent mEncrypted to same level as other data members. > public: >- nsWebSocketSSLHandler() {nsWebSocketHandler::mEncrypted = PR_TRUE;} >+ nsWebSocketSSLHandler() {BaseWebSocketHandler::mEncrypted = PR_TRUE;} Put space between { and BaseWebSocketHandler, and btw ; and } >diff --git a/netwerk/protocol/websocket/nsWebSocketHandlerIPC.cpp b/netwerk/protocol/websocket/nsWebSocketHandlerIPC.cpp per IRC, split each class into its own file, and name WebSocketChannelChild|Parent. And we need ChannelEventQueue for all incoming IPDL traffic to child. >+bool >+nsWebSocketHandlerParent::RecvDeleteSelf() >+{ >+ LOG(("nsWebSocketHandlerParent::RecvDeleteSelf() %p\n", this)); >+ mProtocol = nsnull; >+ mAuthProvider = nsnull; >+ return Send__delete__(this); >+} Send__delete__ should be guarded with mIPCOpen. The child process could conceivably drop dead at any point. >+ >+bool >+nsWebSocketHandlerParent::RecvAsyncOpen(const IPC::URI& aURI, >+ const nsCString& aOrigin, >+ const nsCString& aProtocol, >+ const bool& aSecure) >+{ >+ LOG(("nsWebSocketHandlerParent::RecvAsyncOpen() %p\n", this)); >+ nsresult rv; >+ if (aSecure) { >+ mProtocol = >+ do_CreateInstance("@mozilla.org/network/protocol;1?name=wss", &rv); >+ } else { >+ mProtocol = >+ do_CreateInstance("@mozilla.org/network/protocol;1?name=ws", &rv); indent do_CreateInstance by 2, not 4 >+bool >+nsWebSocketHandlerParent::CancelEarly() >+{ >+ LOG(("nsWebSocketHandlerParent::CancelEarly() %p\n", this)); >+ return SendAsyncOpenFailed(); >+} Also guard this with mIPCOpen. >diff --git a/netwerk/protocol/websocket/nsWebSocketHandlerIPC.h b/netwerk/protocol/websocket/nsWebSocketHandlerIPC.h >+ >+class nsWebSocketHandlerParent : public PWebSocketParent, >+ public nsIWebSocketListener, >+ public nsIInterfaceRequestor >+{ >+private: >+ bool RecvAsyncOpen(const IPC::URI& aURI, >+ const nsCString& aOrigin, >+ const nsCString& aProtocol, >+ const bool& aSecure); >+ bool RecvClose(); >+ bool RecvSendMsg(const nsCString& aMsg); >+ bool RecvSendBinaryMsg(const nsCString& aMsg); >+ bool RecvDeleteSelf(); >+ bool CancelEarly(); >+ >+ void ActorDestroy(ActorDestroyReason why); >+ >+ nsCOMPtr<nsIAuthPromptProvider> mAuthProvider; >+ nsCOMPtr<nsIWebSocketProtocol> mProtocol; In anticipation of bug 664860 (and to avoid confusion with the Base class' String mProtocol field), let's rename mProtocol -> mChannel.

(In reply to comment #41) >>+ // Forwarded methods corresponding to methods on nsIWebSocketProtocolHandler > >s/nsIWebSocketProtocolHandler/nsIWebSocketListener/ Nope, these are from the ProtocolHandler (which is actually the channel). > >diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.cpp b/netwerk/protocol/websocket/nsWebSocketHandler.cpp > > >+#include "mozilla/net/NeckoChild.h" > > See comments on logging below. Also may need to remove #include of prlog.h > below this. > > > #if defined(PR_LOGGING) > >-static PRLogModuleInfo *webSocketLog = nsnull; > >+PRLogModuleInfo *webSocketLog = nsnull; > > #endif > > #define LOG(args) PR_LOG(webSocketLog, PR_LOG_DEBUG, args) > > Hmm, we're going to want logging on when !DEBUG, which means we need to have > FORCE_PR_LOG on AFICT. Look at how nsHttp.h does it in lines 43-61. I'm > surprised we don't run into an error re-#defining LOG here--it's probably a > warning? Anyway, I think you'll want to centralize this in > BaseWebChannel.h. Make sure to compile an opt build and make sure that > logging works in non-debug mode. > > Our logging is a real mess. I really want to keep these sorts of things out of header files, because it just perpetuates the mess. What I've done in my updated patch is add LOG defines per file as needed, store webSocketLog in BaseWebSocketChannel.cpp, and add extern definitions to the other consumers, which is an acceptable duplication/cleanliness tradeoff in my opinion. > >diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.h b/netwerk/protocol/websocket/nsWebSocketHandler.h > >+public: > >+ BaseWebSocketHandler(); > >+ > >+ NS_DECL_NSIPROTOCOLHANDLER > >+ > >+ NS_SCRIPTABLE NS_IMETHOD QueryInterface(const nsIID & uuid, void **result NS_OUTPARAM) = 0; > >+ NS_IMETHOD_(nsrefcnt ) AddRef(void) = 0; > >+ NS_IMETHOD_(nsrefcnt ) Release(void) = 0; > > As mentioned on IRC, I don't think you need to declare methods you're > inheriting from interfaces unless you're actually defining them, so these > can be eliminated. Turns out we refuse to link without these here. It might be an ambiguity thing since it's inheriting from multiple nsISupports bases? > >+bool > >+nsWebSocketHandlerParent::RecvDeleteSelf() > >+{ > >+ LOG(("nsWebSocketHandlerParent::RecvDeleteSelf() %p\n", this)); > >+ mProtocol = nsnull; > >+ mAuthProvider = nsnull; > >+ return Send__delete__(this); > >+} > > Send__delete__ should be guarded with mIPCOpen. The child process could > conceivably drop dead at any point. Well, any point is a bit much. I mean, I guess if nulling out a member triggered a destructor which spun the event loop... (ok, I'll fix it.)

Haven't done anything further with tests, but this makes all the other requested changes.

From test_websocket.html comments: >Test cases: I haven't looked at the mochitest(s), but let's make sure we cover >- bogus URI in websocket constructor * 1. client tries to connect to a http scheme location; * 4. client tries to connect using a relative url; >- valid but refused/unavailable URI * 3. client tries to connect to an non-existent ws server; * 5. client uses an invalid protocol value; >- sync XHR in onmessage() (have server close WS: make sure readyState doesn't change to CLOSED during onmessage()). >- test that involves an HTTP redirect (both failed and succeeded redirects would be good) I'll see what I can do about these last two.

>Hmm, we're going to want logging on when !DEBUG, which means we need to have >FORCE_PR_LOG on AFICT. Look at how nsHttp.h does it in lines 43-61. I'm >surprised we don't run into an error re-#defining LOG here--it's probably a >warning? Anyway, I think you'll want to centralize this in BaseWebChannel.h. >Make sure to compile an opt build and make sure that logging works in non-debug >mode. NSPR logging is not part of opt builds.

Reentrancy test.

Now with working nspr logging.

Test updated to pass with new protocol restrictions. I also confirmed that it does in fact fail without the IPDL buffering present.

Nice test. So good, in fact, that it breaks non-e10s WS. (did you check? It breaks for me, at least, with TEST-UNEXPECTED-FAIL | | onclose called before onmessage! (I changed wording--yours was wrong?) TEST-UNEXPECTED-FAIL | | readystate should not be CLOSED - didn't expect 3, but got it TEST-UNEXPECTED-FAIL | | got a close callback before finishing onmessage Not checking in--I'll move to a new bug.

Comment on attachment 543566 [details] [diff] [review] DON'T LAND: Re-entrancy test, with change of wording Moved to bug 668948

Looks good. The level of error/state checking is less in the e10s code than the mainline, but that looks like a good thing--the nsWebsocket.cpp code does enough checking that many of the things nsWebSocketHandler.cpp is checking for are not possible. Should cleanup all that logic at some point. Added some of those checks in e10s in form of assertions. Running through try one last time, then will land.

http://hg.mozilla.org/integration/mozilla-inbound/rev/4a894d4dc4da I'm landing the main patch separately from the SpecialPowers patch, as with it I'm occasionally seeing SpecialPowers is not defined at http://mochi.test:8888/tests/Harness_sanity/file_SpecialPowersFrame1.html:11 mainly only on windows debug. I don't see any reason why it could be causing that, but just in case..

Comment on attachment 535998 [details] [diff] [review] Update websocket test to use SpecialPowers. Special powers patch landed as http://hg.mozilla.org/integration/mozilla-inbound/rev/7a747adc8303

Adding dev-doc-needed for the SpecialPowers API addition.

http://hg.mozilla.org/mozilla-central/rev/4a894d4dc4da http://hg.mozilla.org/mozilla-central/rev/7a747adc8303

(In reply to comment #55) > Comment on attachment 535998 [details] [diff] [review] [review] > Update websocket test to use SpecialPowers. > > Special powers patch landed as > > http://hg.mozilla.org/integration/mozilla-inbound/rev/7a747adc8303 Please check your commits (hg out) before pushing stuff.

I tried http://websockets.org/echo.html and http://mrdoob.com/projects/multiuserpad with browser.tabs.remote=true and the pages work. With browser.tabs.remote=false they don't work, is this the expected result? Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110804 Firefox/7.0a2 Fennec/7.0a2 Device: LG Opyimus 2X

From a developer standpoint, is the key issue here that WebSockets now work on mobile?

Yes. The problem with testing this is that there are not many public tests that exercise the more recent websocket protocol, plus virtually no tests make use of the prefixed version.

OK, the compatibility tables for WebSockets have been updated.

Note that there are virtually no demos online on the web that work in Mozilla, because WebSocket was prefixed with "Moz" in bug 659324.

It turns out there is one site that supports MozWebSocket, that is http://websocketstest.com/ . I used that for a litmus test. https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=26364

Mozilla /5.0 (Android;Linux armv7l;rv:7.0) Gecko/20110830 Firefox/7.0 Fennec/7.0 Device: Motorola DROID2 Verified site: http://websocketstest.com/ and every thing was reported as working on that page, except for the 'HTTP Proxy' part. Marking bug as verified fixed.