Last Comment Bug 664894 - Eliminate nsWebSocketEstablishedConnection and fold code into nsWebSocket
: Eliminate nsWebSocketEstablishedConnection and fold code into nsWebSocket
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Wellington Fernando de Macedo
:
Mentors:
Depends on:
Blocks: 695636
  Show dependency treegraph
 
Reported: 2011-06-16 17:24 PDT by Jason Duell [:jduell] (needinfo me)
Modified: 2011-10-21 02:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (40.46 KB, patch)
2011-08-03 20:08 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Splinter Review
websock.log (15.06 KB, text/plain)
2011-08-04 07:41 PDT, Wellington Fernando de Macedo
no flags Details
v2 (49.92 KB, patch)
2011-08-04 19:04 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Splinter Review
v3: rebased (53.26 KB, patch)
2011-10-13 02:40 PDT, Jason Duell [:jduell] (needinfo me)
no flags Details | Diff | Splinter Review
v4 (54.42 KB, patch)
2011-10-15 09:15 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Splinter Review
v5: unbitrot, add onerror handler keepalive checks (55.25 KB, patch)
2011-10-19 02:33 PDT, Jason Duell [:jduell] (needinfo me)
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2011-06-16 17:24:03 PDT
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] (way behind * queues, especially ni? queue) 2011-06-17 04:06:58 PDT
CC'ing the original author of all our websocket code :)
Comment 2 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-17 04:11:28 PDT
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] (way behind * queues, especially ni? queue) 2011-06-17 04:13:35 PDT
...so this could be just a leftover from Bug 640003.
Comment 4 Wellington Fernando de Macedo 2011-06-18 06:43:07 PDT
(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 [:jduell] (needinfo me) 2011-06-18 22:19:52 PDT
> 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 2011-06-19 06:12:33 PDT
(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 2011-07-31 11:34:48 PDT
Sorry, Patrick, are you already working on this?
Comment 8 Patrick McManus [:mcmanus] 2011-07-31 12:00:12 PDT
(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 2011-08-01 14:27:27 PDT
> 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 [:jduell] (needinfo me) 2011-08-03 16:53:47 PDT
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] 2011-08-03 17:14:27 PDT
jason, did you mean to make those changes to component and target milestone?
Comment 12 Wellington Fernando de Macedo 2011-08-03 20:08:14 PDT
Created attachment 550590 [details] [diff] [review]
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 2011-08-03 20:10:40 PDT
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] 2011-08-03 20:48:58 PDT
(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 [:jduell] (needinfo me) 2011-08-04 02:02:41 PDT
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] 2011-08-04 07:19:40 PDT
(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 2011-08-04 07:41:07 PDT
Created attachment 550689 [details]
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 2011-08-04 07:43:24 PDT
> 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 2011-08-04 19:04:59 PDT
Created attachment 550928 [details] [diff] [review]
v2

With python2.6 this patch works.
Comment 20 Patrick McManus [:mcmanus] 2011-08-05 06:16:50 PDT
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 2011-08-05 06:28:31 PDT
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] 2011-08-05 06:34:55 PDT
(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 [:jduell] (needinfo me) 2011-10-13 02:40:31 PDT
Created attachment 566770 [details] [diff] [review]
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 2011-10-13 16:32:03 PDT
> 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 2011-10-13 16:33:02 PDT
(...) * and give you an answer soon.
Comment 26 Jason Duell [:jduell] (needinfo me) 2011-10-13 23:37:34 PDT
>  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 2011-10-15 09:15:14 PDT
Created attachment 567286 [details] [diff] [review]
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 2011-10-15 09:27:51 PDT
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 [:jduell] (needinfo me) 2011-10-19 02:33:28 PDT
Created attachment 567989 [details] [diff] [review]
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 [:jduell] (needinfo me) 2011-10-20 01:32:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99d60858d86
Comment 31 Marco Bonardo [::mak] 2011-10-21 02:08:25 PDT
https://hg.mozilla.org/mozilla-central/rev/a99d60858d86

Note You need to log in before you can comment on or make changes to this bug.