Closed Bug 507218 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: IPC, defect)

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: moz)

Details

Attachments

(1 file, 6 obsolete files)

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?
Attached patch draft1 (obsolete) — Splinter Review
I have coded some of my ideas.  Comments?
Attachment #393274 - Flags: review?(bent.mozilla)
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?
Attachment #393274 - Flags: review?(bent.mozilla) → review-
(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?
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).
Attached patch nsIXULRuntime stuff (obsolete) — Splinter Review
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?
Attached patch draft2 (obsolete) — Splinter Review
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?
Attachment #393274 - Attachment is obsolete: true
Attachment #394822 - Attachment is obsolete: true
Attached patch Updated draft2 (obsolete) — Splinter Review
Updated to reflect updated electrolysis tree.
Attachment #394894 - Attachment is obsolete: true
Attached patch draft 3 (obsolete) — Splinter Review
Accounts for GeckoChildProcess_TestShell disappearing.
Attachment #394940 - Attachment is obsolete: true
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.
Attachment #395081 - Flags: review-
Attached patch After bent's comments (obsolete) — Splinter Review
Attachment #395081 - Attachment is obsolete: true
Attachment #395123 - Flags: review?(bent.mozilla)
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.
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 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.
Attachment #395123 - Flags: review?(bent.mozilla) → review-
(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"?
less churn is probably better. And yeah, the XRE_GetProcessType needs to be accurate for whatever kind of process we're in.
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.
Attachment #395123 - Attachment is obsolete: true
Attachment #395158 - Flags: review?(bent.mozilla)
Attachment #395158 - Flags: review?(bent.mozilla) → review+
Comment on attachment 395158 [details] [diff] [review]
Changing GeckoChildProcessType to GeckoProcessType

This looks great! r=bent
http://hg.mozilla.org/projects/electrolysis/rev/d906997755b8

Woot, on to greater things!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.