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

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Jason Duell]

Websockets for workers needs this.  See bug 504553 comment 42 and onwards.

Comment 1

[Martin Best (:mbest)]

Is anyone actively working on this?

Comment 2

[Martin Best (:mbest)]

Since this bug hasn't been active for a few weeks.  I'm looking for status on the progress.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

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

[Doug Turner (:dougt)]

ping.

Comment 5

[Alexis Metaireau (:alexis)]

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.

Comment 6

[Jason Duell]

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.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 8

[Alexis Metaireau (:alexis)]

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.

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: 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.

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: 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.

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

(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.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Support delivery of WebSocket events off-main-thread in WebSocketChannel (non-e10s)

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: 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

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Support delivery of WebSocket events off-main-thread in WebSocketChannel (non-e10s)

Updated to fix try failure: minor QueryInterface fix.

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: 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

Comment 16

[Jason Duell]

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.

Comment 17

[Jason Duell]

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.

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Implement NS_IMPL_ISUPPORTS13

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: 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

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: 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.

Comment 21

[Jason Duell]

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.

Comment 22

[Steve Workman [:sworkman] (INACTIVE)]

(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!

Comment 23

[Steve Workman [:sworkman] (INACTIVE)]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0e0c591f7d0
https://hg.mozilla.org/mozilla-central/rev/272ead5346d3