Closed Bug 1118150 Opened 9 years ago Closed 4 years ago

nsIShellService.isDefaultBrowser may throw an exception on Windows

Categories

(Firefox :: Shell Integration, defect, P5)

Product:
Firefox
Component:
Shell Integration
Platform:
x86
Windows 7
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: hiro, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

isDefaultBrowser_may_fail.patch
6.70 KB, patch
Details | Diff | Splinter Review
A workaround
1.94 KB, patch
Details | Diff | Splinter Review
GetLongPathName_fix_in_nsis.diff
1.25 KB, patch
Details | Diff | Splinter Review 
A firefox user in a company reported me the following error occurred on startup.

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIShellService.isDefaultBrowser]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/preferences/advanced.js :: gAdvancedPane.updateSetDefaultBrowser :: line 810"  data: no] preferences.xml:737


From MSDN Document of GetLongPathName <http://msdn.microsoft.com/ja-jp/library/windows/desktop/aa364980%28v=vs.85%29.aspx>:

It is possible to have access to a file or directory but not have access to some of the parent directories of that file or directory. As a result, GetLongPathName may fail when it is unable to query the parent directory of a path component to determine the long name for that component.

So nsIShellService.isDefaultBrowser should be inside try-catch block. 

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7cbbf92a3e9
Attachment #8544419 - Flags: review?(gavin.sharp)
I'm tempted to suggest that we should instead fix nsWindowsShellService::IsDefaultBrowser to never throw an exception (returning false on failure). It's not clear to me that it's ever useful to be able to detect failure to retrieve this information.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #1)
> I'm tempted to suggest that we should instead fix
> nsWindowsShellService::IsDefaultBrowser to never throw an exception
> (returning false on failure). 

Agreed. I was thinking to use another function instead of GetLongPathName but I can not find any other method to obtain long file name on Windows.

I was also thinking about not throwing the exception even if GetLongPathName fails. But it doesn't seem good idea because once GetLongPathName fails we don't have any clue to judge the default browser. So the caller of isDefaultBrowser should know the failure.
See Also: → 456895
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #1)
> > I'm tempted to suggest that we should instead fix
> > nsWindowsShellService::IsDefaultBrowser to never throw an exception
> > (returning false on failure). 
> 
> Agreed. I was thinking to use another function instead of GetLongPathName
> but I can not find any other method to obtain long file name on Windows.

"long file name" was inaccurate. It's "long path name" exactly.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> I was also thinking about not throwing the exception even if GetLongPathName
> fails. But it doesn't seem good idea because once GetLongPathName fails we
> don't have any clue to judge the default browser. So the caller of
> isDefaultBrowser should know the failure.

That is reasonable in theory, but not useful in practice if the caller doesn't do anything special with that knowledge. In your patch it just assumes false and behaves the same way, right?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #4)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > I was also thinking about not throwing the exception even if GetLongPathName
> > fails. But it doesn't seem good idea because once GetLongPathName fails we
> > don't have any clue to judge the default browser. So the caller of
> > isDefaultBrowser should know the failure.
> 
> That is reasonable in theory, but not useful in practice if the caller
> doesn't do anything special with that knowledge. In your patch it just
> assumes false and behaves the same way, right?

Yes, you are right. Do you have a good idea to implement IsDefaultBrowser without GetLongPathName?
You'd have to ask a Windows expert for an alternative to GetLongPathName, but I'm not sure why that's necessary. Why not just change the "return NS_ERROR_FAILURE" to be "return NS_OK"?
Comment on attachment 8544419 [details] [diff] [review]
isDefaultBrowser_may_fail.patch

clearing review flag.

Thanks Gavin for the suggestion.

rstrong, do you have any idea to check the default browser without GetLongPathName and GetShortPathName?

Or should just return NS_OK in failure case of GetLongPathName?
Flags: needinfo?(robert.strong.bugs)
Attachment #8544419 - Flags: review?(gavin.sharp)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Comment on attachment 8544419 [details] [diff] [review]
> isDefaultBrowser_may_fail.patch
> 
> clearing review flag.
> 
> Thanks Gavin for the suggestion.
> 
> rstrong, do you have any idea to check the default browser without
> GetLongPathName and GetShortPathName?
I don't. I am also quite certain that GetLongPathName is used extensively by Windows apps and failures should be extremely uncommon to say the least so best effort would be called for to fix this bug. It would be good to know if any of the other cases where GetLongPathName is used also fail.

You might be able to use "XREExeF" directory provider key for the failure case. At the very least it would be good to know what that returns for the GetLongPathName failure case.

> Or should just return NS_OK in failure case of GetLongPathName?
Depends on the answers to the above.
Flags: needinfo?(robert.strong.bugs)
I think using "XREExeF" as a fallback but what it returns for the GetLongPathName failure case should be checked first.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> I think using "XREExeF" as a fallback but what it returns for the
> GetLongPathName failure case should be checked first.

Thanks, I will try it.
Unfortunately XRE_EXECUTABLE_FILE can not be used there because we have to get the long path name of the XRE_EXECUTABLE_FILE path if run with the short path name. 

e.g.

if the firefox is installed in

c:\target_folder\firefox

then run c:\target~1\firefox\firefox.exe

we need to convert the path to

c:\target_folder\firefox\firefox.exe
FYI, you can reproduce ERROR_ACCESS_DENIED error the following steps.

1. Create "Target_Folder" directory
2. Create "firefox" directory in the "Target_directory"
3. Remove read access permission from olny "Target_Folder"
4. Make sure "firefox" directory can read
5. Install firefox in "firefox" directory
6. Run Target_Folder\firefox\firefox.exe

The important thing is the error does not happen if the directory name in step2 (i.e. firefox) is longer than 12 characters.

I cloud now understand what the MSDN document says:

This check can be skipped for directory components that have file extensions longer than 3 characters, or total lengths longer than 12 characters.
Try using the value that is passed to GetLongPathName as a fallback. I think this should be good enough for this rare edgecase.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #14)
> Try using the value that is passed to GetLongPathName as a fallback. I think
> this should be good enough for this rare edgecase.

Nevermind, that won't work.

Is the system in question intentionally configured this way? Does getting the short path name fail to?
Also, were they able to install into this location configured in this manner?
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #16)
> Also, were they able to install into this location configured in this manner?

Yes, because installation process is normally done by administrator.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #15)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #14)
> > Try using the value that is passed to GetLongPathName as a fallback. I think
> > this should be good enough for this rare edgecase.
> 
> Nevermind, that won't work.
> 
> Is the system in question intentionally configured this way?

Yes, it's a common use case in Japanese company.

> Does getting the short path name fail to?

On the below conditions:
 C:\target_folder (no read access)
 C:\target_folder\firefox (read access)

 GetShortNameW SUCCESS
 GetLongNameW FAIL

On the below conditions:
 C:\target_folder (no read access)
 C:\target_folder\firefox1234567890 (read access)

 GetShortNameW FAIL
 GetLongNameW SUCCESS

Now I am thinking it's a some sort of fault of file system/API design by Microsoft.
Attached patch A workaround — — Splinter Review 
This patch does not fix failures of IsDefaultBrowser completely. But this patch will work fine in most cases.

GetLongPathNameW fails if some parent directory names are not longer than 12 characters. But even in that cases if firefox was launched by long path name, we can use the name returned by GetModuleFileNameW itself.

With this patch, if firefox is launched by short name, IsDefaultBrowser never throws the exception but the result of IsDefaultBrowser will not be correct. But on the current implementation, in the case of launching by short name IsDefaultBrowser is not reliable (may be incorrect) and throws the exception. So I think this patch will make its behaviour a bit better.
Attachment #8545545 - Attachment is obsolete: true
Attachment #8545852 - Flags: feedback?(robert.strong.bugs)
Could you check what value is set in the registry after running the following for an affected installation as an affected user?

"<path to install>\uninstall\helper.exe" /SetAsDefaultAppUser

The value to check is the (Default) value under
HKEY_CURRENT_USER\Software\Classes\FirefoxHTML\shell\open\command\

Actually, check if this value is present and if it is delete it before running the command above. Thanks!
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #20)
> Could you check what value is set in the registry after running the
> following for an affected installation as an affected user?
> 
> "<path to install>\uninstall\helper.exe" /SetAsDefaultAppUser
> 
> The value to check is the (Default) value under
> HKEY_CURRENT_USER\Software\Classes\FirefoxHTML\shell\open\command\
> 
> Actually, check if this value is present and if it is delete it before
> running the command above. Thanks!

A privilege requirement dialog (I do not know what is the name exactly) popped up when running the helper command, and applied the privilege, but the registry key did not appeared.
Try setting it as default for the system first by running
"<path to install>\uninstall\helper.exe" /SetAsDefaultAppGlobal

and then trying the steps in comment #20
Also, you'll need to run "<path to install>\uninstall\helper.exe" /SetAsDefaultAppGlobal as an admin whereas "<path to install>\uninstall\helper.exe" /SetAsDefaultAppUser will need to be run as a user that is affected by this bug.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #23)
> Also, you'll need to run "<path to install>\uninstall\helper.exe"
> /SetAsDefaultAppGlobal as an admin whereas "<path to
> install>\uninstall\helper.exe" /SetAsDefaultAppUser will need to be run as a
> user that is affected by this bug.

Did not appear the registry key.
A short cut on desktop was just created by running the helper with SetAsDefaultAppGlobal as the admin.
There is a fallback in the NSIS code which appears to be failing as well. I don't think there is much that can be done about this configuration and it definitely is a rare edgecase. I also suspect that there are other parts of the code that are also broken. Also, masking the failure doesn't seem that valuable especially for a company that can configure the install to not check if it is default. It would be good to know if chrome also suffers from this and if they don't how chrome handles it when checking and setting as default could be looked into for a possible fix.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #25)
> Also, masking the failure doesn't seem that
> valuable especially for a company that can configure the install to not
> check if it is default. 

But on the current trunk, the in-content preference window is totally broken because the exception by isDefaultBrowser is thrown in initialization of the in-content preference window. So I think  we should care about the case at least.
Agreed wholeheartedly though I do think it should at the very least log the error whether that is from the shell service or the callers.
Brian, do you have an opinion on how to best deal with this?
Flags: needinfo?(netzen)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #25)
> There is a fallback in the NSIS code which appears to be failing as well. 

Your suspicion seems right. There muse be another issue in NSIS code.

HKEY_CURRENT_USER\Software\Classes\FirefoxHTML\shell\open\command\ key did not appear after running the helper with /SetAsDefaultAppGlobal as I mentioned in comment #24.

*BUT* once after running another helper.exe, which is installed in normal place (i.e. c:\program files(x86)\Mozilla Firefox\...), with /SetAsDefaultAppGlobal option, then run the helper, which is installed in problematic place, with /SetAsDefaultAppUser, the registry key was set to:

"" -osinst -url "%1"

So in the helper GetLongPathNameW also failed there. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#2374

I am trying to get error code there but I can not yet. According to document[1], adding ?e option can be used to call GetLastError() but adding ?e makes setup.exe crash...

[1] http://nsis.sourceforge.net/Docs/System/System.html#callfuncs
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #25)
> > There is a fallback in the NSIS code which appears to be failing as well. 
> 
> Your suspicion seems right. There muse be another issue in NSIS code.
> 
> HKEY_CURRENT_USER\Software\Classes\FirefoxHTML\shell\open\command\ key did
> not appear after running the helper with /SetAsDefaultAppGlobal as I
> mentioned in comment #24.
> 
> *BUT* once after running another helper.exe, which is installed in normal
> place (i.e. c:\program files(x86)\Mozilla Firefox\...), with
> /SetAsDefaultAppGlobal option, then run the helper, which is installed in
> problematic place, with /SetAsDefaultAppUser, the registry key was set to:
> 
> "" -osinst -url "%1"
> 
> So in the helper GetLongPathNameW also failed there.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> windows/nsis/common.nsh#2374
> 
> I am trying to get error code there but I can not yet. According to
> document[1], adding ?e option can be used to call GetLastError() but adding
> ?e makes setup.exe crash...
> 
> [1] http://nsis.sourceforge.net/Docs/System/System.html#callfuncs

Could you find out if chrome is also affected first? I suspect there isn't much that can be done and just as setting permissions so a user can't launch an app (e.g. removing execute) will break any app setting permissions in this manner will break any app that needs to get long paths, etc.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #30)

> Could you find out if chrome is also affected first? I suspect there isn't
> much that can be done and just as setting permissions so a user can't launch
> an app (e.g. removing execute) will break any app setting permissions in
> this manner will break any app that needs to get long paths, etc.

Google chrome can not be chosen install place, it is just installed in c:\program files(x86)\Google\Chrome. So I changed the permissions:

 c:\program files(x86)\Google (no read access)
 c:\program files(x86)\Google\Chrome (read access) *Chrome is just 6 characters!*

Then chrome works fine. And able to set default browser. I actually don't know where GetLongPathName is used in chrome codes though.
For the record, this patch works fine on setting default browser except SetAsDefaultAppGlobal case in comment #29.
Comment on attachment 8545852 [details] [diff] [review]
A workaround

Fugue out a solution that handles the NSIS code as well and it will likely be accepted. I'd be interested in knowing if chrome fixes up the path when launched with a short path as well. Might point to what needs to be done. It may be that it is just using the AppAssoc api's
Attachment #8545852 - Flags: feedback?(robert.strong.bugs)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #33)
> Comment on attachment 8545852 [details] [diff] [review]
> A workaround
> 
> Fugue out a solution that handles the NSIS code as well and it will likely
> be accepted. I'd be interested in knowing if chrome fixes up the path when
> launched with a short path as well. 

They don't convert the path to long one. They stores C:\Progra~2\Google\... in registry key (ChromeHTML\shell\open\command) even if running chrome with the long path.
I'm ok with falling back to using the short path. I also suspect there are other parts of Firefox and gecko code that are affected which will require finding other cases and checking them for failure. BTW: thank you very much for your work on this bug.
I agree that falling back tot he short path sounds good, we'd want to test to make sure it works properly though. Would you like me to pick this bug up? It would probably be best to post a separate bug to investigate the other potential cases after a quick search in this bug to ensure there are other cases like that.
Flags: needinfo?(netzen)
Not at the moment. The multiplatform Mar signing is a higher priority and after that's done I'll go over other bug priorities to see where this bug ranks.
I am going to post a revised patch, but before that I want to know what falling back to short path means exactly. That means doing falling back only if GetLongPathName fails with access denied error or whenever GetLongPathName fails?
Without investigating / testing it while writing the patch it is hard to know but both the access denied or all errors will require an if block and it should be simple enough to switch it from one to the other without a lot of effort.
Priority: -- → P5

I can't tell for sure when this changed, but at some point the GetLongPathName call in nsWindowsShellService::IsDefaultBrowser (which is now inside this BinaryPath::GetLong call) starting getting its return value replaced by NS_OK when it fails, which I agree isn't an ideal fix, but at least it means this exception shouldn't be able to happen.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: