1037669 - Add a "document settings object" that hangs off of nsIWebSocketChannel

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Jason Duell]

Followup to bug 965413.

I'm guessing Tanvi or Christoph may actually be the most logical people to fix this, as they're touching all this code very soon and know the landscape well.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Interesting, this is actually one of the cases which bug 1038756 wouldn't fix, because the channel is not created through NS_NewChannel, but rather by calling NewChannelFromURIWithProxyFlags directly on the ioservice. Anyway, we can set the LoadInfo on that channel manually right after the channel is created [1].

Creating dependency to bug 1038756 anyway, because they are so closely related.


[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#2785

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1037669_loadInfo_on_websockets.patch

Providing a WIP patch to start the discussion:

I think it's best if we manually set the loadInfo right after channel creation in the WebSocketChannel. Here are the remaining questions/thoughts:

In WebSocket::Init() we set mPrincipal, what if we also create and set mDocument, which would be the |originDoc| we use to call ContentPolicies [1].

Later in AsyncOpen [2], we could "eventually" use the |aContext| argument to pass that information. Currently aContext is always a nullptr, what sideEffects would that cause here [3]. It would be great to have the requestingContext (node) in addition to the principal, especially to handle redirects for MCB correctly.

Finally, what do we do for e10s? Passing a nullptr as the principal is not that great :-(


[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp#724
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#2663
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#2828

Comment 3

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Created attachment 8472641 [details] [diff] [review]
> bug_1037669_loadInfo_on_websockets.patch


The websocket channel is created with do_createInstance here - http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp#781.

We should add loadInfo to the wschannel in EstablishConnection().  Then when AsyncOpen is called, a local http channel is used to bootstrap the connection:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#2769

We can pull the loadinfo from wschannel and use it for the localchannel.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1037669_loadInfo_on_websockets.patch

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> The websocket channel is created with do_createInstance here -
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.
> cpp#781.
> 
> We should add loadInfo to the wschannel in EstablishConnection().  Then when
> AsyncOpen is called, a local http channel is used to bootstrap the
> connection:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/
> WebSocketChannel.cpp#2769
> 
> We can pull the loadinfo from wschannel and use it for the localchannel.

That seems reasonable - here we go.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1037669_storing_loadinfo_on_websocketchannels.patch

Since we are getting closer to land 1038756, we can also get that patch reviewed, which can land right after the patch series of bug 1038756.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8488059 [details] [diff] [review]
bug_1037669_storing_loadinfo_on_websocketchannels.patch

Review of attachment 8488059 [details] [diff] [review]:
-----------------------------------------------------------------

Jason knows this code better than me.

Comment 7

[Jason Duell]

Comment on attachment 8488059 [details] [diff] [review]
bug_1037669_storing_loadinfo_on_websocketchannels.patch

Review of attachment 8488059 [details] [diff] [review]:
-----------------------------------------------------------------

as discussed on IRC we should move the Set/GetLoadInfo methods into BaseWebsocketChannel, and then we get support for both WebsocketChannel and WebsocketChannelChild (we need the latter for e10s websockets).

::: netwerk/protocol/websocket/nsIWebSocketChannel.idl
@@ +52,5 @@
>       */
>      attribute nsILoadGroup loadGroup;
>  
>      /**
> +     * The load info of the websockets code.

Change to 

   // The load info of the websocket

Last I checked the code itself didn't have a loadinfo :)

(Yes, I know the loadGroup attribute above has the "websockets code" wording: that's a typo so feel free to fix that one too.)

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8488059 [details] [diff] [review]
bug_1037669_storing_loadinfo_on_websocketchannels.patch

Canceling review request for now. Once I got it working for e10s I will flag both of you again.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1037669_storing_loadinfo_on_websocketchannels.patch

(In reply to Jason Duell (:jduell) from comment #7)
>    // The load info of the websocket
>
> (Yes, I know the loadGroup attribute above has the "websockets code"
> wording: that's a typo so feel free to fix that one too.)

Whenever I base some comments/code on other comments/code already in that file something like this happens. Updated both comments :-)

Comment 10

[Jason Duell]

Comment on attachment 8492765 [details] [diff] [review]
bug_1037669_storing_loadinfo_on_websocketchannels.patch

Review of attachment 8492765 [details] [diff] [review]:
-----------------------------------------------------------------

Looks perfect.

Comment 11

[Tanvi Vyas[:tanvi]]

Comment on attachment 8492765 [details] [diff] [review]
bug_1037669_storing_loadinfo_on_websocketchannels.patch

>diff --git a/content/base/src/WebSocket.cpp b/content/base/src/WebSocket.cpp
>@@ -796,16 +797,25 @@ WebSocket::EstablishConnection()
>+  nsCOMPtr<nsIDocument> doc = do_QueryReferent(mOriginDocument);

Add a comment about the how we are setting a custom loadinfo - something like:
Manually add loadinfo to the channel since it wasn't set during channel creation.

>+  nsCOMPtr<nsILoadInfo> loadInfo =
>+    new LoadInfo(mPrincipal,
>+                 doc,
>+                 nsILoadInfo::SEC_NORMAL,
>+                 nsIContentPolicy::TYPE_WEBSOCKET);
>+  rv = wsChannel->SetLoadInfo(loadInfo);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11f65f0567c5

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/11f65f0567c5