507218 - Client code needs to know what kind of process it's running in

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

A bunch of client code (networking/prefs/etc) needs to know what 'kind' of process it's running in: chrome, content, plugin (at least for now)...

I'd like a fast binary API,

GeckoChildProcessType XRE_GetProcessType();

And an XPCOM version for script, hung on nsIXULRuntime.

bent, can you mentor?

Comment 1

[Robin Bate Boerop]

Attachment: draft1

I have coded some of my ideas.  Comments?

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 393274 [details] [diff] [review]
draft1

Hi Robin,

I think that we can get away with storing the process type as a simple static rather than creating a new subclass of ChildProcess. Does that make sense?

Comment 3

[Robin Bate Boerop]

(In reply to comment #2)
> I think that we can get away with storing the process type as a simple static
> rather than creating a new subclass of ChildProcess. Does that make sense?

Yes, that is a simpler solution.  I began down that path, but found this one better.

The issue is: where to set the process type.  One option is in ChildProcess, together with the logic for creating the child process singleton.  But, that would require hacking into chromium (ChildProcess is chromium code).

Another option would be to place the logic for setting the process type near the invocation of the ChildProcess constructor, whenever one is invoking that.  In this case, one would have to augment the code with a comment which would say "don't forget to set the process type near when you instantiate ChildProcess".  Clearly, this is error prone, and fails to bundle the logic of creating a new process with the associated necessary tasks (setting the process type).  That would defeat the use of OOP.

I believe that the right place for the code is in a class derived from ChildProcess.

There are other advantages to coding things the way that I have:

1. GeckoChildProcess (the class deriving from ChildProcess that my patch creates) shrinks the number of points at which Mozilla code is dependent on chromium code by removing various references to "child_process.h", "child_thread.h", "base/thread.h", and ChildThread.  In fact, there is only one dependency on child_process.h in the entire codebase, after this patch.
2. GeckoChildProcess, which is more succinct than ChildProcess, makes clear that not all of chromium's ChildProcess functionality is needed.  The code is easier to understand.
3. There is likely to be more logic associated with ChildProcess code, in future development, and that code will want to live in GeckoChildProcess.

What do you think?

Comment 4

[Benjamin Smedberg]

The only entrypoint for starting a child process is XRE_InitChildProcess. Otherwise you can safely assume that it's a chrome process type (make that the default value).

Comment 5

[Robin Bate Boerop]

Attachment: nsIXULRuntime stuff

Is this on the right track for the nsIXULRuntime stuff?  I am new to XPCOM, and worry that I do not understand the choices for strings, yet.  Do we even want a string as a return value for the process type?  Is nsXULAppInfo the right place to implement all of nsIXULRuntime?

Comment 6

[Robin Bate Boerop]

Attachment: draft2

Here's a patch which includes some helpful comments from those on IRC's #content channel.  Comments?

I'm unclear where unit tests for this stuff should go.  Pointers?

Comment 7

[Robin Bate Boerop]

Attachment: Updated draft2

Updated to reflect updated electrolysis tree.

Comment 8

[Robin Bate Boerop]

Attachment: draft 3

Accounts for GeckoChildProcess_TestShell disappearing.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 395081 [details] [diff] [review]
draft 3

>diff -r 601a02ccacf9 ipc/glue/GeckoChildProcess.cpp

Please don't make a new subclass here. I understand your desire to have a clean OOP abstraction but I don't think it buys us anything here.

> 1. GeckoChildProcess (the class deriving from ChildProcess that my patch
> creates) shrinks the number of points at which Mozilla code is dependent on
> chromium code by removing various references to "child_process.h",
> "child_thread.h", "base/thread.h", and ChildThread.  In fact, there is only one
> dependency on child_process.h in the entire codebase, after this patch.

I don't know what you mean by "dependency" here. If you mean "having to type 'child_process.h'" then I guess you're right, but no real dependency is broken as the child_process.h header is still included.

> 2. GeckoChildProcess, which is more succinct than ChildProcess, makes clear
> that not all of chromium's ChildProcess functionality is needed.

Actually since it's a subclass it indicates nothing of the sort. We could be using all of it under the hood and no one would know without reading the code.

> 3. There is likely to be more logic associated with ChildProcess code, in
> future development, and that code will want to live in GeckoChildProcess.

What sort of logic are you thinking of? I can't think of anything else we might need here that we can't get from chromium code already.

>+// Ensure that the GeckoChildProcessType enum, defined in xpcom/build/nsXULAppAPI.h,
>+// is synchronized with the const unsigned longs defined in xpcom/system/nsIXULRuntime.idl.

Please keep all code and comments as close to 80 chars max width as possible.

>+#define SYNC_ENUMS(a,b) \
>+  PR_STATIC_ASSERT(nsIXULRuntime::PROCESS_TYPE_ ## a == \
>+                   static_cast<int>(GeckoChildProcess_ ## b));

Please add an empty line below here to separate the macro definition and use.

>+nsXULAppInfo::GetProcessType(PRUint32* aResult)
>+{

Please add an NS_ENSURE_ARG_POINTER(aResult) before derefing aResult.

>-    ChildThread* mainThread;
>+    GeckoThread* mainThread;

This should be unnecessary once the new subclass is gone.

>+GeckoChildProcessType XRE_GetProcessType()

Prevailing style is to have return type on its own line followed by the function name. Please follow that.

>+XRE_API(GeckoChildProcessType,
>+        XRE_GetProcessType, ())

One hidden gotcha: When adding public functions like this we need to also add a call to it in toolkit/library/dlldeps-xul.cpp so that the windows linker doesn't strip it out in some cases.


>+  unsigned long getProcessType();

Let's make this |readonly attribute unsigned long processType| to make the JS reflection a little nicer. The C++ shouldn't change.

Comment 10

[Robin Bate Boerop]

Attachment: After bent's comments

Comment 11

[Benjamin Smedberg]

Comment on attachment 395123 [details] [diff] [review]
After bent's comments

The default should be GeckoProcessType_Default, not GeckoProcessType_Invalid... that way code entering through XRE_main or XRE_InitEmbedding has the correct type.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Agreed. Also, please revert these two changes:

@@ -255,7 +257,7 @@
   MessageLoopForIO mainMessageLoop;
 
   {
-    ChildThread* mainThread;
+    GeckoThread* mainThread;
 
     switch (aProcess) {
     case GeckoChildProcess_Default:
@@ -276,8 +278,10 @@
 
     default:
       NS_RUNTIMEABORT("Unknown main thread class");
+      mainThread = 0;
     }

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 395123 [details] [diff] [review]
After bent's comments

Oh, and you need to make the changes to dlldeps-xul.cpp that I mentioned above.

Comment 14

[Robin Bate Boerop]

(In reply to comment #11)
> The default should be GeckoProcessType_Default, not GeckoProcessType_Invalid...
> that way code entering through XRE_main or XRE_InitEmbedding has the correct
> type.

I think that I was confused by the "GeckoChildProcessType" enum having the word "Child" in it.  You want XRE_GetProcessType() to make sense even when the process is not a "child" process, right?

I thought otherwise, hence the patches with the ChildProcess singleton, etc.

May I change GeckoChildProcessType to "GeckoProcessType"?

Comment 15

[Benjamin Smedberg]

less churn is probably better. And yeah, the XRE_GetProcessType needs to be accurate for whatever kind of process we're in.

Comment 16

[Robin Bate Boerop]

Attachment: Changing GeckoChildProcessType to GeckoProcessType

After discussion on #content, renamed GeckoChildProcessType enum to "GeckoProcessType".

Made other changes suggested by bent, above.

I'm unsure of whether some of this new code belongs inside "#ifdef MOZ_IPC".  Also, there are no unit tests, yet.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 395158 [details] [diff] [review]
Changing GeckoChildProcessType to GeckoProcessType

This looks great! r=bent

Comment 18

[Benjamin Smedberg]

http://hg.mozilla.org/projects/electrolysis/rev/d906997755b8

Woot, on to greater things!