Enable xpcshell-test for socket process isolation

RESOLVED FIXED

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: junior, Assigned: junior)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-triaged])

Attachments

(3 attachments, 2 obsolete attachments)

SPI doesn't launch socket process in current implementation. We need to fix it.
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.
Flags: needinfo?(honzab.moz)
Priority: -- → P2
Whiteboard: [necko-triaged]
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)
(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.
Flags: needinfo?(honzab.moz)
QA Contact: jduell.mcbugs
QA Contact: jduell.mcbugs
Attachment #9014219 - Attachment description: Bug 1496257 - socket operation should wait until socket process launched → Bug 1496257 - P1 socket operation should wait until socket process launched
Queue the nsHttpChannel::AsyncOpen in P3 in order to make the patch unrelated to nsHttpChannel
Need not to cherry-pick this patch if we don't split nsHttpTranscation
Assignee: nobody → juhsu
Attachment #9016129 - Attachment description: Bug 1496257 - P3 queue the nsHttpChannel::AsyncOpen if the socket process is not ready → Bug 1496257 - P3 queue the nsHttpChannel::AsyncOpenFinal if the socket process is not ready
Duplicate of this bug: 1513872
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
Flags: needinfo?(juhsu)
(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.
backport the modification from bug 1513057
Use CallOrWaitForSocketProcess instead
Attachment #9032505 - Attachment is obsolete: true
Attachment #9032506 - Attachment is obsolete: true
Blocks: 1540000
You need to log in before you can comment on or make changes to this bug.