Open Bug 1497249 Opened 3 years ago Updated 9 months ago

[socket-proc] Connection stickiness handling for upgrade observers

Categories

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

enhancement

Tracking

()

REOPENED
81 Branch
Tracking Status
firefox64 --- wontfix
firefox80 --- wontfix
firefox81 --- wontfix
firefox82 --- wontfix

People

(Reporter: mayhemer, Assigned: kershaw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(4 files)

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.
Priority: -- → P3
Whiteboard: [necko-triaged]

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

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.

Flags: needinfo?(michal.novotny)

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

Flags: needinfo?(michal.novotny)

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

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

Should be done before landing back to central with pref off

Priority: P3 → P2

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

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

Assignee: nobody → kershaw
Flags: needinfo?(michal.novotny)

Done, sorry for the delay.

Flags: needinfo?(michal.novotny)
Pushed by kjang@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla80 → ---

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!

Flags: needinfo?(michal.novotny)
Pushed by kjang@mozilla.com:
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
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Flags: needinfo?(michal.novotny)
Regressions: 1663718
Regressions: 1668851

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

Flags: needinfo?(jstutte)

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

Flags: needinfo?(jstutte)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.