This bug is a follow up of bug 1203802.

This first patch is about renaming the existing interfaces: nsIWebSocketFrameService -> nsIWebSocketService nsIWebSocketFrameListener -> nsIWebSocketProxy The next steps are to dispatch events when a new WebSocket is created and create a nsIWebSocketProxy per each WebSocket object.

Sorry for the spam. I uploaded the same patch than before. This is the correct one.

This is the last patch for this bug. It exposed Created/Closed/Opened events.

I can't apply patches from bug 1203802 https://bugzilla.mozilla.org/show_bug.cgi?id=1203802#c74 Honza

I've been testing the API and I don't see any issues. All seem to be working nicely! One issue I had was with the last patch: msg.patch, I couldn't cleanly apply it, there is a failure related to the dom/base/test/test_websocket_frame.html One missing thing we've discussed on IRC with Andrea is a reference to the initiating HTTP request that is sent by the client to create (upgrade to) WS connection. This reference should be used to create a link in the existing Network panel that can navigate the user to the Web Sockets panel (and highlight the right WS connection). Since API in this bug needs to land next week I don't want to block this bug and I think it should be done as a follow up. Andrea, please let me know if you want me to file the bug and provide some more info about why the tools need it. Honza

(In reply to Andrea Marchesini (:baku) from comment #1) > nsIWebSocketFrameService -> nsIWebSocketService > nsIWebSocketFrameListener -> nsIWebSocketProxy nsIWebSocketProxy is definitely a bad name. It's a listener, so it's name should be something like nsIWebSocketEventListener. nsIWebSocketService is OK, but since it just adds and removes event listeners, then e.g. nsIWebSocketEventService would be more appropriate.

Andrea, what do you think about comment #12? Honza

This definitely makes sense. I'm very bad with naming things.

(In reply to Andrea Marchesini (:baku) from comment #14) > This definitely makes sense. I'm very bad with naming things. ok, good, please update the patch, so Michal can do the review this week. Honza

Comment on attachment 8676833 [details] [diff] [review] part 1 - Renaming to WebSocketProxy Review of attachment 8676833 [details] [diff] [review]: ----------------------------------------------------------------- AFAICS this patch only renames the classes, so r+ with some better names :) ::: layout/build/nsLayoutModule.cpp @@ +636,1 @@ > { 0x5973dd8f, 0xed2c, 0x41ff, { 0x9e, 0x64, 0x25, 0x1f, 0xf5, 0x5a, 0x67, 0xb9 }} I'm not sure but I guess you have to change CID too.

(In reply to Jan Honza Odvarko [:Honza] from comment #15) > (In reply to Andrea Marchesini (:baku) from comment #14) > > This definitely makes sense. I'm very bad with naming things. > ok, good, please update the patch, so Michal can do the review this week. > > Honza No need to update the patches now. I'll review them as they are and then you can just replace the names...

(In reply to Michal Novotny (:michal) from comment #17) > (In reply to Jan Honza Odvarko [:Honza] from comment #15) > > (In reply to Andrea Marchesini (:baku) from comment #14) > > > This definitely makes sense. I'm very bad with naming things. > > ok, good, please update the patch, so Michal can do the review this week. > > > > Honza > > No need to update the patches now. I'll review them as they are and then you > can just replace the names... Great, thanks! Honza

Comment on attachment 8676835 [details] [diff] [review] part 2 - Unique Serial number for WebSocketChannel (IPC too) Review of attachment 8676835 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/websocket/BaseWebSocketChannel.cpp @@ +52,5 @@ > + processID = cc->GetID(); > + } > + > + MOZ_RELEASE_ASSERT(processID < (uint64_t(1) << kWebSocketIDProcessBits)); > + uint64_t processBits = processID & ((uint64_t(1) << kWebSocketIDProcessBits) - 1); I don't understand this code. If ContentChild::GetID() is guaranteed to fit into 22 bits (I don't think so) then the release assertion is enough and ANDing with the bitmask is unnecessary. If it can be larger and actually is larger then the program crashes on the assertion. The same problem is with the same code in dom/ipc/ContentChild.cpp. @@ +57,5 @@ > + > + // Make sure no actual window ends up with mWebSocketID == 0. > + uint64_t windowID = ++gNextWebSocketID; > + > + MOZ_RELEASE_ASSERT(windowID < (uint64_t(1) << kWebSocketIDWebSocketBits)); The same as above. This code simply relies on the fact that firefox doesn't create enough WebSocketChannels to overflow kWebSocketIDWebSocketBits. Also please rename "window" to "websocket"

Comment on attachment 8676841 [details] [diff] [review] part 3 - Events Review of attachment 8676841 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/websocket/nsIWebSocketChannel.idl @@ +238,5 @@ > + return 0; > + } > + return serial; > + } > +%} This isn't an interface change, so UUID doesn't need to be changed.

Comment on attachment 8676961 [details] [diff] [review] part 4 - MessageAvailable event Looks good, I just don't know why it is needed, because webSocketMessageAvailable() and frameReceived() provide the same information.

I think this is needed because with this patch we dispatch an event when the Message is delivered. In theory, a frame can contain a partial message so we need to collect more frames in order to dispatch a full message body. Plus, with this message we give information about the 'type' of message: blob, arrayBuffer, etc.

> I don't understand this code. If ContentChild::GetID() is guaranteed to fit > into 22 bits (I don't think so) then the release assertion is enough and > ANDing with the bitmask is unnecessary. If it can be larger and actually is > larger then the program crashes on the assertion. Correct. I think it should be fine because having a 22 bits of ID, means that we running contentChild 4194303 (if I did the math correctly). That should be fine for now. Any changes can be done in follow up for window and webSocket IDs. I'm OK about removing the assertion. > > + // Make sure no actual window ends up with mWebSocketID == 0. > > + uint64_t windowID = ++gNextWebSocketID; > > + > > + MOZ_RELEASE_ASSERT(windowID < (uint64_t(1) << kWebSocketIDWebSocketBits)); > > The same as above. This code simply relies on the fact that firefox doesn't > create enough WebSocketChannels to overflow kWebSocketIDWebSocketBits. Also > please rename "window" to "websocket"

Here a patch without assertion and with the check in case we overflow the gNextWebSocketID. Of course, in theory it means that a page could have 2 webSockets using the same ID but that is, in reality almost impossible with a bitMap of 31.

Comment on attachment 8679274 [details] [diff] [review] part 1 - Renaming WebSocketFrameService to WebSocketEventService Review of attachment 8679274 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/build/nsLayoutModule.cpp @@ +637,1 @@ > { 0x5973dd8f, 0xed2c, 0x41ff, { 0x9e, 0x64, 0x25, 0x1f, 0xf5, 0x5a, 0x67, 0xb9 }} You didn't react to my comment but also didn't change the CID value. Are you 100% sure that keeping the CID value unchanged is OK? ::: netwerk/protocol/websocket/PWebSocketFrameListener.ipdl @@ -4,5 @@ > -/* This Source Code Form is subject to the terms of the Mozilla Public > - * License, v. 2.0. If a copy of the MPL was not distributed with this > - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > - > -include protocol PNecko; Instead of renaming, this patch removes PWebSocketFrameListener.ipdl and creates PWebSocketEventListener.ipdl. Please fix this.

Comment on attachment 8679276 [details] [diff] [review] part 4 - MessageAvailable event (In reply to Andrea Marchesini (:baku) from comment #24) > I think this is needed because with this patch we dispatch an event when the > Message is delivered. In theory, a frame can contain a partial message so we > need to collect more frames in order to dispatch a full message body. This is not true. The frame passed to WebSocketEventService is actually a message, so for every frameReceived() call (non-control) there is exactly one webSocketMessageAvailable() call and there is an obvious duplicity. I'm not going to block the review on this, but if webSocketMessageAvailable() should stay it would make much more sense to change frameReceived() so that it provides really raw frames as received from the server, i.e. masked and possibly fragmented or compressed.

> This is not true. Don't we have OPCODE_CONTINUATION for this?

(In reply to Andrea Marchesini (:baku) from comment #29) > > This is not true. > > Don't we have OPCODE_CONTINUATION for this? It is used only in WebSocketChannel::ProcessInput(). No WebSocketFrame is ever created with this opcode.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7393a036ce36 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2284c3e8c336 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f527785e39c6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2730cc97c6ec

sorry had to back this out for failing on own tests like https://treeherder.mozilla.org/logviewer.html#?job_id=16384136&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa26a79f3f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd7ac7ed4f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/929a872c1f15 https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9c356aafe6

Andrea, just heads up, the merge from nightly to aurora is happening on Friday, not Monday (in fact all branches will be merged early). Hope we'll make it. Honza

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f42f6bc21a8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/684e5160767d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe351a62696 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6cd337118c4

https://hg.mozilla.org/mozilla-central/rev/0f42f6bc21a8 https://hg.mozilla.org/mozilla-central/rev/684e5160767d https://hg.mozilla.org/mozilla-central/rev/3fe351a62696 https://hg.mozilla.org/mozilla-central/rev/a6cd337118c4