Closed Bug 464194 Opened 12 years ago Closed 12 years ago

ShellExecuteW and SHParseDisplayName do not exist on wince

Categories

(Core Graveyard :: File Handling, defect)

ARM
Windows CE
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: blassey, Assigned: blassey)

Details

(Keywords: fixed1.9.1, mobile)

Attachments

(1 file, 7 obsolete files)

ShellExecuteEx is an appropriate substitute for ShellExecuteW.  We can skip SHParseDisplayName and go right to the fall back.
Attachment #347474 - Flags: review?(cbiesinger)
Comment on attachment 347474 [details] [diff] [review]
uses ShellExecuteEx and skills SHParseDisplayName

>+        int result = (int) ::ShellExecuteEx(&seinfo);
>         // Returns a value greater than 32 if successful. See msdn.
>         if (result > 32)
>           return NS_OK;

ShellExecuteEx, unlike ShellExecute, returns a BOOL.

>+      int r = (int) ::ShellExecuteEx(&seinfo);
>       if (r < 32)
>         rv = NS_ERROR_FAILURE;

Same as above. The existing code is already wrong, btw, it should be |if (r <= 32)|.
Also, windows ce doesn't have the A/W version of ShellExecuteEx or SHELLEXECUTEINFO, so the ambiguous one must be used.  That necessitates the use of the TEXT() macro and a conversion conditional on UNICODE being defined.

Also note, this is apparently contrary to the docs on msdn (http://msdn.microsoft.com/en-us/library/ms942612.aspx) which specify that the params are narrow whether or not UNICODE is defined.  When attempting to set lpFile, lpParameters or lpVerb to a narrow string on windows ce, you get a compilation error though.

I have built with this patch on both windows ce and windows xp and it builds without warning or error.
Assignee: nobody → blassey
Attachment #347474 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #347496 - Flags: review?(cbiesinger)
Attachment #347474 - Flags: review?(cbiesinger)
Comment on attachment 347496 [details] [diff] [review]
checks bool ret val, switches on GetLastError

>+        seinfo.lpParameters =  NS_ConvertUTF16toUTF8(args).get();
Do not use NS_ConvertUTF16toUTF8 to convert non-ascii chars.
Windows narrow API will never expect UTF-8 string.
You should use ShellExecuteExW explicitly on desktop Windows. Otherwise, non-ascii args will be broken.
Note that UNICODE preprocessor macro is not yet defined on desktop Windows.
What about
#ifdef WINCE
#define SHELLEXECUTEINFOW SHELLEXECUTEINFO
#define ShellExecuteExW(seinfo) ShellExecuteEx((seinfo))
#endif
or similar trick?
Alternatively, could convert with WideCharToMultiByte to convert to the active code page and make sure windows likes the argument.
Narrowing conversion is lossy. For example, U+00A5 (yen sign) will be converted into 0x5C (backslash) on Japanese locale. It may lead a directory traversal.
Also, some indic scripts are not supported at all by Windows codepages.
Attached patch updated based on emk's comments (obsolete) — Splinter Review
Attachment #347496 - Attachment is obsolete: true
Attachment #348723 - Flags: review?(benjamin)
Attachment #347496 - Flags: review?(cbiesinger)
Comment on attachment 348723 [details] [diff] [review]
updated based on emk's comments

>+        switch (GetLastError()) {

This is wrong, GetLastError doesn't return SE_ERR_ codes. Some of the shell errors happen to have the same numerical value as the system errors, so you'd get away with it sometimes, but some differ, f.e. SE_ERR_SHARE vs. ERROR_SHARING_VIOLATION.

If you want to keep the changes minimal, you could use 
switch ((int)seinfo.hInstApp)

>-    HMODULE hDll = ::LoadLibraryW(L"shell32.dll");
>+    hDll = ::LoadLibraryW(L"shell32.dll");

Pre-existing code, I know, but why LoadLibrary instead of GetModuleHandle? If shell32.dll wasn't loaded somehow, how can we call ShellExecuteEx?

>     // Version 6.0 and higher
>     if (pMySHParseDisplayName = 
>           (MySHParseDisplayName)::GetProcAddress(hDll,
>                                                  "SHParseDisplayName")) {

So with this patch there is a lot of overlap in the shell32 < 6.0 and >= 6.0 branches, except that the >= 6.0 calls SHParseDisplayName. It would be nice if the identical parts in those branches could be moved before the if statement, but at least this code should be consistent: 
 - one uses sinfo, the other seinfo
 - one checks only the return value of ShellExecuteEx, the other additionally (and wrongly) sinfo.hInstApp
 - the flags in fMask are different. Why would we want UI in one case but not the other?
(In reply to comment #8)

> >-    HMODULE hDll = ::LoadLibraryW(L"shell32.dll");
> >+    hDll = ::LoadLibraryW(L"shell32.dll");
> 
> Pre-existing code, I know, but why LoadLibrary instead of GetModuleHandle? If
> shell32.dll wasn't loaded somehow, how can we call ShellExecuteEx?

The only reason I can think of is if, for some reason, the code above it changes and we don't load shell32.dll it would be an incredibly hard bug to track down.
Attached patch updated based on comments (obsolete) — Splinter Review
Attachment #348723 - Attachment is obsolete: true
Attachment #349364 - Flags: review?(benjamin)
Attachment #348723 - Flags: review?(benjamin)
Attachment #349364 - Flags: review?(benjamin) → review+
This was backed out on suspicion of causing orange on both make check and mochitests while running exthandler-related tests:
http://hg.mozilla.org/mozilla-central/rev/eec0447b4117
http://hg.mozilla.org/mozilla-central/rev/db6ff8e0afb8
Specifically, the failure was caused by:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80520003 (NS_ERROR_FILE_EXECUTION_FAILED) [nsILocalHandlerApp.launchWithURI]"  nsresult: "0x80520003 (NS_ERROR_FILE_EXECUTION_FAILED)"  location: "JS frame :: http://localhost:8888/tests/uriloader/exthandler/tests/mochitest/handlerApps.js :: test :: line 110"  data: no]

while running http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml .
Comment on attachment 349364 [details] [diff] [review]
updated based on comments

Although it is not related to the bustage, I'll have to say...
You must not fallback without SEE_MASK_INVOKEIDLIST when SHParseDisplayName fails 
because it will cause bug 394974 again. See bug 394974 comment #25 for details.
(In reply to comment #13)
> Specifically, the failure was caused by:

Sorry, this turns out to have been caused by bug 455381. This bug is unrelated to the test failures as far as I can tell.
Masatoshi, do these flags look correct to you?
Attachment #352412 - Flags: review?(VYV03354)
Comment on attachment 352412 [details] [diff] [review]
adds SEE_MASK_INVOKEIDLIST to fMask

> Masatoshi, do these flags look correct to you?
No, it will make things worse. pidl will have a bogus value unless SHParseDisplayName succeeds.

1. If SHParseDisplayName doesn't exist, (i.e. if you are running on Win2k or WinCE) just call ShellExecuteExW without SEE_MASK_INVOKEIDLIST.
2. If SHParseDisplayName exists but failed, bail out immediately. Do never try to call ShellExecuteExW. This is crucial to workaround the MS07-061 issue. Probably it's better to add a comment about the reason why we have to bail out.
3. If SHParseDisplayName exists and succeeded, call ShellExecuteExW with SEE_MASK_INVOKEIDLIST.

You can #ifdef-out the case 2. and 3. for Windows Mobile.
Attachment #352412 - Flags: review?(VYV03354) → review-
Attached patch bails out on parse failure (obsolete) — Splinter Review
With this patch, if WINCE is defined we short cut to running ShellExecuteEx with lpFile set to the uri spec.  For desktop, if SHParseDisplayName is found and it returns successfully, we add the SEE_MASK_INVOKEIDLIST to the flags and set lpIDList to pidl.  Otherwise, we set the whole SHELLEXECUTEINFO structure to 0.  If cbSize is non-zero, we call ShellExecuteEx, but since we set the structure to 0, that fails, we clean up and bail out with NS_ERROR_FAILURE;.
Attachment #352412 - Attachment is obsolete: true
Attachment #354511 - Flags: review?(VYV03354)
Comment on attachment 354511 [details] [diff] [review]
bails out on parse failure

This patch does not even compile. Did you really test the patch?
>+        memset(&sinfo, 0, sizeof(sinfo));
>+  if (sinfo.cbsize) {
This is overkill. You can use NS_SUCCEEDED(rv) to check whether SHParseDisplayName have failed.
>+  if (pidl)
>+    CoTaskMemFree(pidl);
pidl may not be initialized. Initialize it with NULL.
Attachment #354511 - Flags: review?(VYV03354) → review-
Attached patch updated based on comments (obsolete) — Splinter Review
Attachment #354511 - Attachment is obsolete: true
Attachment #354529 - Flags: review?(VYV03354)
Comment on attachment 354529 [details] [diff] [review]
updated based on comments

It doesn't still compile on desktop Windows.
>+        (MySHParseDisplayName)::GetProcAddress(hDll, "SHParseDisplayName"))
Open curly bracket ("{") is missing.
Please also fix the indent of the following code.
Attachment #354529 - Flags: review?(VYV03354) → review-
>>         if (result > 32)
>>           return NS_OK;

> ShellExecuteEx, unlike ShellExecute, returns a BOOL.

given the distinctly different return paths, I think you should use inline functions for the different platforms to wrap this difference.

the patch i looked at shows you just checking for truth which doesn't seem compatible w/ 32.
Attachment #349364 - Attachment is obsolete: true
Attachment #354529 - Attachment is obsolete: true
Attachment #354673 - Flags: review?(VYV03354)
Comment on attachment 354673 [details] [diff] [review]
adds missing curly brace

>+      if (!result || ((int)sinfo.hInstApp) < 32)
>+      rv = NS_ERROR_FAILURE;
Fix indent.
Otherwise looks good.
Attachment #354673 - Flags: review?(VYV03354) → review+
changeset:   23177:04f503f9694d
user:        Brad Lassey <blassey@mozilla.com>
date:        Mon Dec 29 11:56:21 2008 -0500
summary:     bug 464194 -  ShellExecuteW and SHParseDisplayName do not exist on wince r=emk, bsmedberg
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #354673 - Flags: approval1.9.1?
Attachment #354673 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 354673 [details] [diff] [review]
adds missing curly brace

a191=beltzner
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.