Closed Bug 1215092 Opened 4 years ago Closed 4 years ago

WebSocketFrameService should be used for WebSocket discovering

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(4 files, 9 obsolete files)

56.95 KB, patch
Details | Diff | Splinter Review
30.47 KB, patch
Details | Diff | Splinter Review
16.58 KB, patch
michal
: review+
Details | Diff | Splinter Review
18.83 KB, patch
michal
: review+
Details | Diff | Splinter Review
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.
Attachment #8674315 - Attachment is obsolete: true
Sorry for the spam. I uploaded the same patch than before. This is the correct one.
Attachment #8674322 - Attachment is obsolete: true
Attached patch part 3 - Events (obsolete) — Splinter Review
This is the last patch for this bug. It exposed Created/Closed/Opened events.
Flags: needinfo?(odvarko)
I can't apply patches from bug 1203802
https://bugzilla.mozilla.org/show_bug.cgi?id=1203802#c74


Honza
Flags: needinfo?(odvarko) → needinfo?(amarchesini)
Attachment #8674214 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8676833 - Flags: review?(michal.novotny)
Attachment #8674324 - Attachment is obsolete: true
Attachment #8676833 - Attachment description: renaming.patch → part 1 - Renaming to WebSocketProxy
Attachment #8676835 - Flags: review?(michal.novotny)
Attached patch part 3 - Events (obsolete) — Splinter Review
Attachment #8675705 - Attachment is obsolete: true
Attachment #8676841 - Flags: review?(michal.novotny)
Attached patch part 4 - MessageAvailable event (obsolete) — Splinter Review
Attachment #8676961 - Flags: review?(michal.novotny)
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
Target Milestone: --- → mozilla44
(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
Flags: needinfo?(amarchesini)
This definitely makes sense. I'm very bad with naming things.
Flags: needinfo?(amarchesini)
(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.
Attachment #8676833 - Flags: review?(michal.novotny) → review+
(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"
Attachment #8676835 - Flags: review?(michal.novotny) → review-
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.
Attachment #8676841 - Flags: review?(michal.novotny) → review+
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.
Flags: needinfo?(amarchesini)
Attachment #8676833 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8679274 - Flags: review?(michal.novotny)
Attached patch part 3 - EventsSplinter Review
Attachment #8676841 - Attachment is obsolete: true
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.
Attachment #8676961 - Attachment is obsolete: true
Attachment #8676961 - Flags: review?(michal.novotny)
Attachment #8679276 - Flags: review?(michal.novotny)
> 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.
Attachment #8676835 - Attachment is obsolete: true
Attachment #8679279 - Flags: review?(michal.novotny)
Attachment #8679274 - Flags: review?(michal.novotny)
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.
Attachment #8679279 - Flags: review?(michal.novotny) → review+
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.
Attachment #8679276 - Flags: review?(michal.novotny) → review+
> 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.
sorry had to back this out for failing on own tests like https://treeherder.mozilla.org/logviewer.html#?job_id=16384136&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
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
You need to log in before you can comment on or make changes to this bug.