1215092 - WebSocketFrameService should be used for WebSocket discovering

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Andrea Marchesini [:baku]]

This bug is a follow up of bug 1203802.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming WebSocketFrameService to WebSocketFrame

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.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - Unique Serial number for WebSocketChannel (IPC too)

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - Unique Serial number for WebSocketChannel (IPC too)

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - Unique Serial number for WebSocketChannel (IPC too)

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

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - Events

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

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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


Honza

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - Renaming to WebSocketProxy

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - Unique Serial number for WebSocketChannel (IPC too)

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 3 - Events

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 4 - MessageAvailable event

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 12

[Michal Novotny [:michal]]

(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.

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Andrea, what do you think about comment #12?

Honza

Comment 14

[Andrea Marchesini [:baku]]

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

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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 16

[Michal Novotny [:michal]]

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.

Comment 17

[Michal Novotny [:michal]]

(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...

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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 19

[Michal Novotny [:michal]]

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 20

[Michal Novotny [:michal]]

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 21

[Michal Novotny [:michal]]

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.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 1 - Renaming WebSocketFrameService to WebSocketEventService

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 3 - Events

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 4 - MessageAvailable event

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.

Comment 25

[Andrea Marchesini [:baku]]

> 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"

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 2 - Unique Serial number for WebSocketChannel (IPC too)

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 27

[Michal Novotny [:michal]]

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 28

[Michal Novotny [:michal]]

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.

Comment 29

[Andrea Marchesini [:baku]]

> This is not true.

Don't we have OPCODE_CONTINUATION for this?

Comment 30

[Michal Novotny [:michal]]

(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.

Comment 31

[Andrea Marchesini [:baku]]

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

Comment 32

[Carsten Book [:Tomcat]]

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

Comment 33

[Pulsebot]

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

Comment 34

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 35

[Andrea Marchesini [:baku]]

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

Comment 36

[Carsten Book [:Tomcat]]

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