Closed Bug 1485353 Opened 2 years ago Closed 2 years ago

Create PHttpTransaction IPC protocol

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox63 --- affected

People

(Reporter: mayhemer, Assigned: CuveeHsu)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(5 files, 1 obsolete file)

This will be the basic class to send information and notifications between the parent process and the socket process.

There are few methods on nsHttpConnectionMgr that now take nsHttpTransaction.  We will add companion methods to those to take PHttpTransactionParent (I'll file a separate bug).

The PHttpTransactionParent should send down the necessary information, see nsHttpTransaction::Init method for the list of args it now accepts.

The PHttpTransactionChild will create and wrap the "real" nsHttpTransaction and let it live on the socket process.  It will also be responsible for notifications from the the data pipe (onstart/ondata/onstop) to the parent with necessary transaction properties serialization (like response headers, response code etc.)

At this early stage I would not care much about notification callbacks.

Transport event sink should be easy to impl.
Depends on: 1485619
Assignee: nobody → juhsu
Whiteboard: [necko-triaged]
1. nsProxyInfo is not easy to serialize, and moving proxy resolution to socket process is to be determined.
   Would not support proxy now. Therefore we can serialize connectionInfo without mProxyInfo 

2. Will not handle security callback in the first version.

3. ResponseHead is already serializable. We can do the same thing to RequestHead.

4. EventTarget [2] for nsTransaction::Init is used to destruct at the right thread. We can remove it.

5. Should move the mTransactionPump to the HttpTransactionChild side. 

[1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/netwerk/protocol/http/nsHttpConnectionInfo.h#169

[2] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/netwerk/protocol/http/nsHttpTransaction.cpp#245
(In reply to Junior Hsu from comment #1)
> 1. nsProxyInfo is not easy to serialize, and moving proxy resolution to
> socket process is to be determined.
>    Would not support proxy now. Therefore we can serialize connectionInfo
> without mProxyInfo 
> 
> 2. Will not handle security callback in the first version.
> 
> 3. ResponseHead is already serializable. We can do the same thing to
> RequestHead.
> 
> 4. EventTarget [2] for nsTransaction::Init is used to destruct at the right
> thread. We can remove it.
> 
> 5. Should move the mTransactionPump to the HttpTransactionChild side. 
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 55da592d85c2baf8d8818010c41d9738c97013d2/netwerk/protocol/http/
> nsHttpConnectionInfo.h#169
> 
> [2]
> https://searchfox.org/mozilla-central/rev/
> 55da592d85c2baf8d8818010c41d9738c97013d2/netwerk/protocol/http/
> nsHttpTransaction.cpp#245

Thanks!  This all sounds about right and fine.

The only bit I'm not sure about is why to move mTransactionPump to the HttpTransactionChild.  It would be great to not use a pump on the socket process at all EVENTUALLY to save dispatch from the socket thread to the main (or some other) thread, pump only works that way - async, AFAIK, but worth double checking.  Better would be to push the data from socket/h2-stream input streams directly to the appropriate IPC interface right on the socket thread.  But that of course, can be done in a followup.
comment out those TODO codes
I can load bbc, cnn here. Sounds not bad, right?
I couldn't bear to run the test...
This is just a WIP patch.
To enable socket process, please type: "MOZ_LOG=socketprocess:5 NECKO_DEDICATED_PROCESS=1 ./mach run".
create a nsAHttpTransactionShell to abstract the transaction operation in the parent process

I know it's not compile-able now.
However, is it possible to quickly check the setup of the shell interface and nsHttpChannel?
And xpcshell-test can't initialize the socket process hence I mark-out the |IsSocketProcessConnected| check.
Comment on attachment 9005781 [details]
Bug 1485353 - P1 create HttpTransactionParent/Child to pretend IPC

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9005781 - Flags: review+
Comment on attachment 9009806 [details]
Bug 1485353 - P2 disable passing objects which is used by both processes

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9009806 - Flags: review+
Comment on attachment 9010012 [details]
Bug 1485353 - P3: Create PHttpTransacation.ipdl

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9010012 - Flags: review+
Comment on attachment 9010494 [details]
Bug 1485353 - P4 facilitate opt-out remote nsHttpTransaction

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9010494 - Flags: review+
Attachment #9010012 - Attachment description: Bug 1485353 - P3 WIP → Bug 1485353 - P3: Create PHttpTransacation.ipdl
Junior, Kershaw, please let me know when this is finally ready to land (when you are done with any last updates to the patches), thanks.
Flags: needinfo?(kershaw)
Flags: needinfo?(juhsu)
Rebase patch from central to larch, Original patch is D6182 written by Kershaw.

  1. Create PHttpTransacation.ipdl
  2. Implement necessary IPC messages, like OnStartRequest, OnDataAvailable, and OnStopRequest
  3. Add support to use OptionalIPCStream between parent and socket process
  4. Can load simple web page now, like example.com
Only specifying NECKO_DEDICATED_PROCESS=0 will disable socket process
(In reply to Honza Bambas (:mayhemer) from comment #11)
> Junior, Kershaw, please let me know when this is finally ready to land (when
> you are done with any last updates to the patches), thanks.

I think P3 is ready to land.
Flags: needinfo?(kershaw)
We can land them after P4, P5 are reviewed.
I know P4 is reviewed once, but it would be better to have it seen again.
Flags: needinfo?(juhsu)
Attachment #9011151 - Attachment is obsolete: true
Comment on attachment 9010494 [details]
Bug 1485353 - P4 facilitate opt-out remote nsHttpTransaction

Honza Bambas (:mayhemer) has requested changes to the revision.
Attachment #9010494 - Flags: review+
Comment on attachment 9011159 [details]
Bug 1485353 - P5 reverse the env setting for NECKO_DEDICATED_PROCESS

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9011159 - Flags: review+
Comment on attachment 9010494 [details]
Bug 1485353 - P4 facilitate opt-out remote nsHttpTransaction

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9010494 - Flags: review+
Blocks: 1494378
Attachment #9010012 - Attachment is obsolete: true
Attachment #9010012 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.