1496257 - Enable xpcshell-test for socket process isolation

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Junior [inactive]]

SPI doesn't launch socket process in current implementation. We need to fix it.

Comment 1

[Junior [inactive]]

Attachment: Bug 1496257 - P1 socket operation should wait until socket process launched

Comment 2

[Junior [inactive]]

attachment 9014219 [details] works for me with the xpcshell-test.

However, I need some input since SyncLaunch on mainthread is not the perfect solution, though.
(SyncLaunch a process takes 270 ms in my computer, which is long)

AsyncLaunch (LaunchAndWaitForProcessHandle) version flow on the main process:

step1 Main Thread
- SocketProcessHost::Launch
  - GeckoChildProcessHost::LaunchAndWaitForProcessHandle

step2 Non-Main Thread
- SocketProcessHost::OnChannelConnected

step3 Main Thread
- SocketProcessHost::InitAfterConnect
  - PSocketProcessParent::Open // top-level ipdl open
  - PSocketProcessParent::SendSetOffline
  - OnProcessLaunchComplete

As you know, launch takes really long time.
Socket request highly possible occurs before step2.

Moreover, step3 looks like it must be in the main thread.

So we have two options:
(1) simply block the main thread if there's a socket request and we haven't finished the process launch.
(2) queues the requests. Once the |OnProcessLaunchComplete| fires, dispatch the requests in queue.

Although it did heavily block in xpcshell-test.
However, we don't block main thread for ./mach run.
Hence I prefer the simple implementation.

Comment 3

[Junior [inactive]]

FWIW, current result

xpcshell                               
~~~~~~~~                               
Ran 582 checks (489 tests, 93 subtests)
Expected results: 388                  
Skipped: 14 tests                      
Unexpected results: 180                
  test: 130 (118 fail, 12 timeout)     
  subtest: 50 (50 fail)

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Junior Hsu from comment #2)
> (2) queues the requests. Once the |OnProcessLaunchComplete| fires, dispatch
> the requests in queue.

This is the most acceptable solution, yes.  I believe we will have more places accessing the gIOService->mSocketProcess in a potentially racy way, tho.  We will simply have to catch and fix them all when they appear.

Main thread blocking (either to have some monitor signalling or spin the main thread even loop) is not an option.

Comment 5

[Junior [inactive]]

Attachment: Bug 1496257 - P2 AsyncLaunch and let IOService queue the pending events

Queue the nsHttpChannel::AsyncOpen in P3 in order to make the patch unrelated to nsHttpChannel

Comment 6

[Junior [inactive]]

Attachment: Bug 1496257 - P3 queue the nsHttpChannel::AsyncOpenFinal if the socket process is not ready

Need not to cherry-pick this patch if we don't split nsHttpTranscation

Comment 8

[Dragana Damjanovic [:dragana]]

Junior, has this landed? can you land it? Can you also make a new bug and upload a patch that we can land on the m.-c. (without http2 part). Please make on top of 1513135

Comment 9

[Junior [inactive]]

land in larch
https://hg.mozilla.org/projects/larch/rev/2ada61a20690ee5b5afc81787c3dc7dd79a5996c
https://hg.mozilla.org/projects/larch/rev/e97fe90fa93bb3df64d3a262c815971a20003d1a
https://hg.mozilla.org/projects/larch/rev/f7ea92227fd01e56ab016d08eb6ad651811f026d

Comment 10

[Junior [inactive]]

(In reply to Dragana Damjanovic [:dragana] from comment #8)
> Junior, has this landed? can you land it? Can you also make a new bug and
> upload a patch that we can land on the m.-c. (without http2 part). Please
> make on top of 1513135

I believe it's bug 1513057. I'll post my patch there since all the patches should be land in one-shot.

Comment 11

[Junior [inactive]]

Attachment: Bug 1496257 - P4 (backport from m-c) rename to CallOrWaitForSocketProcess

backport the modification from bug 1513057

Comment 12

[Junior [inactive]]

Attachment: Bug 1496257 - P5 adjust the API changes

Use CallOrWaitForSocketProcess instead