Closed
Bug 1037669
Opened 11 years ago
Closed 11 years ago
Add a "document settings object" that hangs off of nsIWebSocketChannel
Categories
(Core :: Networking: WebSockets, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jduell.mcbugs, Assigned: ckerschb)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 3 obsolete files)
8.32 KB,
patch
|
jduell.mcbugs
:
review+
tanvi
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
(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)
Assignee | ||
Comment 5•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: tanvi → mozilla
Assignee | ||
Updated•11 years ago
|
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)
Reporter | ||
Comment 7•11 years ago
|
||
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-
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
Attachment #8492765 -
Flags: review?(tanvi)
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
Target Milestone: --- → mozilla35
Comment 13•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•