1517781 - Clean up child process connection timeouts and expose a promise-based interface

Status: NEW

(Core :: IPC, enhancement, P2)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Bug 1446161 exposed a convenient promise-based interface for waiting for process launch; we should also have one for waiting for the process to connect to the channel, and handle timeouts better.

Specifically, timeouts are currently a parameter passed in when synchronously blocking, which we'd prefer not to do; to get a timeout with asynchronous use, the process-type-specific code would have to set up its own timer and add complexity to deal with the callback ordering.

There's also bug 1488990: ContentParent starts setting things up after WhenProcessHandleReady and doesn't wait for the process to connect (which, in general, isn't actually necessary and just adds latency), so as far as I can tell it won't ever time out if the process doesn't connect.

Instead, GeckoChildProcessHost should be given a timeout and set a timer for it, and report failure to connect by that time as an error.  That way, the behavior will be the same independent of whether the caller uses sync blocking, the OnThingHappened callback methods, or MozPromise.


(Open question: should the connect timeout be relative to when the OS launch starts or when it finishes?  The former bounds the total delay, but the latter may avoid unnecessary failures if OS process launching is slow.  Currently it's relative to whenever the blocking wait method is called.)

(Potential problem: the Web Replay middleman process launches content processes but doesn't have most of XPCOM, so I don't know if we have nsITimer there.)

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #0)
> (Potential problem: the Web Replay middleman process launches content
> processes but doesn't have most of XPCOM, so I don't know if we have
> nsITimer there.)

I would think we'd almost have to have timer support in the middleman process.  Brian?

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

For background, the little I know about Web Replay and XPCOM was what I ran into in bug 1487287: we don't have SharedThreadPool or, apparently, the observer service.  Possibly I was doing something wrong / at the wrong time there, but in that case it was feasible to just retain the current behavior.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> (In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #0)
> > (Potential problem: the Web Replay middleman process launches content
> > processes but doesn't have most of XPCOM, so I don't know if we have
> > nsITimer there.)
> 
> I would think we'd almost have to have timer support in the middleman
> process.  Brian?

Timers should work in middleman processes, and XPCOM should as well --- we set up a document in the middleman, which entails having most browser state in place.  We do handle only a fraction of IPDL messages in the middleman (see HandleMessageInMiddleman in toolkit/recordreplay/ipc/ParentForwarding.cpp) so not all initialization will be performed.

Also, today I'm going to be working on bug 1517837, to make it easy to completely disable Web Replay's tests.  If Web Replay is causing problems for a patch (here or in other bugs), feel free to disable the tests and I'll work on fixing things up afterwards.