Eliminate nsWebSocketEstablishedConnection and fold code into nsWebSocket

RESOLVED FIXED in mozilla10

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jduell, Assigned: Wellington Fernando de Macedo)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 4 obsolete attachments)

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

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

Comment 2

6 years ago
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

6 years ago
...so this could be just a leftover from Bug 640003.
(Assignee)

Comment 4

6 years ago
(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.
(Reporter)

Comment 5

6 years ago
> 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.
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Updated

6 years ago
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED
(Assignee)

Comment 7

6 years ago
Sorry, Patrick, are you already working on this?
Assignee: wfernandom2004 → nobody
Status: ASSIGNED → NEW
(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.
(Assignee)

Comment 9

6 years ago
> 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?
(Reporter)

Comment 10

6 years ago
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 :)
Assignee: nobody → wfernandom2004
Component: Networking: WebSockets → Plug-ins
Target Milestone: --- → mozilla7
Version: unspecified → 8 Branch
jason, did you mean to make those changes to component and target milestone?
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 12

6 years ago
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.
Attachment #550590 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 13

6 years ago
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?
(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..
(Reporter)

Comment 15

6 years ago
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?
Assignee: wfernandom2004 → nobody
Component: Plug-ins → Networking: WebSockets
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: mozilla7 → ---
Version: 8 Branch → unspecified
(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.
(Assignee)

Comment 17

6 years ago
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
(Assignee)

Comment 18

6 years ago
> 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.
(Assignee)

Comment 19

6 years ago
Created attachment 550928 [details] [diff] [review]
v2

With python2.6 this patch works.
Assignee: nobody → wfernandom2004
Attachment #550590 - Attachment is obsolete: true
Attachment #550590 - Flags: review?(Olli.Pettay)
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
(Assignee)

Comment 21

6 years ago
Because those classes in netwerk/websocketchannel reference nsWebSocket objects, which aren't thread safe. So, without that changes you will get many assertions.
(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.
(Reporter)

Comment 23

6 years ago
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.
Attachment #550928 - Attachment is obsolete: true
(Assignee)

Comment 24

6 years ago
> 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.
(Assignee)

Comment 25

6 years ago
(...) * and give you an answer soon.
(Reporter)

Comment 26

6 years ago
>  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!
(Assignee)

Comment 27

6 years ago
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.
Attachment #566770 - Attachment is obsolete: true
Attachment #567286 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 28

6 years ago
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*).
(Reporter)

Updated

6 years ago
Blocks: 695636
(Reporter)

Comment 29

6 years ago
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!
Attachment #567286 - Attachment is obsolete: true
Attachment #567286 - Flags: review?(jduell.mcbugs)
Attachment #567989 - Flags: review+
(Reporter)

Comment 30

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99d60858d86
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/a99d60858d86
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.