640824 - Stub installer fails with false no access rights error if install path is quoted

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[ZAngusSF]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 [RC 1]

Downloaded and installed FF4 RC1 over FF4b12 in "C:\Program Files\Mozilla Firefox 4".  During the install I pasted that path, with quotes, in the dialog requesting the folder into which I wanted to install FF.  I got an error dialog saying I didn't have access rights to the folder.  If I remove the quotes, or if I use the  [Browse] button to reach the folder, the same string without quotes (i.e. C:\Program Files\Mozilla Firefox 4) works fine.

Reproducible: Always

Steps to Reproduce:
1. Select "Custom" install
2. Paste path with quotes (e.g. "C:\Program Files\Mozilla Firefox 4")
3. Error dialog
Actual Results:  
Error dialog claiming I don't have access rights to the target directory.

Expected Results:  
Installation should proceed.  Installer should either strip the quotes (if it can't handle them) or accept the string as-is (it's normal in Windows to "quote" paths which contain strings).

Comment 1

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

I've also noticed that if you paste a valid quoted path into the Win7 (probably WinVista as well) start menu -> searchbox it will search instead of opening it.

A valid bug but very much so an edgecase

Comment 2

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

Attachment: patch rev1

Comment 3

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

Attachment: patch rev1

forgot to qrefresh

Comment 4

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

Since the stub installer i now our primary distribution method this will be fixed there first and the full installer will automatically get this change when it is rewritten to use the same code and UI as the stub installer.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 8398970 [details] [diff] [review]
patch rev1

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

::: browser/installer/windows/nsis/stub.nsi
@@ +1884,5 @@
>    Pop $0
>    System::Call 'user32::GetWindowTextW(i $DirRequest, w .r0, i ${NSIS_MAX_STRLEN})'
> +  StrCpy $1 "$0" 1 ; the first character
> +  ${If} "$1" == "$\""
> +  ${OrIf} "$1" == "'"

I don't think single quotes should be accepted as quoted paths on Windows

@@ +1905,5 @@
>    ${EndIf}
>  FunctionEnd
>  
>  Function OnClick_ButtonBrowse
> +  StrCpy $0 "$INSTDIR"

I know this code didn't do what it ws supposed to before, since both the If block and the Else block did the same thing, but the comment seems reasonable. Should there be an attempt to append a \ if there is not one there?

Comment 6

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

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Comment on attachment 8398970 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8398970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +1884,5 @@
> >    Pop $0
> >    System::Call 'user32::GetWindowTextW(i $DirRequest, w .r0, i ${NSIS_MAX_STRLEN})'
> > +  StrCpy $1 "$0" 1 ; the first character
> > +  ${If} "$1" == "$\""
> > +  ${OrIf} "$1" == "'"
> 
> I don't think single quotes should be accepted as quoted paths on Windows
Done

> @@ +1905,5 @@
> >    ${EndIf}
> >  FunctionEnd
> >  
> >  Function OnClick_ButtonBrowse
> > +  StrCpy $0 "$INSTDIR"
> 
> I know this code didn't do what it ws supposed to before, since both the If
> block and the Else block did the same thing, but the comment seems
> reasonable. Should there be an attempt to append a \ if there is not one
> there?
The comment has to do with the builtin directory / browse button while the stub installer uses a custom directory / browse button
http://nsis.sourceforge.net/Docs/Chapter4.html#4.8.1.21

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 8398970 [details] [diff] [review]
patch rev1

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

r=me with the single quote change. Thanks.

Comment 8

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

Attachment: patch updated to comment

Comment 9

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/96c761698d25

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96c761698d25