Closed Bug 1037669 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Networking: WebSockets, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jduell.mcbugs, Assigned: ckerschb)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 965413
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
Depends on: 1038756
Blocks: 1041180
Blocks: 1052887
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
Attachment #8472641 - Flags: feedback?(jduell.mcbugs)
(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.
(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.
Attachment #8472641 - Attachment is obsolete: true
Attachment #8472641 - Flags: feedback?(jduell.mcbugs)
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.
Attachment #8472699 - Attachment is obsolete: true
Attachment #8488059 - Flags: review?(jonas)
Assignee: tanvi → mozilla
Attachment #8488059 - Flags: review?(tanvi)
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.
Attachment #8488059 - Flags: review?(jonas) → review?(jduell.mcbugs)
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.)
Attachment #8488059 - Flags: review?(jduell.mcbugs) → review-
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.
Attachment #8488059 - Flags: review?(tanvi)
(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 :-)
Attachment #8488059 - Attachment is obsolete: true
Attachment #8492765 - Flags: review?(jduell.mcbugs)
Attachment #8492765 - Flags: review?(tanvi)
Status: NEW → ASSIGNED
Comment on attachment 8492765 [details] [diff] [review]
bug_1037669_storing_loadinfo_on_websocketchannels.patch

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

Looks perfect.
Attachment #8492765 - Flags: review?(jduell.mcbugs) → review+
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);
>+
Attachment #8492765 - Flags: review?(tanvi) → review+
Blocks: 1071520
https://hg.mozilla.org/mozilla-central/rev/11f65f0567c5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1072316
You need to log in before you can comment on or make changes to this bug.