493060 - Installer default mail client option doesn't set it correctly on Windows XP

Status: RESOLVED FIXED

(Thunderbird :: Installer, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: patch to move writing the Clients\Mail string to a better location

Specifically
- the start menu still shows OE
- MAPI goes to OE

What seems to be happening is that SetAsDefaultAppUser is what sets the mail/news client values, and we don't call it during install. We call SetAsDefaultMailAppUser instead.

What's even more annoying is that Thunderbird doesn't even check for this value as part of its default app check, so the only way to fix this is to set another app as the default and then run the Tb default app check again. Argh.

I'll ask for review once the try server build is complete.

Comment 1

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

Just so you know... the reason that value isn't checked, etc. is that for the most part OS integration works for the unprivileged user install case which is something that Firefox wants. Mail clients have an additional dependency on being able to write to HKLM for MAPI integration so it may make sense to require that the user has the ability to write to HKLM. It is really a PITA to try to support both unprivileged user installs side by side with privileged user installs.

Comment 2

[Rain [:rain1] (inactive)]

(In reply to comment #1)
> Just so you know... the reason that value isn't checked, etc. is that for the
> most part OS integration works for the unprivileged user install case which is
> something that Firefox wants. Mail clients have an additional dependency on
> being able to write to HKLM for MAPI integration so it may make sense to
> require that the user has the ability to write to HKLM. It is really a PITA to
> try to support both unprivileged user installs side by side with privileged
> user installs.

Hm, I see. In that case, should we not set HKCU\Software\Clients\Mail to Mozilla Thunderbird if we don't have the corresponding key in HKLM?

Comment 3

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

I don't think it should when the keys aren't for the current installation. I'm working on adding this for Firefox over in bug 404541 and will likely add it for other apps as well in that bug.

Comment 4

[David Ascher (:davida)]

Sid, Robert, what's the status of this bug?  As it stands, it sounds like getting some resolution blocks TB3

Comment 5

[Rain [:rain1] (inactive)]

(In reply to comment #4)
> Sid, Robert, what's the status of this bug?  As it stands, it sounds like
> getting some resolution blocks TB3

We basically need to port bug 404541 to Thunderbird. IIRC rs said that his hands were full with Fennec right now, so he might not be able to get to it.

Does this block b4?

Comment 6

[Rain [:rain1] (inactive)]

Comment on attachment 377502 [details] [diff] [review]
patch to move writing the Clients\Mail string to a better location

this patch is wrong

Comment 7

[David :Bienvenu]

mcsmurf, would you be interested in taking a stab at this?

Comment 8

[Ludovic Hirlimann [:Usul]]

Wolfgang would you be interested in fixing this one ?

Comment 9

[Frank Wein [:mcsmurf]]

Attachment: Patch

This patch has not been tested yet, but it should work that way...

Comment 10

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

note: Thunderbird uses Software\clients\mail and Software\clients\News which don't use the FileMainEXE in the name unlike StartMenuInternet. It uses "Software\Clients\Mail\${ClientsRegName}" etc.

Comment 11

[Frank Wein [:mcsmurf]]

Attachment: Patch 2

Fixed the ClientsRegName issue, fixed on bogus return statement.

Comment 12

[Simon Paquet [:sipaq]]

Who can drive this in today?

Comment 13

[Rain [:rain1] (inactive)]

I ran the patch through the try server earlier today, but packaging failed.

https://build.mozillamessaging.com/buildbot/try/builders/Try%20server%20comm-central%20win32%20hg%20builder/builds/158/steps/packaging%20%28exe%29/logs/stdio

Comment 14

[David Ascher (:davida)]

The section:

  ; Cleanup operations to perform at the end of the installation.
 @@ -427,7 +456,16 @@
      ${MUI_INSTALLOPTIONS_READ} $0 "options.ini" "Field 6" "State"
      ${If} "$0" == "1"
        ${LogHeader} "Setting as the default mail application"
 -      Call SetAsDefaultMailAppUser
 +      ClearErrors
 +      ${GetParameters} $0
 +      ${GetOptions} "$0" "/UAC:" $0
 +      ${If} ${Errors}
 +        Call SetAsDefaultAppUserHKCU
 +      ${Else}
 +        GetFunctionAddress $0 SetAsDefaultAppUserHKCU
 +        UAC::ExecCodeSegment $0
 +      ${EndIf}
 +     ${EndIf}
      ${EndIf}
    ${EndUnless}

looks like it adds an ${EndIf} too many, which would match the error log.

Comment 15

[Frank Wein [:mcsmurf]]

Attachment: Patch 3

Robert: Do you still review TB installer patches?

Fixed the installer.nsi error

Comment 16

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

I'll review the patch but I've never been a Thunderbird peer so you'll need to get a peer to also r+ it.

Comment 17

[Rain [:rain1] (inactive)]

OK, this try server build worked.

http://s3.mozillamessaging.com/build/try-server/2009-09-11_10:[e-mail]-493060-xp-installer/[e-mail]

Comment 18

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

Comment on attachment 400068 [details] [diff] [review]
Patch 3

No need to resubmit yet since I'll shortly review the other two files
>diff --git a/mail/installer/windows/nsis/installer.nsi b/mail/installer/windows/nsis/installer.nsi
>--- a/mail/installer/windows/nsis/installer.nsi
>+++ b/mail/installer/windows/nsis/installer.nsi
>...
>@@ -404,17 +418,31 @@
>     ${LogMsg} "Added Shortcut: $SMPROGRAMS\$StartMenuDir\${BrandFullName} ($(SAFE_MODE)).lnk"
>   ${EndIf}
> 
>-  ${If} $AddQuickLaunchSC == 1
>-    CreateShortCut "$QUICKLAUNCH\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}" "" "$INSTDIR\${FileMainEXE}" 0
>-    ${LogMsg} "Added Shortcut: $QUICKLAUNCH\${BrandFullName}.lnk"
>-  ${EndIf}
>+  !insertmacro MUI_STARTMENU_WRITE_END
> 
>   ${If} $AddDesktopSC == 1
>     CreateShortCut "$DESKTOP\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}" "" "$INSTDIR\${FileMainEXE}" 0
>     ${LogMsg} "Added Shortcut: $DESKTOP\${BrandFullName}.lnk"
>   ${EndIf}
>-
>-  !insertmacro MUI_STARTMENU_WRITE_END
>+  
nit: remove spaces on the empty line

>@@ -427,8 +455,16 @@
>     ${MUI_INSTALLOPTIONS_READ} $0 "options.ini" "Field 6" "State"
>     ${If} "$0" == "1"
>       ${LogHeader} "Setting as the default mail application"
>-      Call SetAsDefaultMailAppUser
>-    ${EndIf}
>+      ClearErrors
>+      ${GetParameters} $0
>+      ${GetOptions} "$0" "/UAC:" $0
>+      ${If} ${Errors}
>+        Call SetAsDefaultMailAppUserHKCU
>+      ${Else}
>+        GetFunctionAddress $0 SetAsDefaultMailAppUserHKCU
>+        UAC::ExecCodeSegment $0
>+      ${EndIf}
>+     ${EndIf}
nit: fix indenting

>   ${EndUnless}
> 
>   ${LogHeader} "Updating Uninstall Log With Previous Uninstall Log"
>@@ -438,9 +474,9 @@
> 
>   ${InstallEndCleanupCommon}
> 
>-  ; If we have to reboot give SHChangeNotify time to finish the refreshing
>-  ; the icons so the OS doesn't display the icons from helper.exe
>   ${If} ${RebootFlag}
>+    ; If we have to reboot give SHChangeNotify time to finish the refreshing
>+    ; the icons so the OS doesn't display the icons from helper.exe
nit: please fix the comment s/the refreshing/refreshing/

Also, below this line is
     ${If} ${FileExists} "$INSTDIR\${FileMainEXE}"
       ClearErrors
       Rename "$INSTDIR\${FileMainEXE}" "$INSTDIR\${FileMainEXE}.moz-delete"
       ${Unless} ${Errors}
         Delete /REBOOTOK "$INSTDIR\${FileMainEXE}.moz-delete"
       ${EndUnless}
     ${EndUnless}
Please change the ${EndUnless} with ${EndIf} 

>@@ -584,22 +624,38 @@
>...
>+  ; If the user doesn't have write access to the installation directory set
>+  ; the installation directory to a subdirectory of the All Users application
>+  ; directory and if the user can't write to that location set the installation
>+  ; directory to a subdirectory of the users local application directory
>+  ; (e.g. non-roaming).
>+  ${CanWriteToInstallDir} $R8
>+  ${If} "$R8" == "false"
>+    SetShellVarContext all      ; Set SHCTX to All Users
>+    StrCpy $INSTDIR "$APPDATA\${BrandFullName}\"
>+    ${If} ${FileExists} "$INSTDIR"
>+      ; Always display the long path if the path already exists.
>+      ${GetLongPath} "$INSTDIR" $INSTDIR
>+    ${EndIf}
>+    ${CanWriteToInstallDir} $R8
>+    ${If} "$R8" == "false"
>+      StrCpy $INSTDIR "$LOCALAPPDATA\${BrandFullName}\"
>+    ${EndIf}
>+
nit: please remove the empty line

Comment 19

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

Comment on attachment 400068 [details] [diff] [review]
Patch 3

>diff --git a/mail/installer/windows/nsis/shared.nsh b/mail/installer/windows/nsis/shared.nsh
>--- a/mail/installer/windows/nsis/shared.nsh
>+++ b/mail/installer/windows/nsis/shared.nsh
>...
>@@ -554,6 +484,7 @@
> !macroend
> !define UpdateProtocolHandlers "!insertmacro UpdateProtocolHandlers"
> 
>+; Removes various registry entries for reasons noted below (does not use SHCTX).
> !macro RemoveDeprecatedKeys
>   StrCpy $0 "SOFTWARE\Classes"

I suggest adding the following to RemoveDeprecatedKeys
http://mxr.mozilla.org/mozilla1.9.1/source/browser/installer/windows/nsis/shared.nsh#475
  ; Remove the app compatibility registry key
  StrCpy $0 "Software\Microsoft\Windows NT\CurrentVersion\AppCompatFlags\Layers"
  DeleteRegValue HKLM "$0" "$INSTDIR\${FileMainEXE}"
  DeleteRegValue HKCU "$0" "$INSTDIR\${FileMainEXE}"

r=me - note that I usually compile / test when I review but I don't have time to do so today so I've only done a code review.

Comment 20

[Phil Ringnalda (:philor)]

Attachment: Patch 4

Comments (thanks rs!) addressed (and a few more bits of trailing whitespace).

Comment 21

[Phil Ringnalda (:philor)]

Comment on attachment 400226 [details] [diff] [review]
Patch 4

I'm not entirely convinced that I know how to thoroughly test, but it certainly seems to work.

Comment 22

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/04542a716ea1

Comment 23

[Frank Wein [:mcsmurf]]

After setting up a limited user on Windows 7, the installer did correctly ask to elevate. After having denied this request, it correctly chose C:\ProgramData\Shredder as default install path. When I acknowledged the elevation request, it chose C:\Program Files\Shredder as default install path.
After having installed TB without elevation, the helper app requests elevation when you want to set TB as default mail client. I found a (non-critical) bug while testing this, will attach patch for this.

Comment 24

[Frank Wein [:mcsmurf]]

Attachment: Fix bug

This patch fixes one issue in SetClientsMail/SetClientsNews. Those macros use AddHandlerValues, which uses SHCTX. We need to set the context to all so that this macro writes to HKLM. No other variables in SetClientsMail/SetClientsNews should be affected by this change as only $INSTDIR, ${ClientsRegName}, ${FileMainEXE}, ${AppRegNameMail} and ${AppRegNameNews} are used.

Comment 25

[Martin Schröder [:mschroeder]]

Comment on attachment 400258 [details] [diff] [review]
Fix bug

Has this patch already been checked in?

Comment 26

[Phil Ringnalda (:philor)]

Has sticking "just one more patch" on a closed bug *ever* actually worked out?

Comment 27

[Frank Wein [:mcsmurf]]

Normally this works when the patch author does not forget about his patch :)

http://hg.mozilla.org/comm-central/rev/b1d0616ac1e4