874755 - Gecko should provide a way to enumerate and communicate with child processes

Status: ASSIGNED

(Core :: DOM: Core & HTML, defect)

Description

[Jim Blandy :jimb]

The remote debugging protocol server needs to be able enumerate all content child processes, and communicate with them. This will let us extend the protocol into E10S processes, Firefox OS apps and pages, and so on. Simply providing a JS interface to ContentParent should be enough.

Comment 1

[Jim Blandy :jimb]

Attachment: Implement nsIContentParent and nsIContentProcessService, for enumerating and controlling content processes.

Work in progress. This includes some xpcshell machinery for creating multiple test child processes (sendCommand only gets you one), and xpcshell tests for the new facilities.

(If something like this is acceptable, I think we can use it to replace XPCShellEnvironment, and perhaps PTestShell as well. We can just use nsIMessageManager and the nsIContentParent interface added by this patch for all communication with child processes.)

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Don't use jsval for contentID.

Comment 3

[Jim Blandy :jimb]

(In reply to :Ms2ger from comment #2)
> Don't use jsval for contentID.

Good point --- I'll fix that. Thanks for looking this over!

Comment 4

[Jim Blandy :jimb]

Attachment: Implement nsIContentParent and nsIContentProcessService, for enumerating and controlling content processes.

I think this is about ready for review; presenting to Try first.
https://tbpl.mozilla.org/?tree=Try&rev=b78516354f2e

Comment 5

[Jim Blandy :jimb]

The xpcshell tests are failing:

Error: Starting child-eval-service failed; nsresult 2147746132 - See following stack:
18:42:22     INFO -  @/builds/slave/test/build/tests/xpcshell/head.js:1217
18:42:22     INFO -  @/builds/slave/test/build/tests/xpcshell/head.js:1192

I can't reproduce this on my machine yet.

Comment 6

[Jim Blandy :jimb]

Okay, it seems like I just needed to list the childevalservice JS and manifest file in a few more places. Fresh try:
https://tbpl.mozilla.org/?tree=Try&rev=28ed18a53e6d

Comment 7

[Jim Blandy :jimb]

Attachment: Implement nsIContentParent and nsIContentProcessService, for enumerating and controlling content processes.

Updated patch.

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 763806 [details] [diff] [review]
Implement nsIContentParent and nsIContentProcessService, for enumerating and controlling content processes.

Review of attachment 763806 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I think this looks good, modulo the dual-purpose interface. I'd probably want someone like smaug or bsmedberg to do a superreview here, though.

::: content/base/public/nsIContentProcessService.idl
@@ +20,5 @@
> +  /* The ID we assigned to this content child process. */
> +  readonly attribute unsigned long long contentID;
> +
> +  /* The nsIMessageSender that sends to this content child process. */
> +  readonly attribute nsIMessageSender sender;

Who receives? Can we find a more descriptive name for this?

@@ +37,5 @@
> +   *    The listener object to notify when we have attempted to start the
> +   *    service.
> +   */
> +  void startService(in ACString aContractID,
> +                    in nsIContentStartServiceListener aListener);

startServiceInChild?

@@ +41,5 @@
> +                    in nsIContentStartServiceListener aListener);
> +};
> +
> +[scriptable, function, uuid(e98bec8a-c089-4e25-b2e7-9706cc0ffc63)]
> +interface nsIContentStartServiceListener : nsISupports

nsIStartContentServiceListener?

@@ +62,5 @@
> + */
> +[scriptable, uuid(592d7405-05a0-43c1-96a5-6d2502ef02e7)]
> +interface nsIContentProcessService : nsISupports
> +{
> +  /* Methods for use in parent processes: */

I'm not keen on this dual interface. Maybe we can build on the interfaces being added in bug 890570?

@@ +69,5 @@
> +   * Return an array of nsIContentParent instances representing all currently
> +   * running content child processes.
> +   */
> +  void listChildren([optional] out unsigned long aLength,
> +                    [retval, array, size_is(aLength)] out nsIContentParent aResult);

If there's any chance this could be used by C++ code, I'd rather expose an nsISimpleEnumerator return value instead. xpidl arrays are gross, and leaks of ContentParent objects are killing us in b2g.

::: content/base/src/nsContentProcessService.cpp
@@ +13,5 @@
> +NS_IMPL_CLASSINFO(nsContentProcessService, NULL,
> +                  nsIClassInfo::SINGLETON, NS_CONTENTPROCESSSERVICE_CID)
> +NS_IMPL_ISUPPORTS2_CI(nsContentProcessService,
> +                      nsISupports,
> +                      nsIContentProcessService)

Is the classinfo actually needed here?

@@ +23,5 @@
> +  *aInstancePtr = nullptr;
> +  if (aOuter)
> +    return NS_ERROR_NO_AGGREGATION;
> +
> +  nsRefPtr<nsIContentProcessService> ccs = new nsContentProcessService();

nsCOMPtr or nsContentProcessService

@@ +52,5 @@
> +
> +NS_IMETHODIMP
> +nsContentProcessService::GetContentID(uint64_t *aContentID)
> +{
> +  *aContentID = ~0;

Why?

@@ +67,5 @@
> +nsContentProcessService::Exit()
> +{
> +  if (XRE_GetProcessType() != GeckoProcessType_Content) {
> +    NS_WARNING("nsIContentProcessService.exit is only for use "
> +               "in content chlid processes.");

child

::: content/base/src/nsContentProcessService.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsContentProcessService_h___
> +#define nsContentProcessService_h___

The modern style would be content_base_src_nsContentProcessService_h

::: content/base/test/unit/test_nsIContentProcessService_ids.js
@@ +10,5 @@
> +function run_test () {
> +  var children = [startContentChild(), startContentChild()];
> +  do_check_neq(children[0].contentID, children[1].contentID);
> +
> +  // In each child, check that the child IDs are the sam

same

::: content/base/test/unit/test_nsIContentProcessService_list.js
@@ +61,5 @@
> +      var child3CC = list[1];
> +    } else {
> +      do_check_eq(list[1], child2CC);
> +      var child3CC = list[0];
> +    }

Does indexOf work instead?

::: content/base/test/unit/test_nsIContentProcessService_serviceStatus.js
@@ +37,5 @@
> +          }
> +        }
> +      };
> +      consoleService.registerListener(observer);
> +      child.startService('@mozilla.org/honest-opinion-of-my-tie;1', (result) => {

:D

::: dom/ipc/ContentParent.cpp
@@ +328,4 @@
>  }
>  
>  /*static*/ already_AddRefed<ContentParent>
> +ContentParent::GetNew(bool aForBrowserElement)

I'm not thrilled by this existing, since we don't really want to give people the ability to spawn unlimited numbers of content processes. Why not use GetNewOrUsed and up the process count in tests if necessary?

::: dom/ipc/PStartService.ipdl
@@ +7,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +rpc protocol PStartService

async, not rpc

::: testing/xpcshell/head.js
@@ +1181,5 @@
> +// Given a function |aFunc| that may either return or throw an exception,
> +// return a function that calls |aFunc| with the same 'this' and arguments
> +// it receives, and returns the value |aFunc| returns; but if the call to
> +// |aFunc| throws, passes the exception to do_throw.
> +function failOnThrow(aFunc) {

I found myself stumbling over every use of this function while reading this patch. I'm not sure if it's the name or just that it wraps very basic behaviour that I prefer to be explicit. Not really sure what to do about this, though.

@@ +1203,5 @@
> +// |aOnThrow|: a structured clone of the exception value; the results of
> +// our best efforts to convert the exception to a string; and possibly a
> +// string containing the call stack at which the error was generated. If
> +// |aOnThrow| is omitted, pass an Error instance to do_throw.
> +function evalInChild(aChild, aCode, aOnReturn, aOnThrow) {

do_eval_in_child to match prevailing style.

@@ +1244,5 @@
> +
> +// Tell the child process represented by |aChild|, an nsIContentParent
> +// instance, to quit. If given, call 'aOnQuit' with no arguments when the
> +// child has terminated.
> +function quitChild(aChild, aOnQuit) {

do_quit_child