If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Support off-main-thread onmessage/recv from WebSocketChannel (WebSockets for workers)

RESOLVED FIXED in mozilla31

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: jduell, Assigned: sworkman)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

5.54 KB, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
32.95 KB, patch
jduell
: review+
Details | Diff | Splinter Review
16.07 KB, patch
jduell
: review+
Details | Diff | Splinter Review
Websockets for workers needs this.  See bug 504553 comment 42 and onwards.

Comment 1

4 years ago
Is anyone actively working on this?

Comment 2

4 years ago
Since this bug hasn't been active for a few weeks.  I'm looking for status on the progress.
(Assignee)

Comment 3

4 years ago
Either Jason or I hope to get to it before the end of the quarter, as soon as we get some free cycles.

Comment 4

4 years ago
ping.
This would be needed for Talkilla https://wiki.mozilla.org/Talkilla

We're currently relying on long polling to avoid relying on websockets but this is really hard to scale.

Marking it as a needinfo for :jduell & :sworkman since they seem likely to take care of it.
Flags: needinfo?(sworkman)
Flags: needinfo?(jduell.mcbugs)
(Reporter)

Comment 6

4 years ago
Steve and and I came up with the threading/IPDL architecture for this, and he's going to code it up soon (this quarter).  So yes, it's coming soon.
Assignee: jduell.mcbugs → sworkman
Flags: needinfo?(sworkman)
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Comment 7

4 years ago
Alexis, just to be 100% clear, this bug is to support onmessage/send from the perspective of WebSocketChannel. The work in 504553 is still needed to provide WebSocket support off main thread. AFAIK, :baku will be working on that.
Summary: Support off-main send/recv from Websockets (for workers) → Support off-main-thread onmessage/recv from WebSocketChannel (WebSockets for workers)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
That's good news, thanks for the comments Steve.

I'll ping :baku to know how much work is needed for bug 504553 to resolve.
(Assignee)

Comment 9

4 years ago
Created attachment 8370394 [details] [diff] [review]
WIP 0.1 Support delivery of WebSocket events off-main-thread in WebSocketChannel

Hi :baku - here is a WIP patch for WebSocketChannel OMT. This is for the single process case, but the concept should be the same for e10s: Before WebSocket calls AsyncOpen on the WebSocketChannel, it should call RetargetDeliveryTo and supply a target thread. Up to this point, calls will have to be made on the main thread, but after that point any time the channel calls a WebSocket function, it should be able to call it on the worker thread. Similarly, it should be able to receive WebSocketChannel calls on the worker thread.

Like I said, it's a WIP, but without changes from WebSocket, it behaves just as it usually does and passes the web socket mochitests. So, I think we're on the right track.

Jason, I had mentioned to you that we could use AsyncWait to retarget delivery. I'm not sure that we need to, however, as the callbacks are happening on the socket thread. The important part is to move subsequent dispatches, or previous calls off the main thread. This patch should do that.

I've added one StaticMutex for sWebSocketAdmissions, and it's possible we may need another one or two when we do more testing. Similarly, not all the variables have been tested for main-thread-only compliance, but again, we can do that as we go, e.g. mURI should not be used off main thread in SendMsgCommon.

Let me know your thoughts.
Attachment #8370394 - Flags: feedback?(jduell.mcbugs)
Attachment #8370394 - Flags: feedback?(amarchesini)
Created attachment 8370399 [details] [diff] [review]
WIP 0.1 Support delivery of WebSocket events off-main-thread in WebSocketChannel (e10s)

Here's how it might look in e10s. I may open a separate bug for that since there's a lot more plumbing to move PWebSocket from PNecko to PNeckoBackground. But for now it's useful to have it here so you can see it.
(In reply to Steve Workman [:sworkman] from comment #10)
> Created attachment 8370399 [details] [diff] [review]
> WIP 0.1 Support delivery of WebSocket events off-main-thread in
> WebSocketChannel (e10s)
> 
> Here's how it might look in e10s. I may open a separate bug for that since
> there's a lot more plumbing to move PWebSocket from PNecko to
> PNeckoBackground. But for now it's useful to have it here so you can see it.

Oh, and this one doesn't even build. So, really just use it as an idea for how we're proposing to do this.
Created attachment 8394417 [details] [diff] [review]
v1.0 Support delivery of WebSocket events off-main-thread in WebSocketChannel (non-e10s)
Attachment #8370394 - Attachment is obsolete: true
Attachment #8370394 - Flags: feedback?(jduell.mcbugs)
Attachment #8370394 - Flags: feedback?(amarchesini)
Attachment #8394417 - Flags: review?(jduell.mcbugs)
Created attachment 8394421 [details] [diff] [review]
v1.0 Support delivery of WebSocket events off-main-thread in WebSocketChannel (e10s)

Foregoing use of PBackground for now since there is some uncertainty about implementation details. Instead, this patch proxies events between the main thread and target thread in the child process. As with the non-e10s version, the target thread should be set by the WebSocket class (WebSocketChannel client) before AsyncOpen is called.

-- Patch builds locally.
-- Passing WebSocket mochitests locally (Note: just these patches applied; no retargeting requested by WebSocket at this stage).
-- Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=30e906ad8b5a
Attachment #8370399 - Attachment is obsolete: true
Attachment #8394421 - Flags: review?(jduell.mcbugs)
Created attachment 8394986 [details] [diff] [review]
v1.1 Support delivery of WebSocket events off-main-thread in WebSocketChannel (non-e10s)

Updated to fix try failure: minor QueryInterface fix.
Attachment #8394417 - Attachment is obsolete: true
Attachment #8394417 - Flags: review?(jduell.mcbugs)
Attachment #8394986 - Flags: review?(jduell.mcbugs)
Created attachment 8394987 [details] [diff] [review]
v1.1 Support delivery of WebSocket events off-main-thread in WebSocketChannel (e10s)

Removed whitespace.

New try job, more comprehensive run: https://tbpl.mozilla.org/?tree=Try&rev=333aa3daa76e
Attachment #8394421 - Attachment is obsolete: true
Attachment #8394421 - Flags: review?(jduell.mcbugs)
Attachment #8394987 - Flags: review?(jduell.mcbugs)
(Reporter)

Comment 16

4 years ago
Comment on attachment 8394986 [details] [diff] [review]
v1.1 Support delivery of WebSocket events off-main-thread in WebSocketChannel (non-e10s)

Review of attachment 8394986 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good.  Still failing on try, but I'm guessing that's something minor.

::: content/base/src/WebSocket.cpp
@@ +806,5 @@
> +  nsCOMPtr<nsIThreadRetargetableRequest> retargetableChannel =
> +    do_QueryInterface(wsChannel);
> +  MOZ_ASSERT(retargetableChannel);
> +  nsCOMPtr<nsIThread> thread = do_GetMainThread();
> +  rv = retargetableChannel->RetargetDeliveryTo(thread);

What's the point of retargeting to the main thread when that's the default?  Is this just a test of your codepath?

::: netwerk/protocol/websocket/BaseWebSocketChannel.cpp
@@ +251,5 @@
> +BaseWebSocketChannel::RetargetDeliveryTo(nsIEventTarget* aTargetThread)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aTargetThread);
> +  MOZ_ASSERT(!mTargetThread, "Delivery target should be set once, before AsyncOpen");

worth adding a mAsyncOpened bool so we can fail if called after AsyncOpen.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +70,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +NS_IMPL_ADDREF(WebSocketChannel)

Go ahead and add NS_IMPL_ISUPPORTS13 to nsISupportsImpl.h and ask bsmedberg to review it.

@@ +1003,5 @@
> +    StaticMutexAutoLock lock(sWebSocketAdmissionsLock);
> +    if (!sWebSocketAdmissions) {
> +      sWebSocketAdmissions = new nsWSAdmissionManager();
> +    }
> +  }

Ugly hack about websockets: unlike most protocols, where we get a channel from a URI we pass to nsIIOService (which then figures out which nsIProtocolHandler to ask to create the channel), we just create websocket channels directly.  But for reasons I forget, we still need to have a nsIProtocolHandler for websockets (probably to create ws:// URIs?).  Instead of writing a separate WebsocketHandler class for that, we made *all* websocketchannels handlers (see BaseWebsocketChannel.h) and we wind up instantiating one copy as the handler:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetModule.cpp#302

It's gross--we should really split out a separate WebSocketHandler class some time (that time could be now if it makes sense).  Meanwhile, set a breakpoint in gdb and see if the handler gets created early on somehow in a way that will let us reliably know that we can create the sWebSocketAdmissions without holding a lock.  

This is because I'd much prefer to have the admissions manager contain a Mutex and arrange for locking within its calls than force all client calls to first get the lock and then call into it.  It's better encapsulation.

@@ +1079,1 @@
>    delete sWebSocketAdmissions;

Ditto for this.  Shutdown is getting called via

   http://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetModule.cpp#680

I suspect that this gets called late enough that we don't need to lock to delete, but I'm not sure.  Maybe find out how hHttpHandler gets destroyed and compare.

@@ +2752,5 @@
> +        sWebSocketAdmissions->SessionCount() >= mMaxConcurrentConnections)
> +    {
> +      LOG(("WebSocketChannel: max concurrency %d exceeded (%d)",
> +           mMaxConcurrentConnections,
> +           sWebSocketAdmissions->SessionCount()));

Once you've got locking within the admission manager, just do one call to SessionCount and keep it in a variable for the 3 uses.

@@ +2920,5 @@
>    }
>  
>    nsresult rv;
>    if (mConnectionLogService && !mPrivateBrowsing) {
> +    // Fix this! No URI access off main thread.

just get the host as a string when we create the websocket (which is still always on main thread?  Or at least we can arrange for that part to be on main thread) and store it as a member variable.
Attachment #8394986 - Flags: review?(jduell.mcbugs) → feedback+
(Reporter)

Comment 17

4 years ago
Comment on attachment 8394987 [details] [diff] [review]
v1.1 Support delivery of WebSocket events off-main-thread in WebSocketChannel (e10s)

Review of attachment 8394987 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  A couple nits but they could be followups.

::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ +363,5 @@
>                                   nsISupports *aContext)
>  {
>    LOG(("WebSocketChannelChild::AsyncOpen() %p\n", this));
>  
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "not main thread");

If we want to be "nice" we could also dispatch this method to the main thread if it's called off-main.  That would be a nice-to-have, but not if it gets complex (should be simple?)

@@ +519,5 @@
> +    return NS_OK;
> +  }
> +private:
> +  nsRefPtr<WebSocketChannelChild> mChild;
> +  OptionalInputStreamParams       mStream;

Do we know if copy by value into mStream does a memcpy of the data?  If so would be more efficient to alloc on the heap and only have one copy.  Could be followup.
Attachment #8394987 - Flags: review?(jduell.mcbugs) → review+
Created attachment 8396826 [details] [diff] [review]
v1.0 Implement NS_IMPL_ISUPPORTS13
Attachment #8396826 - Flags: review?(benjamin)
Created attachment 8396829 [details] [diff] [review]
v1.2 Support delivery of WebSocket events off-main-thread in WebSocketChannel (non-e10s)

-- Removed the WebSocket chunks from the patch - they were included by accident.
-- Rather than add mAsyncOpened I used the existing mWasOpened.
-- Made nsWSAdmissionsManager a static class containing a private singleton sManager. This encapsulates all the mutex un/locking and keeps WebSocketChannel a bit cleaner. I didn't look into when it's actually initialized or deleted, since I though this solution should cover both of those cases. Let me know what you think.
-- Using mHost in order to use the URI on multiple threads.

Also, new try job (for all patches): https://tbpl.mozilla.org/?tree=Try&rev=0ab30180b76a
Attachment #8394986 - Attachment is obsolete: true
Attachment #8396829 - Flags: review?(jduell.mcbugs)
Created attachment 8396833 [details] [diff] [review]
v1.2 Support delivery of WebSocket events off-main-thread in WebSocketChannel (e10s)

-- Dispatching AsyncOpen off main thread: For now I'd like to leave this. The function dispatches off the main thread to do Proxy or DNS resolution anyway, so I'd rather leave it at that for now.
-- Copying the OptionalInputStreamParams: It looks like for the String version at least there is a copy. Rather than dig too deep I just converted it to a heap object.
Attachment #8394987 - Attachment is obsolete: true
Attachment #8396833 - Flags: review?(jduell.mcbugs)
(Reporter)

Updated

4 years ago
Attachment #8396829 - Flags: review?(jduell.mcbugs) → review+
(Reporter)

Comment 21

4 years ago
Comment on attachment 8396833 [details] [diff] [review]
v1.2 Support delivery of WebSocket events off-main-thread in WebSocketChannel (e10s)

Review of attachment 8396833 [details] [diff] [review]:
-----------------------------------------------------------------

OK.  If the folks who implement the DOM parts of websockets for workers want an AsyncOpen off-main thread, let's give it to them, but for now I'm ok with it being main-thread only.
Attachment #8396833 - Flags: review?(jduell.mcbugs) → review+
(In reply to Jason Duell (:jduell) from comment #21)
> OK.  If the folks who implement the DOM parts of websockets for workers want
> an AsyncOpen off-main thread, let's give it to them, but for now I'm ok with
> it being main-thread only.

Sounds good.

Thanks for the reviews!

Updated

4 years ago
Attachment #8396826 - Flags: review?(benjamin) → review+
Thanks for the reviews!

On inbound (folded WebSocketChannel patches into one; kept nsISupportsImpl patch separate):

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e0c591f7d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/272ead5346d3
(Assignee)

Updated

4 years ago
Blocks: 989070
https://hg.mozilla.org/mozilla-central/rev/f0e0c591f7d0
https://hg.mozilla.org/mozilla-central/rev/272ead5346d3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.