1497249 - [socket-proc] Connection stickiness handling for upgrade observers

Status: RESOLVED MOVED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/projects/larch/file/322a2074e4f0/netwerk/protocol/http/nsHttpChannel.cpp#l7648
https://hg.mozilla.org/projects/larch/file/322a2074e4f0/netwerk/protocol/http/nsHttpChannel.cpp#l7743

Comment 1

[Honza Bambas (:mayhemer)]

Priority and also implementation of this bug depends on how we are going to handle websocket upgrades.  This is related to listening sockets as well - how are we going to deal with it.

Comment 2

[Kershaw Chang [:kershaw]]

To complete websocket upgrade, it means that the ownership of socket transport should be transfered [1] to websocket channel.
I am not sure how to do this since parent process can't access the socket transport in socket process. Maybe we have to provide IPC version of nsISocketTransport, nsIAsyncInputStream and nsIAsyncOutputStream [2].

[1] https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/netwerk/protocol/http/nsHttpConnectionMgr.cpp#2875-2885
[2] https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/media/mtransport/ipc/WebrtcProxyChannel.cpp#289

Comment 3

[Kershaw Chang [:kershaw]]

Hi Michal,

Could you take a look at comment2? I think adding IPC interface for nsISocketTransport and nsIAsync(Input/Output)Stream might be too complicated. Could you provide your insight here?

Thanks.

Comment 4

[Michal Novotny [:michal]]

(In reply to Kershaw Chang [:kershaw] from comment #3)

Hi Michal,

Could you take a look at comment2? I think adding IPC interface for nsISocketTransport and nsIAsync(Input/Output)Stream might be too complicated. Could you provide your insight here?

Thanks.

I'm not sure I understand the problem here because I'm not familiar with socket process changes. Websocket channel needs the socket to read/write data from/to it.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Michal Novotny [:michal] from comment #4)

(In reply to Kershaw Chang [:kershaw] from comment #3)

Hi Michal,

Could you take a look at comment2? I think adding IPC interface for nsISocketTransport and nsIAsync(Input/Output)Stream might be too complicated. Could you provide your insight here?

Thanks.

I'm not sure I understand the problem here because I'm not familiar with socket process changes. Websocket channel needs the socket to read/write data from/to it.

Sorry that I didn't explain how socket process works first.

The current architecture is keeping http channel in parent process and others under http channel (e.g. http transaction and http connection) in socket process. The thing is that the socket transport that Websocket channel needs is only accessible in socket process.

A possible solution is to implement IPC interface for nsISocketTransport, nsIAsyncInputStream and nsIAsyncOutputStream, but it seems this is a bit too complicated.

Comment 6

[Michal Novotny [:michal]]

(In reply to Kershaw Chang [:kershaw] from comment #5)

A possible solution is to implement IPC interface for nsISocketTransport, nsIAsyncInputStream and nsIAsyncOutputStream, but it seems this is a bit too complicated.

I guess the other possible solution is to separate socket manipulation from the WebSocketChannel and let it run in the socket process. I'm not sure what is less complicated. Also, the generic solution could be used by other code, not just websocket channel.

Comment 7

[Junior [inactive]]

Should be done before landing back to central with pref off

Comment 8

[Junior [inactive]]

Attachment: bug 1497249 - let Proxy/Websocket upgrade works again in parent process

Simple patch for enable upgrade again. Only without socket process.

Comment 9

[Junior [inactive]]

https://hg.mozilla.org/projects/larch/rev/1a8cf90422a3a378bda9123dec8d8b8eebfc8220
leave open intentionally since we only deal with non-socket-process case

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Bug 1497249 - P1: Introduce nsWebSocketConnection interface for separating socket manipulation

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Bug 1497249 - P2: ipdl for nsIWebSocketConnection

Comment 12

[Kershaw Chang [:kershaw]]

Ping for review https://phabricator.services.mozilla.com/D30624.
Thanks.

Comment 13

[Michal Novotny [:michal]]

Done, sorry for the delay.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74bb3c2ab767
P1: Introduce nsWebSocketConnection interface for separating socket manipulation r=michal,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/f7b6aac1b58b
P2: ipdl for nsIWebSocketConnection r=michal

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/74bb3c2ab767
https://hg.mozilla.org/mozilla-central/rev/f7b6aac1b58b

Comment 16

[Alexandru Michis [:malexandru]]

Backed out 3 changesets (bug 1653642, bug 1497249) for causing Bug 1654282.
Backed out changesets f7b6aac1b58b, 74bb3c2ab767.
Link: https://hg.mozilla.org/integration/autoland/rev/d72591a16d42a6cefcffe3df901fa9d0db440193

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/d72591a16d42

Comment 18

[Kershaw Chang [:kershaw]]

Attachment: Bug 1497249 - Don't release WebSocketChannel in nsWebSocketConnection::Close, r=michal

Depends on D30624

Comment 19

[Kershaw Chang [:kershaw]]

Michal, it would be great if you can review D30624 and D85735 within this week, so we have more time to test theses patches in this cycle.
Thank you!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/432c7e9dc19f
P1: Introduce nsWebSocketConnection interface for separating socket manipulation r=michal,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/2502449c2e24
P2: ipdl for nsIWebSocketConnection r=michal
https://hg.mozilla.org/integration/autoland/rev/1cd6d2f9a603
Don't release WebSocketChannel in nsWebSocketConnection::Close, r=michal,necko-reviewers

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/432c7e9dc19f
https://hg.mozilla.org/mozilla-central/rev/2502449c2e24
https://hg.mozilla.org/mozilla-central/rev/1cd6d2f9a603

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Beta81 for causing bug 1663718. It remains landed for 82+.
https://hg.mozilla.org/releases/mozilla-beta/rev/0218c1751126fd28df953d37fb611a82362f9e56

Comment 23

[Mathew Hodson]

This looks like it was backed out of 82 and later in bug 1673340. Should this bug be reopened?

Comment 24

[Kershaw Chang [:kershaw]]

(In reply to Mathew Hodson from comment #23)

This looks like it was backed out of 82 and later in bug 1673340. Should this bug be reopened?

Yes, let's reopen this.

Comment 25

[Kershaw Chang [:kershaw]]

This bug was fixed by bug 1716566.