1400294 - ShellExecute might be a performance disaster

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[(no longer active)]

nsProcess::Run/RunAsync uses ShellExecute, but that may be a mistake.  See this profile for example: https://perfht.ml/2f0Oen7

This function does all sorts of stuff, like delay loading DLLs, etc.  We call it for example from runMinidumpAnalyzer(), so the first time this function gets called on the UI thread, things are going to be super janky.

Kan-Ru, is this the kind of thing that your team would be interested to take on?

Comment 1

[(no longer active)]

The profile comes from bug 1397092 comment 156.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

The nsIProcess implementation is ancient, it could use a more modern implementation but nobody has touched it in years AFAIK. No hurt in modernizing it.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Not using ShellExecute might also help with bug 1393716 and friends, whatever it's doing on slow clients with AV installed it can get stuck for over a minute in the kernel.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Sorry I meant bug 1386760.

Comment 5

[(no longer active)]

Note that ShellExecute does useful things also, such as handling things like CreateProcess failing due to a UAC prompt, so if we switch away from it we should handle those cases ourselves...

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Cervantes, can you take a look?

Comment 7

[Cervantes Yu [:cyu] [:cervantes]]

I can look into this.

It looks to me that many use cases (if not all) of nsIProcess can be replaced by CreateProcess(). Where do we ShellExecuteExW that requires UAC? How about we CreateProcess() at first and fallback to ShellExeciteExW if it fails due to a UAC prompt or something else.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #7)
> It looks to me that many use cases (if not all) of nsIProcess can be
> replaced by CreateProcess(). Where do we ShellExecuteExW that requires UAC?

IIRC launch the updater using nsIProcess and that might require UAC so it's worth checking it out. On the other hand you shouldn't encounter any issues for the minidump-analyzer and the pingsender. In fact the minidump-analyzer was originally launched using CreateProcess(). I switched it to using nsIProcess because it allowed the content crash reporting flow to be written in JavaScript instead of a tangled mess of platform-dependent C++.

Comment 9

[(no longer active)]

Wine's ShellExecuteEx implementation may be a good point to start: <https://github.com/wine-mirror/wine/blob/07cf14dc928a1a00baecbbc7ca5a6f3fe680238c/dlls/shell32/shlexec.c#L308>  Note that it doesn't have UAC handling, etc.

Comment 10

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Add 'useProcess' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

This switches the ShellExecuteExW() calls to CreateProcess() in screenshotting, telemetry sending, and crash analysis, that we are sure to be launching executables instead of shell services.

Comment 11

[Cervantes Yu [:cyu] [:cervantes]]

My local profiles show that minidump-analyzer.exe takes 13.44 ms to launch using ShellExecuteExW(), or 1.3ms to launch using CreateProcess().

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8911710 [details] [diff] [review]
Add 'useProcess' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

I've had a quick look at your patch and it seems sane overall - and the performance gain would really be helpful on browser shutdown - but my Windows experience is limited and I'm not an XPCOM peer. I think it would be better if somebody with more Windows experience would review it rather than me.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8911710 [details] [diff] [review]
Add 'useProcess' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

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

This seems pretty reasonable.  Did we decide to not fallback (as comment 7 suggests) if CreateProcess fails?

How confident are we that the screenshot executable won't require a UAC prompt?  Comment 8 indicates that the minidump-analyzer and pingsender should not raise UAC prompts, but I don't see discussion around the screenshot executable.  Please talk to the screenshots people, or revert that change before landing.

I am not a Windows expert either, please get one to review the changes.  I've tagged aklotz and dmajor for reviews--only one of you needs to look at this--but if they're both busy, perhaps rstrong or somebody else?

r=me assuming that we are really confident around the UAC issue and not falling back to use ShellExecute.

::: xpcom/threads/nsProcessCommon.cpp
@@ +491,4 @@
>  
> +    PROCESS_INFORMATION processInfo;
> +    retVal = CreateProcess(NULL,
> +                           cmdLine, NULL, NULL,

The MSDN page has security remarks about CreateProcess:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx

saying that we shouldn't pass NULL for the application name (the first parameter).  I *think* assembleCmdLine quotes things appropriately?

Comment 14

[Marco Castelluccio [:marco]]

(In reply to Nathan Froyd [:froydnj] from comment #13)
> How confident are we that the screenshot executable won't require a UAC
> prompt?  Comment 8 indicates that the minidump-analyzer and pingsender
> should not raise UAC prompts, but I don't see discussion around the
> screenshot executable.  Please talk to the screenshots people, or revert
> that change before landing.

I think the screenshot executable is only used during testing, so not sure it's worth optimizing launching it.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8911710 [details] [diff] [review]
Add 'useProcess' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

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

Not a big fan of "useProcess" as the attribute name since ShellExecute also creates a new process under the hood. "useCreateProcess" or "noShell" or something would be nicer IMHO, but I'll defer to Nathan on that.

r=me once my other comments are addressed.

::: xpcom/threads/nsProcessCommon.cpp
@@ +470,5 @@
>    wchar_t* cmdLine = nullptr;
>  
>    // |aMyArgv| is null-terminated and always starts with the program path. If
>    // the second slot is non-null then arguments are being passed.
> +  if (aMyArgv[1]) {

This check isn't quite right for the mUseProcess = true case. Even when there are no command line arguments, you still need to include argv[0] in the command line parameter. So this condition should probably be changed to:

if (aMyArgv[1] || mUseProcess)

@@ +491,4 @@
>  
> +    PROCESS_INFORMATION processInfo;
> +    retVal = CreateProcess(NULL,
> +                           cmdLine, NULL, NULL,

+1. Please don't pass NULL for the first parameter to CreateProcess. Take the instantiation of wideFile from the ShellExecute case and put it above the mUseProcess if statement, then you can pass wideFile as the first parameter.

@@ +505,5 @@
> +
> +    mProcess = processInfo.hProcess;
> +  } else {
> +    // The program name in aMyArgv[0] is always UTF-8
> +    NS_ConvertUTF8toUTF16 wideFile(aMyArgv[0]);

As I mentioned above, move this so that it is accessible to both the if block and the else block.

Comment 16

[(Away)]

Comment on attachment 8911710 [details] [diff] [review]
Add 'useProcess' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

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

Looking good but I'd like to get an answer to the argv[0] question before I sign off.

::: browser/tools/mozscreenshots/mozscreenshots/extension/Screenshot.jsm
@@ +83,5 @@
>          exe.append("screenshot.exe");
>        }
>        let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
>        process.init(exe);
> +      process.useProcess = true;

The name `useProcess` isn't super clear because ShellExecute creates a process too, and it looks awkward in the context of `process.useProcess` -- but I don't really have a better idea.

::: xpcom/threads/nsProcessCommon.cpp
@@ +470,5 @@
>    wchar_t* cmdLine = nullptr;
>  
>    // |aMyArgv| is null-terminated and always starts with the program path. If
>    // the second slot is non-null then arguments are being passed.
> +  if (aMyArgv[1]) {

I think this won't work if mUseProcess is true and we only pass an executable name with no params.

@@ +473,5 @@
>    // the second slot is non-null then arguments are being passed.
> +  if (aMyArgv[1]) {
> +    // Pass the executable path as argv[0] to the launched program when calling
> +    // CreateProcess().
> +    char** argv = mUseProcess ? aMyArgv : aMyArgv + 1;

I'm curious, why not pass argv[0] as the first parameter to CreateProcess? Then we wouldn't have this inconsistency of whether to pass aMyArgv or aMyArgv to assembleCmdLine. I only skimmed the CreateProcess documentation so maybe I missed some detail.

@@ +493,5 @@
> +    retVal = CreateProcess(NULL,
> +                           cmdLine, NULL, NULL,
> +                           /* bInheritHandles = */ FALSE,
> +                           /* dwCreationFlags = */ 0,
> +                           NULL, NULL,

For consistency (and because it's hard to understand these APIs with a ton of params) I think it would be good to have `/* foo = */` for all parameters, one per line.

@@ +538,2 @@
>    if (cmdLine) {
>      free(cmdLine);

This is a pre-existing problem, but we will leak `cmdLine` if we take an early return before this.

Comment 17

[(Away)]

Comment on attachment 8911710 [details] [diff] [review]
Add 'useProcess' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

Oops. Aaron and I collided!

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

;-)

Comment 19

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Add 'noShell' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

Thanks for the reviews. Here is the updated patch. Changes:

* Rename the attribute to noShell.
* if (aMyArgv[1] || mNoShell) { ...
* Pass argv[0] to application name in CreateProcess(). We escape the command line arguments, but it doesn't hurt if we repeat argv[0] in argument lpApplicationName of CreateProcess().
* Remove the use in Screenshot.jsm since it's test-only.
* Comment the arguments of CreateProcess().
* Use UniquePtr to free cmdLine.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ad340e2cd167551503bc5389258350450ba8ce2

Comment 20

[(Away)]

Comment on attachment 8914276 [details] [diff] [review]
Add 'noShell' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

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

::: xpcom/threads/nsProcessCommon.cpp
@@ +473,5 @@
> +    void operator()(void* p) {
> +      free(p);
> +    }
> +  };
> +  UniquePtr<wchar_t, FreePolicy> cmdLine;

Would UniqueFreePtr work here?

Comment 21

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to David Major [:dmajor] from comment #20)
> > +  };
> > +  UniquePtr<wchar_t, FreePolicy> cmdLine;
> 
> Would UniqueFreePtr work here?

Yes, we can use this handy class.

Comment 22

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Add 'noShell' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows.

Update: use UniqueFreePtr

Comment 23

[Cervantes Yu [:cyu] [:cervantes]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e484df87b3ee915f29b0f37f47e045f7e1f092cd
Bug 1400294 - Add 'noShell' attribute to nsIProcess to use CreateProcess() for launching a process that doesn't require shell service on Windows. r=froydnj,r=aklotz,r=dmajor

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e484df87b3ee