User-Agent: Mozilla/5.0 (X11; U; Linux i686; pt-BR; rv:1.9.0.11) Gecko/2009060309 Ubuntu/8.04 (hardy) Firefox/3.0.11 Build Identifier: Make possible for workers to create websockets. Reproducible: Always

Confirming bug still exists in Firefox 4 Beta 1.

I will start to work on this next week.

Thank you, just a note that this actually had a decent performance improvement for me on Chrome... So its a very worthwhile implementation. I put my WebSocket client on a WebWorker thread to process the messages and send the data back with postMessage, and it helped a good bit. I'm guessing the downloading and uploading of data can be CPU consuming and slows down rendering on main thread.

Sorry, I've started this work, however I'm a lot busy right now in my new work. Can anyone else implement this bug, please?

http://stackoverflow.com/questions/8841837/webworkers-and-websockets-with-firefox/ suggests people are running into this in the wild.

changing category to networking:websockets - its really the content side not networking, but its a better match than 'general'.

This *just* needs someone to write an implementation using JSAPI directly, the same way we've done with all other APIs that we have in workers. And also to figure out how to access websocket channels off the main thread. This is certainly a lot of work though. And the first part (i.e. everything by the websocket channels part) will get a lot easier once we have the new DOM Bindings.

Jonas, Is there anyone you can suggest to work on this? It sounds like it's mostly DOM work not necko, module the off-main-thread websocket channel issue. (Can a websocket channel be accessed simultaneously by the main thread and the worker thread? If not, it's probably not so much work, I'd guess.)

No, I don't have anyone in mind. A websocket object will only ever be accessed on a single thread for the lifetime of the object.

Bug still in 12, even though WebSocket is no longer prefixed and it's part of the WebSocket spec: From http://www.w3.org/TR/websockets/#the-websocket-interface: > This constructor must be visible when the script's global object is either > a Window object or an object implementing the WorkerUtils interface. It's causing a major problem for us, because our library will have to special case on firefox and not use a webworker when we want to do our connection to the remote server. This page allows you to check to see if WebSocket is available inside a WebWorker.

I'm just using bugzilla as a backup of my code. I hope to have something ready for the next week.

Compared with the previous implementation this patch does: 1. WebSocketHelper - a virtual class that implements the "logic" of the WebSocket. It can run on any thread. It needs a WebSocketProxy in order to comunicate to/from the nsWebSocketChannel/nsWebSocketListener. 2. WebSocketProxy - a virtual class that is needed by a WebSocketHelper. 3. WebSocketImpl - It inherits the WebSocketProxy - it runs on the main thread ONLY and it proxies any communication from nsWebSocketChannel/nsWebSocketListener to the WebSocketHelper. 4. WebSocketWorkerImpl - It inherits the WebSocketProxy. It runs on a worker thread. It creates+proxies a WebSocketImpl running on the main thread. 5. mozilla::dom::WebSocket - it inherits the WebSocketHelper and this is the object for the main thread. It uses the WebSocketImpl - everything runs in the main thread. 6. mozilla::dom::Workers::WebSocket - it inherits the WebSocketHelper and it's the object for the worker thread. It uses the WebSocketWorkerImpl.

unfortunately there are some UTF-8 non-characters in a test.js file...

amarchesini, So this patch can't be viewed in bugzilla splinter, nor does it apply cleanly to the tree (you seem to be assuming that the existing code in /content/base/src is called "WebSocket.h|cpp", when the files are called "nsWebSocket.h|cpp", for one thing). Please fix and upload a patch that at least applies to the tree. I'd also like to be a reviewer here: you definitely need either me or :smaug to +r the patch, in addition to someone who knows Workers (I assume bent or sicking). Keeping it to 2 reviewers for starters is probably enough. Looking forward to seeing what you've done!

I think what you need to do is to enable "git style patches" in mercurial. Add the following to your .hgrc [diff] git = 1

> you definitely need either me or :smaug to +r the patch To be precise: I can't give you the final +r to the patch (not a DOM peer: smaug is the peer who's been doing +r for websockets), but I'm happy to take a first look at it since I've been working on websockets stuff more than anyone else recently.

(In reply to Jason Duell (:jduell) from comment #15) > amarchesini, > > So this patch can't be viewed in bugzilla splinter, nor does it apply > cleanly to the tree (you seem to be assuming that the existing code in > /content/base/src is called "WebSocket.h|cpp", when the files are called > "nsWebSocket.h|cpp", for one thing). > > Please fix and upload a patch that at least applies to the tree. Jason, this patch applies over the patch in the dependent bug (bug 775368), as is customary.

Any news about this?

Hey, sorry, all my time is being used for b2g stuff at the moment. I'll get to this as soon as we clear our next deadline.

same here. Very excited about this work, but slammed with B2G until the 27th.

Rebased + some bug fixed.

rebased on 785319

Support for preferences.

Comment on attachment 658515 [details] [diff] [review] patch 3 Review of attachment 658515 [details] [diff] [review]: ----------------------------------------------------------------- You need some comments explaining how this all works. Figuring out what all these different classes do is very hard. There are proxies that inherit from impls and impls that inherit from proxies and who knows what else going on. ::: content/base/src/WebSocketImpl.cpp @@ +290,5 @@ > +// C++ traverse > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(WebSocketImpl) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mChannel) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mURI) > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END I don't think this is necessary. Channels and URIs should not be cycle collected. @@ +342,5 @@ > + > + nsCOMPtr<nsILoadGroup> loadGroup; > + GetLoadGroup(getter_AddRefs(loadGroup)); > + if (loadGroup) > + loadGroup->RemoveRequest(this, nullptr, NS_OK); if (foo) { bar; } ::: dom/workers/WebSocketWorker.cpp @@ +140,5 @@ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + MOZ_COUNT_CTOR(mozilla::dom::workers::WebSocket); > + > + // Creation of the proxy: > + SetProxy(new WebSocketWorkerImpl(this)); I think you should probably drop the WebSocketWorkerImpl and just create the proxy on the main thread. ::: dom/workers/WebSocketWorkerImpl.cpp @@ +281,5 @@ > +WebSocketWorkerImpl::WebSocketWorkerImpl(WebSocket* aWebSocket) > +: WebSocketProxy(aWebSocket), > + mWebSocket(aWebSocket) > +{ > + MOZ_COUNT_CTOR(mozilla::dom::workers::WebSocketImpl); This doesn't match the class name, which is going to be very confusing when someone leaks these. ::: dom/workers/WebSocketWorkerImpl.h @@ +29,5 @@ > + { > + MOZ_COUNT_DTOR(mozilla::dom::workers::WebSocketWorkerProxy); > + } > + > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebSocketWorkerProxy) This class inherits from WebSocketImpl, which has code in its ctor/dtor to abort if it's constructed or destroyed off the main thread, but it has threadsafe refcounting? That doesn't make any sense. @@ +101,5 @@ > + > +public: > + WorkerPrivate* GetWorkerPrivate() { return mWebSocket->GetWorkerPrivate(); } > + > +public: public: foo public: bar Please drop the second 'public'.

What is happening with this bug?

I have to spend time on this bug mostly writing documentation. I think the best approach can be to review this code face to face with someone because the patch is huge, it changes too much code etc.

Jagex has released a Runescape Bestiary: http://services.runescape.com/m=news/try-the-runescape-bestiary. At this moment though, the actual bestiary only shows a big Google Chrome logo and a request to "upgrade" my browser to the latest Google Chrome, as it apparently is the only browser which "fully" supports HTML5 and WebGL. I first wanted to file a tech evangelism bug, but on the forum, Mod Chrisso mentions this bug is the cause of not having it available in Firefox (source: http://services.runescape.com/m=forum/l=0/sl=0/forums.ws?15,16,839,64246943,goto,8). I don't know if this comment adds anything to the priority of this bug, but I guess it doesn't hurt to mention it :)

Comment on attachment 658515 [details] [diff] [review] patch 3 I'm going to assume for now that khuey's r- means I can wait for a new patch before reviewing. I also assume this bug will become more lively when B2G cools off a bit.

This is required by the Runescape Bestiary app[1], if you bypass the userAgent (contains 'chrome') and appVersion (contains 'Chrome/N.', N > 21) sniffing that redirects to a 'download chrome' overlay. All the other HTML5 tech seems to be in place except for WebSockets in Workers.

Vlad and I agree that we should priorities this as a result of the attention it's likely to get. Runescape is solid brand and has a US Alexa rank of 1,446. I'm putting this up to a P1 for games as a result and let's try and see if we can get them to clear that error under Firefox. It would be good to have this working by GDC (end of March).

I've been on the CC list for this issue for a while - if there's anything we (Jagex) can do, please do get in touch with me directly. Of course as soon as a patch hits release builds, we'll remove the Chrome only guard on the Bestiary and let our player base know. We have internal release targets for the full RuneScape game on this platform in the next couple of months and of course we'd love for it to run on FireFox at launch. Many thanks.

We're in the process of totally reworking our memory model and event systems for workers in order to do things like this. End of March is probably too optimistic for that.

Thanks for the Kyle for the info on time frame. I'll reach out to Chris and see if there are some alternative solution possible in the short term.

Is there an updated on progress?

See blocking bugs

I don't think Andrea is working on this right now. We're also going to have to have a dramatically different approach here once bug 853893 is done since we'll actually be able to mostly use the main-thread implementation on the worker thread.

All blocking bugs has been fixed! Who will deal with this bug?

I think most of this work is more DOM-y than necko-y, i.e. I can't think of anyone in the networking team who knows the worker-related issues well enough to work on this. Doug, any thoughts on who could take this?

Jason: The one piece that we could need help with is the ability to use a websocket channel directly from the worker thread. We can always proxy through the main thread, but the less we need to, the better.

From a brief look over the WebSocketChannel code, it looks like it's fine to send() messages from non-main threads. Also ok to close() off-main. We don't currently have a method to deliver incoming messages to a non-main thread, but we could add one. AsyncOpen() is trickier--we currently use the Prefs service, and also do have a global admissionsService object that we might need to make thread-safe. We'd also need to make sure we don't run afoul of nsIURI thread usage constraints (from talking to bsmedberg it sounds like we need to create URIs on the main thread but can use them on other threads as long as we don't modify them concurrently from multiple threads--but I got a sense from sicking that nsIURI usage is more limited?). My gut sense is that if we can live with main thread asyncOpen but have off-main send/recv we're good enough. I'm not 100% sure about some of the accessor functions: uri, baseUri, loadGroup, notificationCallbacks, etc. The referred-to objects would need to have threadsafe addref/release at a minimum. URIs don't. LoadGroup/callbacks generally do. Do you want to create websocketChannel objects off-main thread, or can that be main thread-only too? Let me know if you want off-main websocket msg delivery and I'll open a bug for it.

(In reply to Jason Duell (:jduell) from comment #43) > Do you want to create websocketChannel objects off-main thread, or can that > be main thread-only too? > > Let me know if you want off-main websocket msg delivery and I'll open a bug > for it. I would like to be able to create the WebSockets directly from inside the WebWorker (not on the main thread). In my own use case, I'm developing a communications library that this way it could be loaded and run on a background worker...

We can hopefully allow JS code to create a "Websocket" (which is a DOM object) on a worker thread, even if the implementation winds up proxing the actual websocketchannel creation/asyncOpen to the main thread.

(In reply to Jason Duell (:jduell) from comment #45) > We can hopefully allow JS code to create a "Websocket" (which is a DOM > object) on a worker thread, even if the implementation winds up proxing the > actual websocketchannel creation/asyncOpen to the main thread. Could this be also done with WebRTC PeerConnection/DataChannels?

> Could this be also done with WebRTC PeerConnection/DataChannels? I don't know, but maybe jesup does...

(In reply to Jason Duell (:jduell) from comment #43) > From a brief look over the WebSocketChannel code, it looks like it's fine to > send() messages from non-main threads. Also ok to close() off-main. Is that true in child processes too? > We > don't currently have a method to deliver incoming messages to a non-main > thread, but we could add one. That would be awesome! Though see above about child processes. > AsyncOpen() is trickier That's fine. In general, the more callsites that we make available off-main-threads, the less code we have to fork between main-thread/off-main-thread. It's not an all-or-nothing. As long as we document in the interface which parts can be used off the main thread, and which ones can't. > I'm not 100% sure about some of the accessor functions: uri, baseUri, > loadGroup, notificationCallbacks, etc. The referred-to objects would need to > have threadsafe addref/release at a minimum. URIs don't. > LoadGroup/callbacks generally do. Could we avoid calling these from the API implementation then? > Do you want to create websocketChannel objects off-main thread, or can that > be main thread-only too? Creating the channel on the main thread, but doing as much else as we can off the main thread, sounds like a good compromise. > Let me know if you want off-main websocket msg delivery and I'll open a bug > for it. Please do. Please, lets keep other APIs to other bugs. This bug is about WebSocket and WebSocket only. And we do know that plenty of people want it, that's not at question.

(In reply to Jason Duell (:jduell) from comment #47) > > Could this be also done with WebRTC PeerConnection/DataChannels? > > I don't know, but maybe jesup does... On https://bugzilla.mozilla.org/show_bug.cgi?id=922363 he was the one suggesting me that maybe some of the work done for WebSockets could be re-used for DataChannels... :-)

(In reply to Jason Duell (:jduell) from comment #47) > > Could this be also done with WebRTC PeerConnection/DataChannels? > > I don't know, but maybe jesup does... On https://bugzilla.mozilla.org/show_bug.cgi?id=922363 he was the one suggesting me that maybe some of the work done for WebSockets could be re-used for DataChannels... :-)

e10s is hard to optimize here--we can send/rev IPDL messages to non-main threads, but *all* msgs have to go to those thread. In this case we'd ideally like the socket transport thread to talk IPDL to the worker thread in the child. We might be able to receive the initial IPDL channel constructor on the socket transport, and have it just dispatch the actual construction to the main thread. I'll see what I can do. > Could we avoid calling these from the API implementation then? If you mean avoid calling them internally from the other API functions (open, send, close, etc), I think the answer is yes. Filed bug 925623 for the necko off-main thread work here. (clearing needsinfo for :jesup. piranna, feel free to email/ping him elsewhere on the datachannels stuff.)

(In reply to Jason Duell (:jduell) from comment #51) > (clearing needsinfo for :jesup. piranna, feel free to email/ping him > elsewhere on the datachannels stuff.) Ok, thanks for the advice.

what is the current status of this bug? is somebody working on it or does it need other bugs to be fixed first?

What still needs to be done to get this bug fixed?

I'm working on it. The main problem is the event management in workers. But it should be too complex. Maybe next week I submit a patch for a review.

This first patch splits DOMEventTargetHelper from the rest of the interfaces. The reason of this is because DOMEventTargetHelper is not thread-safe.

By "not threadsafe", do you mean that you can't use it in worker threads, or that you can't use it from multiple threads at the same time? It should be possible to use DOMEventTargetHelper from worker threads. However you indeed won't be able to then addref it from the main thread. But I'd rather try to avoid calling addref/release from the main thread than to try to avoid using DOMEventTargetHelper. You will have to use cycle collection of the WebSocket object anyway, and that will make it impossible to call addref/release from the main thread. What we do in other APIs is to addref on the worker thread, then send some message to the main thread so that work can be done there, then send a message back to the worker thread in order to do the release. Would that be possible here too?

> By "not threadsafe", do you mean that you can't use it in worker threads, or > that you can't use it from multiple threads at the same time? The second one. For how the WebSocketChannel works, I have to call a couple of methods from the main-thread. But I cannot move a DOMEventTargetHelper from a worker thread to the main thread because it's not thread-safe. So what this patch does is to split DOMEventTargetHelper and the rest. The rest is a WebSocketImpl that can be moved to any thread and it talks with its WebSocket DOMEventTargetHelper object only on the right thread. > Would that be possible here too? Sure :) this is what I'm doing.

This second patch has 2 tests: a basic test and an advanced one. The basic test is a copy of content/base/test/test_websocket_basic.html and it passes. The second test is a copy of content/base/test/test_websocket.html but it still have problems in WebSocketChannel object. I'll discuss these issues by emails with Steve and jduell.

The only remaining issue is about test44 where it crashes because of this: js/xpconnect/src/xpcprivate.h:273: // These get non-addref'd pointers static nsXPConnect* XPConnect() { // Do a release-mode assert that we're not doing anything significant in // XPConnect off the main thread. If you're an extension developer hitting // this, you need to change your code. See bug 716167. if (!MOZ_LIKELY(NS_IsMainThread())) MOZ_CRASH(); Unwrapping the param of send(): #3 0x00007fffef979942 in mozilla::dom::UnwrapArg<nsIDOMBlob, nsIDOMBlob> (cx=0x7fff280295d0, v=..., ppArg=0x7fff327fc430, ppArgRef=0x7fff327fc400, vp=...) at ../../dist/include/mozilla/dom/BindingUtils.h:65 #4 0x00007fffeff8ef18 in mozilla::dom::WebSocketBinding::send (cx=0x7fff280295d0, obj=..., self=0x7fff280d3e60, args=...) at ../build/dom/bindings/WebSocketBinding.cpp:538

You can't use the main-thread Blob binding on workers, of course. Sounds like you need separate worker and non-worker bindings for WebSocket, or to convert Blob to WebIDL.

(In reply to Boris Zbarsky [:bz] from comment #63) > or to convert Blob to WebIDL. ... which is bug 827823 for those playing along at home.

(In reply to Andrew Overholt [:overholt] from comment #64) > (In reply to Boris Zbarsky [:bz] from comment #63) > > or to convert Blob to WebIDL. > > ... which is bug 827823 for those playing along at home. I don't know if that bug is going to land soon. Maybe we can have a temporary solution? What about if we change the webidl file in: void send(any data); Then, when the blob is ported to WebIDL, we can re-enable the current webidl. How does it sound?

"Sounds like you need separate worker and non-worker bindings for WebSocket". The the codegen will just "deal", I would think (by passing blobs to your worker binding as JSObject* and letting you handle the case when it's not actually a blob).

Added to docs: https://developer.mozilla.org/en-US/docs/Web/Reference/Functions_and_classes_available_to_workers

Yeah, using 'any' sounds ok for now. That way we can use the same code in main thread and in workers which is a big win.

With this custom send() method WebSockets are fully working on workers. I'm going to push these patches to try and I'll post the result here. jduell, I ask you to review this code, but maybe you want a feedback from bent for workers and bz for the JS api/WebIDL.

https://tbpl.mozilla.org/?tree=Try&rev=eaab57a27f62 It's not fully green. The issues are related to WebSocketChannel, probably. I'm debugging it.

Comment on attachment 8410177 [details] [diff] [review] patch 1 - WebSocketImpl Review of attachment 8410177 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me overall. +r on the necko bits, but you need a DOM/content peer for the rest. smaug has done the previous websocket reviews FWIW.

Comment on attachment 8411687 [details] [diff] [review] patch 3 - Custom WebSocket::Send() for workers Review of attachment 8411687 [details] [diff] [review]: ----------------------------------------------------------------- No necko bits in this one.

Sorry, the first patch was not up-to-date.

Comment on attachment 8417945 [details] [diff] [review] patch 1 - WebSocketImpl I tried to apply this to m-c and to inbound and then tried some older changeset as parent too, but no luck. Is this stuff on top of some other patches?

Comment on attachment 8417945 [details] [diff] [review] patch 1 - WebSocketImpl > nsRefPtr<WebSocket> webSocket = new WebSocket(ownerWindow); >- nsresult rv = webSocket->Init(aGlobal.GetContext(), principal, >- aUrl, protocolArray); >+ nsresult rv = webSocket->mImpl->Init(aGlobal.GetContext(), principal, >+ aUrl, protocolArray); In this kind of setup Init() shouldn't keep mImpl alive, but the caller. So create first websocket, then create Impl and assign it to local nsRefPtr and then call Init. Something like that. But hard to debug the issue without knowing how to build this stuff.

Based on the assertions something is going wrong with threads. Assertion failure: mTargetThread, at /builds/slave/try-lx-00000000000000000000000/build/netwerk/ipc/ChannelEventQueue.cpp:74 certainly doesn't look good. Though, that assertion seems to be about the second patch.

Could you push to try without the second patch, and perhaps tell how to build the first patch.

Here the 2 patches you have to apply before pushing my 2 patches. On try: https://tbpl.mozilla.org/?tree=Try&rev=5a7447fc364a

Just a tiny bit red tryserver.

https://tbpl.mozilla.org/?tree=Try&rev=8f02831c7184 This new push has a change suggested by smaug.

Comment on attachment 8417945 [details] [diff] [review] patch 1 - WebSocketImpl > if (mKeepingAlive && !shouldKeepAlive) { > mKeepingAlive = false; >- static_cast<EventTarget*>(this)->Release(); >+ >+ mImpl->Release(); >+ mImpl = nullptr; This looks wrong. mImpl should set to null only when mImpl object is actually going away, not when we update the mKeepingAlive flag > void > WebSocket::DontKeepAliveAnyMore() > { > NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); > if (mKeepingAlive) { > mKeepingAlive = false; >- static_cast<EventTarget*>(this)->Release(); >+ >+ mImpl->Release(); >+ mImpl = nullptr; > } Similar here.

Comment on attachment 8421842 [details] [diff] [review] patch 2 - WebSocketChannel::GetEffectiveURL() in IPC Review of attachment 8421842 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but one nit. If you agree to it and fix it, feel free to mark next patch +r w/o review ::: netwerk/protocol/websocket/WebSocketChannelChild.cpp @@ +56,5 @@ > { > NS_ABORT_IF_FALSE(NS_IsMainThread(), "not main thread"); > > LOG(("WebSocketChannelChild::WebSocketChannelChild() %p\n", this)); > BaseWebSocketChannel::mEncrypted = aSecure; So it looks like WebSocketChannelChild is currently (i.e. without your patch) not keeping mEncrypted up to date with redirects to wss://. WebSocketChannel resets it here: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#2536 But the Child is currently only setting it in its constructor. That's a bug. Let's also update mEncrypted in ::OnStart along with mSecure, or better yet, collapse them into one variable (I don't think it's possible for them to validly have different values).

Comment on attachment 8421843 [details] [diff] [review] patch 3 - WebSocket in Workers As discussed on IRC WebSocketImpl may be deleted in non-owner-of-mParent thread, which causes mParent to be released in the wrong thread. Disconnect() also must not be called in a random thread, or WebSocket::mImpl must be used in a threadsafe way. Probably the latter since Disconnect() does some other stuff which need to be main-thread only.

Comment on attachment 8421844 [details] [diff] [review] patch 1 - WebSocketImpl >+ ~WebSocketImpl() >+ { >+ // If we threw during Init we never called disconnect >+ if (mDisconnected) { Shouldn't this be !mDisconnected. > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(WebSocket, > DOMEventTargetHelper) >- tmp->Disconnect(); >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mPrincipal) >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mURI) >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mChannel) >+ if (tmp->mImpl) { >+ tmp->mImpl->Disconnect(); >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mPrincipal) >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mURI) >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mChannel) >+ tmp->mImpl = nullptr; You could just assert here that mImpl is null. Disconnect() should have set it to null already. >+WebSocketImpl::Init(JSContext* aCx, >+ nsIPrincipal* aPrincipal, >+ const nsAString& aURL, >+ nsTArray<nsString>& aProtocolArray) > { > NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); > MOZ_ASSERT(aPrincipal); > >- if (!PrefEnabled()) { >- return NS_ERROR_DOM_SECURITY_ERR; >- } >+ nsRefPtr<WebSocketImpl> kungfuDeathGrip = this; Tiny bit ugly, but I guess this is fine to ensure WebSocket::mImpl is deleted correctly if Init fails Could you add a comment why kungfuDeathGrip is needed here.

Jduell, can you review this patch? It contains the changes you proposed plus something else, still related to WebSocketChannel. Thanks.

It's almost green. Fixing the remaining issues.

Comment on attachment 8423820 [details] [diff] [review] patch 2 - WebSocket in workers So mImpl is still accessed unsafely in several threads (main thread and worker thread)

https://tbpl.mozilla.org/php/getParsedLog.php?id=39963172&tree=Try&full=1 is worrisome.

Comment on attachment 8425417 [details] [diff] [review] patch 2 - WebSocket in workers Please be consistent with * usage. * should go with the type, not with param/variable name. >+ void ReferenceObject(); >+ void ReleaseObject(); These need some comments. And should the first one be AddRefObject() >+class DispatchEvent : public WorkerRunnable DispatchEvent sounds very generic name, and don't we really have something like this already in workers code? > nsresult > WebSocketImpl::CloseConnection(uint16_t aReasonCode, > const nsACString& aReasonString) > { >- NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); >+ // This method can be called from the main-thread and from the worker-thread. >+ Assert that. Perhaps MOZ_ASSERT(mWorkerPrivate || NS_IsMainThread()); >@@ -1254,34 +1844,112 @@ WebSocket::UpdateMustKeepAlive() > { > shouldKeepAlive = false; > } > } > } > > if (mKeepingAlive && !shouldKeepAlive) { > mKeepingAlive = false; >- mImpl->Release(); >+ mImpl->ReleaseObject(); Is this really right. We end up calling UnregisterFeature, yet WS may stay alive (because there is a js reference to it) and we then add a new event listener and RegisterFeature is called. While those runnables are being processed it is possible to get stuff gced in the worker, I think >+void >+WebSocket::Send(JS::Handle<JSObject*> aData, >+ ErrorResult& aRv) Hmm, why do we need this on workers? Some special case in bindings? > NS_IMETHODIMP > WebSocketImpl::GetLoadGroup(nsILoadGroup** aLoadGroup) > { > *aLoadGroup = nullptr; > >- nsresult rv; >- nsIScriptContext* sc = mParent->GetContextForEventHandlers(&rv); >- nsCOMPtr<nsIDocument> doc = >- nsContentUtils::GetDocumentFromScriptContext(sc); >- >- if (doc) { >- *aLoadGroup = doc->GetDocumentLoadGroup().take(); >+ if (mParent) { >+ nsresult rv; >+ nsIScriptContext* sc = mParent->GetContextForEventHandlers(&rv); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } >+ >+ nsCOMPtr<nsIDocument> doc = >+ nsContentUtils::GetDocumentFromScriptContext(sc); >+ >+ if (doc) { >+ *aLoadGroup = doc->GetDocumentLoadGroup().take(); >+ } > } > > return NS_OK; > } Hmm, why don't we always have a load group? If we're about to cancel all the requests in a load group, and the ws running in a worker doesn't have one, that one isn't cancelled? That is btw a case to test. What happens if a worker is using ws while a new page is loaded? Or what if there is an active worker ws when stop is called? >diff --git a/content/base/test/test_websocket.html b/content/base/test/test_websocket.html >--- a/content/base/test/test_websocket.html >+++ b/content/base/test/test_websocket.html >@@ -67,17 +67,17 @@ > * 42. non-char utf-8 sequences > * 43. Test setting binaryType attribute > * 44. Test sending/receving binary ArrayBuffer > * 45. Test sending/receving binary Blob > * 46. Test that we don't dispatch incoming msgs once in CLOSING state > * 47. Make sure onerror/onclose aren't called during close() > */ > >-var first_test = 1; >+var first_test = 3; er what? >diff --git a/dom/workers/test/websocket_worker.js b/dom/workers/test/websocket_worker.js >new file mode 100644 >--- /dev/null >+++ b/dom/workers/test/websocket_worker.js This has tons of same code as test_websocket.html Could you please do hg copy?

> >+void > >+WebSocket::Send(JS::Handle<JSObject*> aData, > >+ ErrorResult& aRv) > Hmm, why do we need this on workers? > Some special case in bindings? Because Blobs are not supported by WebIDL bindings at the moment. So we need to use a custom "Send()" method for this. Comment 63 is about this. I'm sending a new version of this patch to try and see how it goes.

Comment on attachment 8423815 [details] [diff] [review] patch 0 - WebSocketChannel fix Review of attachment 8423815 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: netwerk/protocol/websocket/WebSocketChannel.cpp @@ -339,5 @@ > // Determine if we will open connection immediately (returns true), or > // delay/queue the connection (returns false) > static void ConditionallyConnect(WebSocketChannel *ws) > { > - NS_ABORT_IF_FALSE(NS_IsMainThread(), "not main thread"); So you got rid of the assert here, yet the only function that calls this has it's own main thread assert... Not a big deal.

> >+class DispatchEvent : public WorkerRunnable > DispatchEvent sounds very generic name, and don't we really have something > like this already in workers code? I don't think we have. But in case I can move it to some better place. > >+void > >+WebSocket::Send(JS::Handle<JSObject*> aData, > >+ ErrorResult& aRv) > Hmm, why do we need this on workers? > Some special case in bindings? Because we don't support Blobs in WebIDL yes, so we need this custom Send() method. > > NS_IMETHODIMP > > WebSocketImpl::GetLoadGroup(nsILoadGroup** aLoadGroup) > > { > Hmm, why don't we always have a load group? The load group is needed for get the PBrowser object in WebSocketChannelChild.

Comment on attachment 8445283 [details] [diff] [review] patch 2 - WebSocket in workers > AutoJSAPI jsapi; >- if (NS_WARN_IF(!jsapi.InitUsingWin(GetOwner()))) { >- return NS_ERROR_FAILURE; >+ Maybe<JSAutoCompartment> ac; >+ JSContext* cx; >+ if (NS_IsMainThread()) { >+ nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(GetOwner()); >+ if (NS_WARN_IF(!globalObject)) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ cx = jsapi.cx(); >+ ac.construct(cx, globalObject->GetGlobalJSObject()); >+ } else { >+ MOZ_ASSERT(impl->mWorkerPrivate); >+ cx = impl->mWorkerPrivate->GetJSContext(); >+ >+ WorkerGlobalScope* globalScope = impl->mWorkerPrivate->GlobalScope(); >+ MOZ_ASSERT(globalScope); >+ >+ JS::Rooted<JSObject*> jsGlobal(cx, globalScope->GetWrapper()); >+ MOZ_ASSERT(jsGlobal); >+ >+ ac.construct(cx, jsGlobal); > } So you don't Init AutoJSAPI at all? AutoJSAPI::Init would enter to a compartment, AFAIK >+ if (mParent) { >+ nsIScriptContext* sc; >+ if (!mWorkerPrivate) { >+ nsresult rv; >+ sc = mParent->GetContextForEventHandlers(&rv); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } >+ } else { >+ nsresult rv = mParent->CheckInnerWindowCorrectness(); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } I don't understand this check. CheckInnerWindowCorrectness() call in, possibly, worker thread? Why would mParent (which btw should be perhaps renamed to mWebSocket or some such) have a window as owner here? What guarantees WebSocketImpl::GetLoadGroup is called only on the main thread? This certainly needs MOZ_ASSERTs > private: > WebSocket(const WebSocket& x) MOZ_DELETE; // prevent bad usage > WebSocket& operator=(const WebSocket& x) MOZ_DELETE; > > // Raw pointer because this WebSocketImpl is created, managed an destroyed by > // WebSocket. This is from the previous patch, but s/an/and/ >+ // Note: This should not be used directly. Use GetImpl() instead. > WebSocketImpl* mImpl; >+ mozilla::Mutex mMutex; I don't understand how this setup is enough. You just protect mImpl, but then for example WebSocket::GetReadyState happily uses impl->mReadyState; (via ReadState()) and I don't see what guarantees non-racy mReadyState reading. I believe mutex needs to be used in almost all the cases when ws reads some state from wsimpl. > > bool mKeepingAlive; > bool mCheckMustKeepAlive; > }; > > } //namespace dom > } //namespace mozilla > >@@ -31,24 +13,24 @@ > * 10. client sends a message before the ws connection is established; > * 11. a simple hello echo; > * 12. client sends a message containing unpaired surrogates > * 13. server sends an invalid message; > * 14. server sends the close frame, it doesn't close the tcp connection and > * it keeps sending normal ws messages; > * 15. server closes the tcp connection, but it doesn't send the close frame; > * 16. client calls close() and tries to send a message; >- * 17. see bug 572975 - all event listeners set >+ * 17. see bug 572975 - all event listeners set - REMOVED (requires forcegc) > * 18. client tries to connect to an http resource; > * 19. server closes the tcp connection before establishing the ws connection; >- * 20. see bug 572975 - only on error and onclose event listeners set >+ * 20. see bug 572975 - only on error and onclose event listeners set - REMOVED (requires forcegc) > * 21. see bug 572975 - same as test 17, but delete strong event listeners when >- * receiving the message event; >- * 22. server takes too long to establish the ws connection; >- * 23. should detect WebSocket on window object; >+ * receiving the message event - REMOVED (requires forcegc and window); >+ * 22. server takes too long to establish the ws connection - REMOVED (requires SpecialPower object); >+ * 23. should detect WebSocket on window object - REMOVED (requires window object); > * 24. server rejects sub-protocol string > * 25. ctor with valid empty sub-protocol array > * 26. ctor with invalid sub-protocol array containing 1 empty element > * 27. ctor with invalid sub-protocol array containing an empty element in list > * 28. ctor using valid 1 element sub-protocol array > * 29. ctor using all valid 5 element sub-protocol array > * 30. ctor using valid 1 element sub-protocol array with element server will > * reject >@@ -61,34 +43,49 @@ > * 36. negative test for sending out of range close code > * 37. negative test for too long of a close reason > * 38. ensure extensions attribute is defined > * 39. a basic wss:// connectivity test > * 40. negative test for wss:// with no cert > * 41. HSTS > * 42. non-char utf-8 sequences > * 43. Test setting binaryType attribute >- * 44. Test sending/receving binary ArrayBuffer >- * 45. Test sending/receving binary Blob >+ * 44. Test sending/receving binary ArrayBuffer >+ * 45. Test sending/receving binary Blob - REMOVED (requires SpecialPower object) perhaps change "REMOVED" to "something like: Only in the main thread worker tests (requires SpecialPower object)" Same thing few times.

Doesn't XHR send readyState changes from main thread to the worker etc, so no locking is needed so much.

One thing to remember, https://bugzilla.mozilla.org/show_bug.cgi?id=1031943

a possible workaround is to use socket.io on websocket server side and import the websocket client library using Import Scripts, for example this code works in worker script: importScripts('//njs.mydomain.tld/socket.io/socket.io.js'); socket = io.connect('njs.mydomain.tld'); socket.on('connect',function(){ console.log("CONNECTED SUCCESSFULLY"); });

Importing a client side library doesn't just make Web Sockets work in Web Worker threads. Socket.io is probably falling back to XHR, which is available to workers.

Any update on this issue ?

:baku anything new on this? or did you pause your work on it since June?

Guys, we are working on this, but Andrea and others are busy working on more important things at the moment. Please be patient.

I don't remember how the previous patch worked and it has been easier to rewrite it. I hope the review process will be easier too.

https://tbpl.mozilla.org/?tree=Try&rev=371a55286c3e green on try.

a small change to make e10s happy.

Comment on attachment 8494252 [details] [diff] [review] patch 2 - WebSocket in workers Would be nice to split the tests out from this patch. > >+ mozilla::Mutex mMutex; Please document for what all cases this mutex is used. > WebSocket::WrapObject(JSContext* cx) > { >+ MOZ_ASSERT(mImpl); >+ >+ if (mImpl->mWorkerPrivate) { >+ return WebSocketBinding_workers::Wrap(cx, this); >+ } >+ > return WebSocketBinding::Wrap(cx, this); > } Please add a comment why different binding (because of send() ?) >+// This class is used to clear any exception. >+class ClearException MOZ_FINAL MOZ_STACK_CLASS ? >+private: >+ // Raw pointer. This worker runs synchronously. >+ WebSocketImpl*mImpl; missing space before mImpl; >+ class ClearWebSocket MOZ_FINAL MOZ_STACK_CLASS > WebSocketImpl::Init(JSContext* aCx, > nsIPrincipal* aPrincipal, > const nsAString& aURL, >- nsTArray<nsString>& aProtocolArray) >+ nsTArray<nsString>& aProtocolArray, >+ ErrorResult& aRv, >+ bool* aConnectionFailed) > { >- NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); >+ AssertIsOnMainThread(); > MOZ_ASSERT(aPrincipal); > > // We need to keep the implementation alive in case the init disconnects it > // because of some error. > nsRefPtr<WebSocketImpl> kungfuDeathGrip = this; > > mPrincipal = aPrincipal; > > // Attempt to kill "ghost" websocket: but usually too early for check to fail >- nsresult rv = mParent->CheckInnerWindowCorrectness(); >- NS_ENSURE_SUCCESS(rv, rv); >+ aRv = mWebSocket->CheckInnerWindowCorrectness(); >+ if (NS_WARN_IF(aRv.Failed())) { >+ return; >+ } > > // Shut down websocket if window is frozen or destroyed (only needed for > // "ghost" websockets--see bug 696085) >- nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); >- NS_ENSURE_STATE(os); >- rv = os->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, true); >- NS_ENSURE_SUCCESS(rv, rv); >- rv = os->AddObserver(this, DOM_WINDOW_FROZEN_TOPIC, true); >- NS_ENSURE_SUCCESS(rv, rv); >+ if (!mWorkerPrivate) { >+ nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); >+ if (NS_WARN_IF(!os)) { >+ aRv.Throw(NS_ERROR_FAILURE); >+ return; >+ } >+ >+ aRv = os->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, true); >+ if (NS_WARN_IF(aRv.Failed())) { >+ return; >+ } >+ >+ aRv = os->AddObserver(this, DOM_WINDOW_FROZEN_TOPIC, true); >+ if (NS_WARN_IF(aRv.Failed())) { >+ return; >+ } >+ } > > unsigned lineno; > JS::AutoFilename file; > if (JS::DescribeScriptedCaller(aCx, &file, &lineno)) { > mScriptFile = file.get(); > mScriptLine = lineno; > } This part doesn't really make sense when we're creating WS for workers. We should get the mScriptFile/Line on worker side and then pass to the runnable and the runnable gives them to the WSImpl. > >+ nsCOMPtr<nsIURI> uri; >+ { >+ nsresult rv = NS_NewURI(getter_AddRefs(uri), mURI); >+ if (NS_FAILED(rv)) { >+ MOZ_CRASH(); >+ } Could you add a comment here that this means either that NS_NewURI can't create an uri from nsIURI::GetSpec or that we're OOM'ing > WebSocket::CreateAndDispatchMessageEvent(const nsACString& aData, >- bool isBinary) >+ bool aIsBinary) > { > MOZ_ASSERT(mImpl); >- NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); >- >- nsresult rv = CheckInnerWindowCorrectness(); >- if (NS_FAILED(rv)) >- return NS_OK; >- >- AutoJSAPI jsapi; >- if (NS_WARN_IF(!jsapi.Init(GetOwner()))) { >- return NS_ERROR_FAILURE; >+ mImpl->AssertIsOnTargetThread(); >+ >+ if (NS_IsMainThread()) { >+ AutoJSAPI jsapi; >+ if (NS_WARN_IF(!jsapi.Init(GetOwner()))) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ return CreateAndDispatchMessageEvent(jsapi.cx(), aData, aIsBinary); > } >- JSContext* cx = jsapi.cx(); >+ >+ MOZ_ASSERT(mWorkerPrivate); >+ return CreateAndDispatchMessageEvent(mWorkerPrivate->GetJSContext(), aData, >+ aIsBinary); Why you don't use AutoJSAPI on worker? I'd really prefer consistent code paths especially in case of JSAPI usage. > if (mImpl->mBinaryType == dom::BinaryType::Blob) { >- rv = nsContentUtils::CreateBlobBuffer(cx, aData, &jsData); >- NS_ENSURE_SUCCESS(rv, rv); >+ nsresult rv = nsContentUtils::CreateBlobBuffer(aCx, aData, &jsData); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } No need for this change. NS_ENSURE_SUCCESS is just fine ;) > WebSocket::CreateAndDispatchCloseEvent(bool aWasClean, > uint16_t aCode, > const nsAString &aReason) > { >- NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); >- >- nsresult rv = CheckInnerWindowCorrectness(); >- if (NS_FAILED(rv)) { >- return NS_OK; >+ MOZ_ASSERT(mImpl); >+ mImpl->AssertIsOnTargetThread(); >+ >+ if (NS_IsMainThread()) { >+ nsresult rv = CheckInnerWindowCorrectness(); >+ if (NS_FAILED(rv)) { >+ return NS_OK; >+ } CheckInnerWindowCorrectness() is actually thread safe since mOwnerWindow should be always null in the worker threads. Same also elsewhere. >+{ >+ 'nativeType': 'mozilla::dom::WebSocket', >+ 'headerFile': 'mozilla/dom/WebSocket.h', >+ 'workers': True, >+}], Please add a comment why we need this (because of the send()) Why does this look so simple :) Could still take another look and tests in a different patch, please.

Can we at lest submit those tests to wpt?

> >- AutoJSAPI jsapi; > >- if (NS_WARN_IF(!jsapi.Init(GetOwner()))) { > >- return NS_ERROR_FAILURE; > >+ mImpl->AssertIsOnTargetThread(); > >+ > >+ if (NS_IsMainThread()) { > >+ AutoJSAPI jsapi; > >+ if (NS_WARN_IF(!jsapi.Init(GetOwner()))) { > >+ return NS_ERROR_FAILURE; > >+ } > >+ > >+ return CreateAndDispatchMessageEvent(jsapi.cx(), aData, aIsBinary); > > } > >- JSContext* cx = jsapi.cx(); > >+ > >+ MOZ_ASSERT(mWorkerPrivate); > >+ return CreateAndDispatchMessageEvent(mWorkerPrivate->GetJSContext(), aData, > >+ aIsBinary); > Why you don't use AutoJSAPI on worker? I'd really prefer consistent code > paths especially in case of JSAPI usage. We don't need to use AutoJSAPI in workers becuase we already have the JSContext. All the other comments are applied. New 2 patches are coming.

(In reply to Andrea Marchesini (:baku) from comment #121) > We don't need to use AutoJSAPI in workers becuase we already have the > JSContext. I'd really like to hear bholley's opinion on this. Technically we may not need it right now, but AFAIK the plan is that one must have AutoJSAPI (or some variant of it) on stack whenever Gecko uses JSAPI. http://mxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h?rev=ed7e4dde009b#166

Comment on attachment 8499398 [details] [diff] [review] patch 3 - tests >+ ws.onerror = function(e) { >+ postMessage({type: 'status', status: false, msg: 'onrrror called!'}); rather many rrr >@@ -31,24 +13,24 @@ > * 10. client sends a message before the ws connection is established; > * 11. a simple hello echo; > * 12. client sends a message containing unpaired surrogates > * 13. server sends an invalid message; > * 14. server sends the close frame, it doesn't close the tcp connection and > * it keeps sending normal ws messages; > * 15. server closes the tcp connection, but it doesn't send the close frame; > * 16. client calls close() and tries to send a message; >- * 17. see bug 572975 - all event listeners set >+ * 17. see bug 572975 - all event listeners set - Only in the main thread tests (requires forcegc) Could you make this working by sending message to main thread, do memory-pressure, and then continue? >- * 44. Test sending/receving binary ArrayBuffer >- * 45. Test sending/receving binary Blob >+ * 44. Test sending/receving binary ArrayBuffer >+ * 45. Test sending/receving binary Blob - Only in the main thread worker tests (requires SpecialPower object) not your comment, but receiving.

This patch is going to land after DOMBlob/DOMFile ported to webidl, so we don't need a custom WebSocket::Send(). Here the same patch rebased on top of bug 1047483.

Comment on attachment 8500973 [details] [diff] [review] patch 2 - WebSocket in workers CallDispatchConnectionCloseEvents::Run has wrong assert (asserts main thread, not current thread), and since the tests didn't catch that, we certainly should add some test. That might reveal some other issues, so I'd prefer to re-review after that.

So, yeah. we don't have any test for this runnable. I'm not sure how to create such test because I have to interrupt the communication from the server in order to generate a 'onStop' calls. I need a feedback from jduell or somebody else from necko team.

Comment on attachment 8501081 [details] [diff] [review] patch 2 - WebSocket in workers > WebSocketImpl::Observe(nsISupports* aSubject, > const char* aTopic, > const char16_t* aData) > { >+ AssertIsOnTargetThread(); Shouldn't this check MainThread, not TargetThread? >+ > if ((mReadyState == WebSocket::CLOSING) || > (mReadyState == WebSocket::CLOSED)) { > return NS_OK; > } > > nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aSubject); >- if (!mParent->GetOwner() || window != mParent->GetOwner()) { >+ if (!mWebSocket->GetOwner() || window != mWebSocket->GetOwner()) { > return NS_OK; > } > > if ((strcmp(aTopic, DOM_WINDOW_FROZEN_TOPIC) == 0) || > (strcmp(aTopic, DOM_WINDOW_DESTROYED_TOPIC) == 0)) > { > CloseConnection(nsIWebSocketChannel::CLOSE_GOING_AWAY); > } So what happens to Websocket connections in dedicated workers when the page goes to bfcache? > WebSocketImpl::GetLoadGroup(nsILoadGroup** aLoadGroup) > { >+ AssertIsOnMainThread(); >+ > *aLoadGroup = nullptr; > > nsresult rv; >- nsIScriptContext* sc = mParent->GetContextForEventHandlers(&rv); >+ nsIScriptContext* sc = mWebSocket->GetContextForEventHandlers(&rv); > nsCOMPtr<nsIDocument> doc = > nsContentUtils::GetDocumentFromScriptContext(sc); > > if (doc) { > *aLoadGroup = doc->GetDocumentLoadGroup().take(); > } > > return NS_OK; > } So in workers we have always null loadgroup? At least in dedicated worker we should get load group from the parent window I think. + >+class DispatchEvent MOZ_FINAL : public WorkerRunnable DispatchEvent is a bit too commonly use name, especially because EventTarget DOM interface has dispatchEvent method. Rename to something else.... maybe WorkerRunnableDispatcher

The bfcache in workers and the loadGroup in workers are not tested. We have to find a way to write a mochitest for these 2 issues if we care.

I added a test for the loadGroup issue.

Comment on attachment 8501871 [details] [diff] [review] patch 3 - tests >+ worker.onmessage = function(e) { >+ if (e.data == 'opened') { >+ stopped = true; >+ window.stop(); >+ } else if (e.data == 'closed') { >+ ok(stopped, "Good!"); >+ runTest(); Shouldn't you set stopped back to false before calling runTests();

Comment on attachment 8501870 [details] [diff] [review] patch 2 - WebSocket in workers > WebSocketChannel::BeginOpen() > { >+ if (!NS_IsMainThread()) { >+ NS_DispatchToMainThread( >+ NS_NewRunnableMethod(this, &WebSocketChannel::BeginOpen), >+ NS_DISPATCH_NORMAL); Odd indentation for NS_DISPATCH_NORMAL. Could be under 'this' r+ if AutoJSAPI is used also in workers, or bholley says one shouldn't use it in workers. (AutoJSAPI changes could be done on top of this patch to ease reviewing.)

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=deffa2b8e003 green on try.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e72ed2e97e83 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7950dac9a28 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f23cea600bd0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8edaa32e2278 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e289b886cccb

Release Note Request (optional, but appreciated) [Why is this notable]: WebSocket in workers is a nice feature that is extremely useful for games and complex webapps.

https://hg.mozilla.org/mozilla-central/rev/e72ed2e97e83 https://hg.mozilla.org/mozilla-central/rev/e7950dac9a28 https://hg.mozilla.org/mozilla-central/rev/f23cea600bd0 https://hg.mozilla.org/mozilla-central/rev/8edaa32e2278 https://hg.mozilla.org/mozilla-central/rev/e289b886cccb

Awesome! Thanks Andrea! This is quite exciting to have!

Finally! Thank you so much for implementing this Andrea and thank you Olli for reviewing it. I'm so happy: finally games like Runescape, that need websockets in webworkers, can run in the browser natively without Java. Gonna test the nightly now :)

++good

(In reply to Olli Pettay [:smaug] from comment #124) > I'd really like to hear bholley's opinion on this. > Technically we may not need it right now, but AFAIK the plan is that one > must have AutoJSAPI (or some variant of it) Eventually AutoJSAPI will be the only way to get a JSContext, so a pre-existing JSContext will mean we already have one. But workers haven't been fully fixed yet - see bug 1072144.

Don't quite understand that comment. Of course there is the pre-existing JSContext in workers, that is what WorkerPrivate::GetJSContext() gives you. But should we not use that, but just AutoJSAPI + WorkerPrivate::GlobalScope() ?

(In reply to Olli Pettay [:smaug] from comment #144) > Don't quite understand that comment. > Of course there is the pre-existing JSContext in workers, that is what > WorkerPrivate::GetJSContext() > gives you. But should we not use that, but just AutoJSAPI + > WorkerPrivate::GlobalScope() ? If the caller is passing a JSContext, we'll get an AutoJSAPI there when do we bug 1072144. But if this is shared code and the mainthread codepath is using AutoJSAPI, I agree that we should be consistent for both.

Added to the release notes with the wording "WebSocket available in Workers"

I just tried web socket in a shared web worker in the latest Firefox Nightly(v36), and I got a `NS_ERROR_FAILURE` error. Please note that the name resolves perfectly fine everywhere else, even within Firefox. The line it threw the error had this: instance = new WebSocket(uri); where value of uri was "wss://mydomain.tld/live/?EIO=3&transport=websocket&sid=F0uG77boysE-pvCnAAAc"

(In reply to aneesiqbalbhatti from comment #147) > I just tried web socket in a shared web worker in the latest Firefox > Nightly(v36), and I got a `NS_ERROR_FAILURE` error. Could you report a new bug? Thanks

And assign that bug to me. Thanks.

Here's the new bug https://bugzilla.mozilla.org/show_bug.cgi?id=1090183

We decided to disable this feature by pref for ff35.

The docs team will proceed under the assumption, for now, that this will ship preffed on in Firefox 36. I've already added mention of this feature to the main WebSocket API page's compatibility table: https://developer.mozilla.org/en-US/docs/WebSockets#Browser_compatibility The documentation changes that remain to be made are: * Update Firefox X for developers and Firefox 36 for developers if this slides out further than Firefox 36. * There appear to be no API syntax changes: no new or changed interfaces, methods, or properties. * Reference pages need to be marked that the API can be used in workers. * Guides and tutorial documentation need to be updated to include examples of and discussion of using WebSocket in a worker. I'm not including URLs here because this documentation work will be handled as part of the overall WebSocket doc revamp, which includes moving the content we already have to a new location. See bug 1115091 for that work.

(In reply to Eric Shepherd [:sheppy] from comment #153) > The docs team will proceed under the assumption, for now, that this will > ship preffed on in Firefox 36. That's the plan :)

relnoted as "WebSocket available in Workers"

Why did we put this for release notes for 37 rather than 36?

Because this certainly won't be in 36.

Thanks for the clarification.

I've moved the note from: https://developer.mozilla.org/en-US/Firefox/Releases/36 to https://developer.mozilla.org/en-US/Firefox/Releases/37 too.

People should really set the "disabled" state for branches when we disable there...

Olli pointed out to me that this feature has been disabled in 37. (bug 1121406#c8) I have updated the relnote to 38. Jean-Yves - We should push this feature to the 38 dev notes.

Clearing ni/. The Fx 38/37 for devs page have been updated long ago.