664894 - Eliminate nsWebSocketEstablishedConnection and fold code into nsWebSocket

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Jason Duell]

AFAICT it doesn't seem like nsWebSocketEstablishedConnection really needs to exist.  nsWebSocket can be the nsIWebSocketListener, and can open the nsIWebSocketProtocol directly.   This would make the code simpler in lots of places, where it's become a bit of a haywire of duplicate checks and cross-references (ex: Close and UpdateMustKeepAlive).

Perhaps I'm missing some reason why there needs to be a separate class?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

CC'ing the original author of all our websocket code :)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

If I recall correctly, nsWebSocketEstablishedConnection was added because 
it needed to be threadsafe. nsWebSocket can not be threadsafe because it is
cycle collectable. 
But now that more of websocket's networking is handled by necko, there
may not be need for such threadsafe broker object.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

...so this could be just a leftover from Bug 640003.

Comment 4

[Wellington Fernando de Macedo]

(In reply to comment #2)
> If I recall correctly, nsWebSocketEstablishedConnection was added because 
> it needed to be threadsafe. nsWebSocket can not be threadsafe because it is
> cycle collectable. 
Yes, that was the reason.

Comment 5

[Jason Duell]

> nsWebSocketEstablishedConnection was added because 
> it needed to be threadsafe. nsWebSocket can not be threadsafe

OK, but there's nothing in nsWebSocketEstablishedConnection that requires any thread-safety any more, is there?  Seems like it's just a main-thread nsIWebSocketListener client at this point.

Comment 6

[Wellington Fernando de Macedo]

(In reply to comment #5)
> > nsWebSocketEstablishedConnection was added because 
> > it needed to be threadsafe. nsWebSocket can not be threadsafe
> 
> OK, but there's nothing in nsWebSocketEstablishedConnection that requires
> any thread-safety any more, is there?  Seems like it's just a main-thread
> nsIWebSocketListener client at this point.

Yes, nsWebSocketEstablishedConnection can be eliminated.

Comment 7

[Wellington Fernando de Macedo]

Sorry, Patrick, are you already working on this?

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to comment #7)
> Sorry, Patrick, are you already working on this?

nope, I heard jason talking about it - but I don't know if he had started anything.

Comment 9

[Wellington Fernando de Macedo]

> nope, I heard jason talking about it - but I don't know if he had started 
> anything.
Jason, I intend to take this bug if nobody is working on it. Have you started fixing anything here?

Comment 10

[Jason Duell]

Go for it. 

If you want to split out a separate WebSocketProtocol class from WebSocketChannel (see bug 664928: it's a little odd to use a channel as a protocol handler), you get extra credit :)

Comment 11

[Patrick McManus [:mcmanus]]

jason, did you mean to make those changes to component and target milestone?

Comment 12

[Wellington Fernando de Macedo]

Attachment: v1

My trunk build (without my modifications) fails 15 of 33 tests of http://mochi.test:8888/tests/content/base/test/test_websocket.html. Is it supposed to be like that?

Well, my modifications in this uploaded patch also fails 15 of the 33 tests, so apparently it seems that I haven't introduced any issue.

Comment 13

[Wellington Fernando de Macedo]

Patrick and Jason, because this patch changes many lines of code (without changing its functions) that queue of bugs that you have sent me by email perhaps should be built upon this patch. What do you think?

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to comment #13)
> Patrick and Jason, because this patch changes many lines of code (without
> changing its functions) that queue of bugs that you have sent me by email
> perhaps should be built upon this patch. What do you think?

Most of that queue has already landed on mozilla-inbound..

Comment 15

[Jason Duell]

No idea how I managed to change all those component/milestone changes.  Weird.

> that queue of bugs that you have sent me by email
> perhaps should be built upon this patch.

Since this bug is not in the critical path to getting full (possibly unprefixed) WS functionality, I'm inclined to land it *after* all the other patches Patrick has been working on.  I hope that doesn't mean too much extra work for you.

> My trunk build (without my modifications) fails 15 of 33 tests of 
> http://mochi.test:8888/tests/content/base/test/test_websocket.html. 

Um, that sounds wrong.  Patrick, any idea?

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to comment #15)

> 
> > My trunk build (without my modifications) fails 15 of 33 tests of 
> > http://mochi.test:8888/tests/content/base/test/test_websocket.html. 
> 
> Um, that sounds wrong.  Patrick, any idea?

It does sound wrong.. I seem to remember at some point that python2.5 needed to be used to make pywebsockets work, but it looks like I am using 2.7 in the objdir I use all the time.

Comment 17

[Wellington Fernando de Macedo]

Attachment: websock.log

> It does sound wrong.. I seem to remember at some point that python2.5 needed to 
> be used to make pywebsockets work, but it looks like I am using 2.7 in the 
> objdir I use all the time.
I use python2.7. Attached is mu wesock.log. I think it is related to:
http://code.google.com/p/pywebsocket/issues/detail?id=101

Comment 18

[Wellington Fernando de Macedo]

> Since this bug is not in the critical path to getting full (possibly 
> unprefixed) WS functionality, I'm inclined to land it *after* all the other 
> patches Patrick has been working on.  I hope that doesn't mean too much extra 
> work for you.
Ok. Patrick, please tell me when I can update this patch.

Comment 19

[Wellington Fernando de Macedo]

Attachment: v2

With python2.6 this patch works.

Comment 20

[Patrick McManus [:mcmanus]]

Hi Wellington,

I don't understand why there are all those changes to netwerk/websocketchannel when the scope of this bug is to consolidate the classes in content/*/nsWebsocket.cpp

Comment 21

[Wellington Fernando de Macedo]

Because those classes in netwerk/websocketchannel reference nsWebSocket objects, which aren't thread safe. So, without that changes you will get many assertions.

Comment 22

[Patrick McManus [:mcmanus]]

(In reply to Wellington Fernando de Macedo from comment #21)
> Because those classes in netwerk/websocketchannel reference nsWebSocket
> objects, which aren't thread safe. So, without that changes you will get
> many assertions.

because of the ref count manipulation - gotcha.

Comment 23

[Jason Duell]

Attachment: v3: rebased

Wellington,

I've rebased your patch and looked over it.  A couple of questions:

1) @@ nsWebSocket::Send

So you've changed a bunch of ENSURE_SUCCESS_AND_FAIL_IF_FAILED() calls to just NS_ENSURE_SUCCESS here (i.e the old nsWebSocketEstablishedConnection::PostMessage method you took the code from used ENSURE_SUCCESS_AND_FAIL_IF_FAILED).  Is there some reason we don't need to fail the connection any more?  It seems to me like we still need to.

> Because those classes in netwerk/websocketchannel reference nsWebSocket
> objects, which aren't thread safe. So, without that changes you will get
> many assertions.

I assume the reason we can't make nsWebSocket's refcounting thread-safe (like nsWebSocketEstablishedCxn) is because it's cycle collected? Or because it's a DOM class?  (it's probably better to avoid the overhead of thread-safe refcounting for nsWebSocket anyway, but I'm just curious).  

Other than the ENSURE_SUCCESS_AND_FAIL_IF_FAILED() issue, this looks ready to land (it passes our WS mochitests).  Holding off on +r till we resolve that.

Comment 24

[Wellington Fernando de Macedo]

> I assume the reason we can't make nsWebSocket's refcounting thread-safe (like 
> nsWebSocketEstablishedCxn) is because it's cycle collected? Or because it's a DOM 
> class?
Both of those reasons.

> So you've changed a bunch of ENSURE_SUCCESS_AND_FAIL_IF_FAILED() calls to just 
> NS_ENSURE_SUCCESS here (i.e the old 
> nsWebSocketEstablishedConnection::PostMessage method you took the code from 
> used ENSURE_SUCCESS_AND_FAIL_IF_FAILED).  Is there some reason we don't need 
> to fail the connection any more?  It seems to me like we still need to.
I don't remember right now the reason I changed that. I will take a look on it, make some tests and give you an answer seen.

Based on the ws code changes, there is another thing to take in account here. It is related to the many "keep alive" blocks. I suspect that the ws netwerk implementation is already keeping the ws objects alive. So I think those blocks can be removed. However I am not sure of that. Needs to talk to mcmanus and make some tests.

regards.

Comment 25

[Wellington Fernando de Macedo]

(...) * and give you an answer soon.

Comment 26

[Jason Duell]

>  I suspect that the ws netwerk implementation is already keeping the ws objects
> alive. So I think those blocks can be removed.

Make sure the solution works for both regular and e10s contexts.  But I'm inclined to do this work in a followup bug--this patch already changes plenty of stuff, and I'm planning to land binary messaging support on top of it very soon once it lands.

Thanks Wellington!

Comment 27

[Wellington Fernando de Macedo]

Attachment: v4

> So you've changed a bunch of ENSURE_SUCCESS_AND_FAIL_IF_FAILED() calls to 
> just NS_ENSURE_SUCCESS here. Is there some reason we don't need to fail the 
> connection any more?  It seems to me like we still need to.
Actually, send() was required to fail. I've fixed.

>>  I suspect that the ws netwerk implementation is already keeping the ws 
>> objects alive. So I think those blocks can be removed.
>(...) I'm inclined to do this work in a followup bug
Ok.

Comment 28

[Wellington Fernando de Macedo]

According to the spec an error event must be triggered when the connection *fails*. So, the test 9 was modified because of that (in that test the ws connection closes before being established => so it *fails*).

Comment 29

[Jason Duell]

Attachment: v5: unbitrot, add onerror handler keepalive checks

I'm running this through try before I land it.

We'll need to rework the onerror dispatch a little--right now if JS calls close() before the connection is established, AND the network winds up failing the connection independently, we will call onerror twice, which is contrary to the spec.  I'll fix that as part of bug 695636 

Thanks for the patch Wellington!

Comment 30

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a99d60858d86

Comment 31

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a99d60858d86