480427 - Add a way to run processes in a background thread

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Dave Townsend [:mossop]]

nsIProcess blocks the main thread if we want to know when it is finished. We need a simple component to do this in the background and notify when complete.

Comment 1

[Dave Townsend [:mossop]]

There are issues calling OSX executables without arguments without the patch in bug 442393.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch adds a new XPCOM component that will run a process on a background thread, notifying when the process has finished. It's currently specifically written for the PFS usage so I've lefts it under the pfs directory, not sure if we should just put it into xpcom, but I have the intention of seeing something better implemented there anyway so this just might go away in the future.

Includes basic tests for argument passing and exit code getting.

Comment 3

[Robert Strong (they/them - no direct email)]

When I talked about doing this with a couple of people it was suggested that instead of handing off a file it would be better to hand off a string since nsIFile and friends aren't thread safe. It would probably be better to get someone else to review this from a Mozilla threadsafe perspective such as bent or bsmedberg.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 366529 [details] [diff] [review]
patch rev 1

Ok sure. The actual handoff of the file is done by string, internally it passes the persistantDescriptor for the file to the background thread so there shouldn't be any thread safety issues. I've run this in a debug build and seen no assertions from it.

Comment 5

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

Comment on attachment 366529 [details] [diff] [review]
patch rev 1

I'm not a toolkit peer but I can give some feedback nonetheless. This looks great, just a few small issues:

>+ProcessRun::ProcessRun(nsACString& aDescriptor, const char **aArgs, PRUint32 aCount,
> ...
>+    mArgs = (char**)NS_Alloc(sizeof(char*) * mCount);
>+    for (PRUint32 i = 0; i < mCount; i++) {
>+        int len = strlen(aArgs[i]) + 1;
>+        mArgs[i] = (char*)NS_Alloc(len);
>+        memcpy(mArgs[i], aArgs[i], len);
>+    }

In general I'm not a fan of doing fallible stuff like this in constructors (this will crash when OOM)... Maybe split out into an Init method? I can't wait for the day when malloc either work or aborts...

>+nsresult ProcessRun::Execute(PRInt32 *exitCode)
> ...
>+    process->GetExitValue(exitCode);

Why wouldn't you also check to see that this call succeeded?

>+nsProcessRunner::nsProcessRunner()
>+nsProcessRunner::~nsProcessRunner()

Kill these?

>+NS_IMETHODIMP nsProcessRunner::Run(const char **args, PRUint32 count,
> ...
>+    nsCOMPtr<nsIRunnable> runner = new ProcessRun(desc, args, count, proxy);

You're not checking that runner is non-null.

>+++ b/toolkit/mozapps/plugins/test/TestProcess.cpp

I don't know what the policy is for simple test files, should there be a license block in this too?

>+++ b/toolkit/mozapps/plugins/test/unit/test_bug480427.js
> ...
>+  if ("@mozilla.org/windows-registry-key;1" in Cc)

Hm... sneaky! Is there really no better way to determine that we're running on windows?

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 366529 [details] [diff] [review]
patch rev 1

(In reply to comment #5)
> >+nsresult ProcessRun::Execute(PRInt32 *exitCode)
> > ...
> >+    process->GetExitValue(exitCode);
> 
> Why wouldn't you also check to see that this call succeeded?

I'm a little unsure what to do at that point. If nsIProcess.run has succeeded but we fail to get an exit code, what conclusion should we draw from that? Did the process complete? Probably. Which means that the exit code is just unknown. So the callback should hear that the process completed, just not sure what to tell it about the exit code really.

> >+++ b/toolkit/mozapps/plugins/test/TestProcess.cpp
> 
> I don't know what the policy is for simple test files, should there be a
> license block in this too?

Probably, just an oversight on my part.

> >+++ b/toolkit/mozapps/plugins/test/unit/test_bug480427.js
> > ...
> >+  if ("@mozilla.org/windows-registry-key;1" in Cc)
> 
> Hm... sneaky! Is there really no better way to determine that we're running on
> windows?

Well that is the documented way on MDC, I'd love to hear of a better way though.

I'll take care of these changes and submit a new patch for review.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Addresses Ben's points.

Comment 8

[Dave Townsend [:mossop]]

I'm suddenly wondering if this is even necessary. Now nsIProcess has the isRunning attribute, we could potentially just run a small event loop in js until the process has ended...

Comment 9

[Benjamin Smedberg]

Spinning a JS busy-wait event loop seems silly if we can do it reliably and get an immediate answer when the process is done.

Comment 10

[Benjamin Smedberg]

In terms of API, what I'd like the best is:

nsIProcess.run(boolean blocking,
               [array, size_is(count)] in string args, in unsigned long count,
               in nsIObserver aObserver, in boolean aHoldWeak);

/**
 @param aObserver When blocking is false, an observer which will be notified when
                  the process is finished running. It will be notified with
                  subject <the process object>, topic "process-finished", data null.
                  aObserver may be null if no notification is necessary.

 @param aHoldWeak Hold aObserver via a weak reference.
 */

The things I'm particularly concerned about are:

* What if you quit Firefox while the process is still running? We need to make sure that the observer is released during xpcom-shutdown and doesn't cause leaks (which can often cause crashes if they entrain XPConnect stuff)

* I think it would be much easier to implement this as a worker thread as part of nsProcess itself, so you could just launch the worker thread and call WaitForSingleObject/PR_WaitProcess from that thread, and detach the thread if shutdown happens.

Comment 11

[Benjamin Smedberg]

Comment on attachment 367078 [details] [diff] [review]
patch rev 2

If you need a hack just for PFS in 1.9.1, I'd prefer a JS component.

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This approach uses a background monitor thread to wait until the process finishes when run in non-blocking mode. This simplifies the nsProcess code quite a bit since we get the exit code on completion and know if the thread exists then the process is still running.

Nothing has changed in terms of running and killing the process, just moved around a bit. Tests are added to verify that we get notified after the process has completed.

I've also taken the opportunity to add some better comments to the nsIProcess interface definition.

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch rev 4

This just fixes the tests from Ted's uber changes

Comment 14

[Benjamin Smedberg]

Comment on attachment 368866 [details] [diff] [review]
patch rev 4

>diff --git a/xpcom/threads/nsIProcess.idl b/xpcom/threads/nsIProcess.idl

> [scriptable, uuid(d573f1f3-fcdd-4dbe-980b-4ba79e6718dc)]

On trunk we don't need nsIProcess2, and I'd prefer to avoid it long-term. If it helps to keep this patch branch-ready short-term, that's fine.

The API comments below can be pushed to later:

init() says you can run the same process twice. Does anyone do that? I certainly didn't know that and didn't expect it. Can we break that and require each process to get their own object?

>+  /**
>+   * Initialises the process with an already running process identifier.
>+   * This is not currently supported on any platform.
>+   * @param pid        The process identifier of an existing process.
>+   */
>+  void initWithPid(in unsigned long pid);

If it's not supported, why bother? Let's leave this for a future patch.

>+  /** XXX what charset? **/

The "native" charset. AFAIK there is no way for JS to convert to this charset, though :-(

>+  /**
>+   * The process identifier of the currently running process.
>+   */
>+  readonly attribute unsigned long pid;

Document that this may not always be available, i.e. on Win2k and WinXP pre-SP1

>+  /**
>+   * The name of the currently running process.
>+   * Not currently supported on any platform.
>+   */
>+  readonly attribute string processName;

Leave it out? I don't even know what a process "name" is...

>+  readonly attribute unsigned long processSignature;

ditto

>+[scriptable, uuid(7d362c71-308e-4724-b1eb-8451fe133026)]
>+interface nsIProcess2 : nsIProcess
>+{
>+  /** XXX what charset? **/
>+  /** Executes the file this object was initialized with optionally calling
>+   * an observer after the process has finished running.
>+   * @param blocking   Whether to wait until the process terminates before
>+                       returning or not.
>+   * @param args       An array of arguments to pass to the process.
>+   * @param count      The length of the args array.
>+   * @param observer   An observer to notify when the process has completed. It
>+   *                   will receive this process instance as the subject and
>+   *                   "process-finished" or "process-failed" as the topic. The
>+   *                   observer will be notified on the main thread.
>+   * @param holdWeak   Whether to use a weak reference to hold the observer.
>+   */
>+  void runAndNotify(in boolean blocking, [array, size_is(count)] in string args,
>+                    in unsigned long count, [optional] in nsIObserver observer,
>+                    [optional] in boolean holdWeak);

Why not runAsync and skip the `blocking` param?

What's the default value of an optional boolean?

>diff --git a/xpcom/threads/nsProcess.h b/xpcom/threads/nsProcess.h

>+  nsCOMPtr<nsIObserver> mObserver;
>+  nsWeakPtr mWeakObserver;

You could probably save a word with this somehow, but this is probably good enough since we don't create many processes.

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp

> //Constructor
> nsProcess::nsProcess()
>-    : mExitValue(-1),
>+    : mThread(nsnull),
>+      mPid(-1),
>+      mObserver(nsnull),
>+      mWeakObserver(nsnull),
>+      mExitValue(-1),
>       mProcess(nsnull)
> {
>+    mLock = PR_NewLock();

nit, make this an initializer also?

> //Destructor
> nsProcess::~nsProcess()
> {
>-#if defined(PROCESSMODEL_WINAPI)
>-    if (mProcess)
>-        CloseHandle(mProcess);
>-#else
>-    if (mProcess) 
>-        PR_DetachProcess(mProcess);
>-#endif
>+    PR_DestroyLock(mLock);

If we shut down while we're waiting on a process, we're guaranteed to leak the nsProcess (because it's being held by the thread). I'm probably ok with that, as long as we don't leak anything else (including any strong observers held in mObserver).

>+void PR_CALLBACK nsProcess::Monitor(void *arg)
>+{
>+    nsRefPtr<nsProcess> process = static_cast<nsProcess*>(arg);

I think you have a race here: if the nsProcess::Run call returns and the caller immediately releases the process object, it could delete itself before this threadproc is run. I think you need to pass an addrefed pointer to this method and then use dont_AddRef here.

>+#if defined(PROCESSMODEL_WINAPI)
>+    DWORD dwRetVal;
>+    unsigned long exitCode = -1;
>+
>+    PR_Lock(process->mLock);
>+    HANDLE handle = process->mProcess;

mProcess isn't mutable; I don't think you don't need a lock here. If you do, please use a scoped nsAutoLock (because we're trying to change the nsAutoLock API and avoid NSPR).

>+    PR_Lock(process->mLock);
>+    CloseHandle(handle);
>+    process->mProcess = NULL;

If mProcess is in fact mutable, you've just nulled out a potentially-different handle than the one which you just closed. 

>+    PR_Unlock(process->mLock);
>+
>+    // If we ran a background thread for the monitor then notify on the main
>+    // thread
>+    if (NS_IsMainThread()) {
>+        process->ProcessComplete();
>+    }
>+    else {
>+        nsCOMPtr<nsIRunnable> event = new nsRunnableMethod<nsProcess>(process, &nsProcess::ProcessComplete);
>+        NS_DispatchToMainThread(event);
>+    }

If XPCOM has shut down, we should skip these hunks and early-return... the main thread may be gone, or the component manager may have already shut down: NS_DispatchToMainThread is unsafe.

>+void nsProcess::ProcessComplete()
>+{
>+    if (mThread) {
>+        nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1");
>+        if (os)
>+            os->RemoveObserver(this, "xpcom-shutdown");
>+    }
>+
>+    char* topic;
>+    PR_Lock(mLock);
>+    if (mExitValue < 0)
>+        topic = "process-failed";
>+    else
>+        topic = "process-finished";
>+    PR_Unlock(mLock);

again, not sure why locking is necessary: there's no shared-mutability, right?

>+    mThread = nsnull;

The only way to avoid leaking here is to do PR_JoinThread.

>+    else {
>+        mThread = PR_CreateThread(PR_SYSTEM_THREAD, Monitor, this,
>+                                  PR_PRIORITY_NORMAL, PR_LOCAL_THREAD,
>+                                  PR_UNJOINABLE_THREAD, 0);

I know I told you to use an unjoinable thread... but I think you actually want a joinable thread (so that you can free it using PR_JoinThread as noted above). My bad.

Comment 15

[Dave Townsend [:mossop]]

Attachment: patch rev 5

(In reply to comment #14)
> (From update of attachment 368866 [details] [diff] [review])
> >diff --git a/xpcom/threads/nsIProcess.idl b/xpcom/threads/nsIProcess.idl
> 
> > [scriptable, uuid(d573f1f3-fcdd-4dbe-980b-4ba79e6718dc)]
> 
> On trunk we don't need nsIProcess2, and I'd prefer to avoid it long-term. If it
> helps to keep this patch branch-ready short-term, that's fine.

Yeah I'll aim this patch at being totally branch worthy, and post a second patch to remove nsIProcess2 and the unused API bits from trunk.

> init() says you can run the same process twice. Does anyone do that? I
> certainly didn't know that and didn't expect it. Can we break that and require
> each process to get their own object?

I guess I don't know of anyone doing it. I've made Init fail if it was initialized before.

> What's the default value of an optional boolean?

false.

> > //Destructor
> > nsProcess::~nsProcess()
> > {
> >-#if defined(PROCESSMODEL_WINAPI)
> >-    if (mProcess)
> >-        CloseHandle(mProcess);
> >-#else
> >-    if (mProcess) 
> >-        PR_DetachProcess(mProcess);
> >-#endif
> >+    PR_DestroyLock(mLock);
> 
> If we shut down while we're waiting on a process, we're guaranteed to leak the
> nsProcess (because it's being held by the thread). I'm probably ok with that,
> as long as we don't leak anything else (including any strong observers held in
> mObserver).

The xpcom-shutdown observer should clear out most of the stuff that nsProcess is hanging onto. The only thing that we will leak is the nsProcess and any PRProcess or windows HANDLE. I can't see an obvious way around that though. I know windows will close the HANDLE for us on shutdown.

> >+    PR_Lock(process->mLock);
> >+    CloseHandle(handle);
> >+    process->mProcess = NULL;
> 
> If mProcess is in fact mutable, you've just nulled out a potentially-different
> handle than the one which you just closed. 

Kill will touch mProcess so I think I do need to lock closing it up.

> >+    PR_Unlock(process->mLock);
> >+
> >+    // If we ran a background thread for the monitor then notify on the main
> >+    // thread
> >+    if (NS_IsMainThread()) {
> >+        process->ProcessComplete();
> >+    }
> >+    else {
> >+        nsCOMPtr<nsIRunnable> event = new nsRunnableMethod<nsProcess>(process, &nsProcess::ProcessComplete);
> >+        NS_DispatchToMainThread(event);
> >+    }
> 
> If XPCOM has shut down, we should skip these hunks and early-return... the main
> thread may be gone, or the component manager may have already shut down:
> NS_DispatchToMainThread is unsafe.

Couldn't see an obvious way to know we were shutting down except by adding an mShutdown flag that we set in xpcom-shutdown.

Comment 16

[Dave Townsend [:mossop]]

Attachment: api changes rev 1

These are the API fixes for trunk, chances are I'll just land this in the same push there.

Comment 17

[Dave Townsend [:mossop]]

Attachment: patch rev 6 [checked in]

Just realised that the location attribute doesn't do what I thought either so marked that for removal.

Comment 18

[Dave Townsend [:mossop]]

Attachment: api changes rev 2

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 369077 [details] [diff] [review]
patch rev 6 [checked in]

Landed the main patch on trunk: http://hg.mozilla.org/mozilla-central/rev/c352ad4174bb

Comment 20

[Nickolay_Ponomarev]

Summary for documentation update:
- this change (if checked in on 1.9.1) adds nsIProcess2 interface on branch with the new functionality (runAsync).
- updates doc comments on various nsIProcess. Current documentation on MDC is deceiving in some cases.
- On trunk merges nsIProcess2 into nsIProcess and removes unimplemented methods/attributes.

https://developer.mozilla.org/en/nsIProcess should be updated.

Dave, please correct me if I read the patch wrong.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> Summary for documentation update:
> - this change (if checked in on 1.9.1) adds nsIProcess2 interface on branch
> with the new functionality (runAsync).
> - updates doc comments on various nsIProcess. Current documentation on MDC is
> deceiving in some cases.
> - On trunk merges nsIProcess2 into nsIProcess and removes unimplemented
> methods/attributes.
> 
> https://developer.mozilla.org/en/nsIProcess should be updated.

Yep that is spot on

Comment 22

[Ginn Chen]

Fix bustage for Solaris
http://hg.mozilla.org/mozilla-central/rev/dd45bf1d8798

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 369077 [details] [diff] [review]
patch rev 6 [checked in]

Would like approval to land this on the branch, it will improve the UX of installing plugins through the plugin installer service. It introduces a new interface but otherwise is not an API change. The component is pretty well tested now too so I don't anticipate any regressions. Any approval needs to apply to the solaris follow-up fix too.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 369077 [details] [diff] [review]
patch rev 6 [checked in]

a191=beltzner for this and the solaris followup

Comment 25

[Dave Townsend [:mossop]]

Landed on branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8df9044446d3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7e60b3b8e711

Comment 26

[Dave Townsend [:mossop]]

Landed the API changes on trunk: http://hg.mozilla.org/mozilla-central/rev/817957a7c933