491947 - Disable DDE shell integration

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

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

This can be done (checked on Vista) by setting the default value of the ddeexec key to an empty string.

The reason for doing this is that we have had numerous random bug reports over the years regarding Windows DDE (Dynamic Data Exchange) not working with Firefox and they are typically due to several different random problems that aren't the fault of Firefox and there are no workarounds.

I've done some investigating and have found that Chrome, Opera (Opera only disables DDE for protocols), and Safari have disabled their DDE integration.

This is filed under the installer since that is where the work will need to be done.

Comment 1

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

One example of a MS DDE bug
http://support.microsoft.com/kb/892850

Comment 2

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

note: the main concern I have with disabling dde is responsiveness of opening urls from other apps when there is heavy disk activity.

Comment 3

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

Hey Jim, thanks for taking this... I'm stuck on some updater idiosyncrasies on Windows CE atm but will be more than happy to help out with this.

Comment 4

[Jim Mathies [:jimm]]

(In reply to comment #3)
> Hey Jim, thanks for taking this... I'm stuck on some updater idiosyncrasies on
> Windows CE atm but will be more than happy to help out with this.

Any tricks you can share for debugging nsis install runs? I've actually got the basics in there but managed to mess up parameters somehow in a call. Trying to figure out how to debug this since the install completes, but doesn't do what I expect it to.

Comment 5

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

I've used NSIS debug builds in the past but it is usually overkill for this type of work. I usually use the log file or messageboxes for this type of change. I finished up the majority of the updater WinCE work and can likely put together a patch for this tomorrow unless you would prefer to keep this bug for yourself.

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> I've used NSIS debug builds in the past but it is usually overkill for this
> type of work. I usually use the log file or messageboxes for this type of
> change. I finished up the majority of the updater WinCE work and can likely put
> together a patch for this tomorrow unless you would prefer to keep this bug for
> yourself.

Let me work on it a bit, I didn't have the log going which I'm sure would help. I'm also interested in trying to address bug 505925 as it pertains to app registration and that ties in with this so this is good starting point for me.

Comment 7

[Jim Mathies [:jimm]]

Attachment: remove dde support v.1

Since we disable DDE registration, I went ahead and cleaned out all the dde related code. Rob, welcome your comments on this, if we are disabling it, is there any need for the dde related code?

I've been testing this on vista and win7 and everything seems to be working fine.

Comment 8

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

I'd prefer it if the removal of the work in nsNativeAppSupportWin.cpp was split off into a separate bug so the changes made in this bug have a smaller scope. I'm also not sure if it should be removed at this time.

Also, the default value under <handler>\shell\open\ddeexec should be an empty string for pre Win Vista.

To make this simpler for other apps that use the same macros in common.nsh a new macro should be added instead of removing AddDDEHandler and modifying AddHandler.

Comment 9

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

Also, the requestPending flag should be removed (don't forget to remove the usage in browser/components/nsBrowserContentHandler.js as well)

Comment 10

[Jim Mathies [:jimm]]

Attachment: remove dde support v.2

Comment 11

[Jim Mathies [:jimm]]

I did a little bit a reorg on the depreciatedkeys function, creating pre and post functions called before and after registration occurs. Also left the other common routines alone and created a new addhandler2 method for non-dde handler registration.

Comment 12

[Jim Mathies [:jimm]]

Rob, also note a came across this odd orphaned pref - advanced.system.supportDDEExec that I removed. Not sure what this was used for but AFAICT it isn't in use. Any idea what that was used for?

Comment 13

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

(In reply to comment #12)
> Rob, also note a came across this odd orphaned pref -
> advanced.system.supportDDEExec that I removed. Not sure what this was used for
> but AFAICT it isn't in use. Any idea what that was used for?
I believe that was for when we used to try to remove / add the ddeexec keys during app shutdown / startup. It was a horrible hack that broke under several circumstances.

Comment 14

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

Comment on attachment 397928 [details] [diff] [review]
remove dde support v.2

First part

>diff --git a/browser/components/shell/src/nsWindowsShellService.cpp b/browser/components/shell/src/nsWindowsShellService.cpp
>--- a/browser/components/shell/src/nsWindowsShellService.cpp
>+++ b/browser/components/shell/src/nsWindowsShellService.cpp
>@@ -151,46 +151,34 @@ OpenKeyForWriting(HKEY aStartKey, const 
> //
> //   HKCU\SOFTWARE\Classes\.<ext>\      (default)         REG_SZ     FirefoxHTML
> //
> //   as aliases to the class:
> //
> //   HKCU\SOFTWARE\Classes\FirefoxHTML\
> //     DefaultIcon                      (default)         REG_SZ     <apppath>,1
> //     shell\open\command               (default)         REG_SZ     <apppath> -requestPending -osint -url "%1"
nit: remove -requestPending

>-//     shell\open\ddeexec               (default)         REG_SZ     "%1",,0,0,,,,
>-//     shell\open\ddeexec               NoActivateHandler REG_SZ
>-//                       \Application   (default)         REG_SZ     Firefox
>-//                       \Topic         (default)         REG_SZ     WWW_OpenURL
> //
> // - Windows Vista Protocol Handler
> //
> //   HKCU\SOFTWARE\Classes\FirefoxURL\  (default)         REG_SZ     <appname> URL
> //                                      EditFlags         REG_DWORD  2
> //                                      FriendlyTypeName  REG_SZ     <appname> URL
> //     DefaultIcon                      (default)         REG_SZ     <apppath>,1
nit: same here

> //     shell\open\command               (default)         REG_SZ     <apppath> -requestPending -osint -url "%1"
>-//     shell\open\ddeexec               (default)         REG_SZ     "%1",,0,0,,,,
>-//     shell\open\ddeexec               NoActivateHandler REG_SZ
>-//                       \Application   (default)         REG_SZ     Firefox
>-//                       \Topic         (default)         REG_SZ     WWW_OpenURL
> //
> // - Protocol Mappings
> //   -----------------
> //   The following protocols:
> //    HTTP, HTTPS, FTP
> //   are mapped like so:
> //
> //   HKCU\SOFTWARE\Classes\<protocol>\
> //     DefaultIcon                      (default)         REG_SZ     <apppath>,1
> //     shell\open\command               (default)         REG_SZ     <apppath> -requestPending -osint -url "%1"
nit: same here

>-//     shell\open\ddeexec               (default)         REG_SZ     "%1",,0,0,,,,
>-//     shell\open\ddeexec               NoActivateHandler REG_SZ
>-//                       \Application   (default)         REG_SZ     Firefox
>-//                       \Topic         (default)         REG_SZ     WWW_OpenURL
> //
> // - Windows Start Menu (Win2K SP2, XP SP1, and newer)
> //   -------------------------------------------------
> //   The following keys are set to make Firefox appear in the Start Menu as the
> //   browser:
> //   
> //   HKCU\SOFTWARE\Clients\StartMenuInternet\FIREFOX.EXE\
> //                                      (default)         REG_SZ     <appname>

You also need to fix the following

#ifndef WINCE
#define APP_REG_NAME L"Firefox"
#define CLS_HTML "FirefoxHTML"
#define CLS_URL "FirefoxURL"
#define CPL_DESKTOP L"Control Panel\\Desktop"
#define VAL_OPEN "\"%APPPATH%\" -requestPending -osint -url \"%1\""
#define VAL_FILE_ICON "%APPPATH%,1"
#else
#define CPL_DESKTOP L"ControlPanel\\Desktop"
#define VAL_OPEN "\"%APPPATH%\" -osint -url \"%1\""
#define VAL_FILE_ICON "%APPPATH%,-2"
#endif

Remove both #define VAL_OPEN and define it once above #define DI as follows since they are the same now
#define VAL_OPEN "\"%APPPATH%\" -osint -url \"%1\""

Comment 15

[Jim Mathies [:jimm]]

Attachment: remove dde support v.3

updated!

Comment 16

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

Comment on attachment 397948 [details] [diff] [review]
remove dde support v.3

Second part... I'll finish the review this afternoon so no need to resubmit yet.

>diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh
>--- a/toolkit/mozapps/installer/windows/nsis/common.nsh
>+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh
>@@ -1178,16 +1178,147 @@
>     !define _MOZFUNC_UN
>     !verbose pop
>   !endif
> !macroend
> 
> 
> ################################################################################
> # Macros for adding file and protocol handlers
>+
>+################################################################################
>+# Macros for adding file and protocol handlers
Please remove the duplicate

>+/**
>+ * Writes common registry values for a handler that DOES NOT use DDE using SHCTX.
>+ *
>+ * @param   _KEY
>+ *          The key name in relation to the HKCR root. SOFTWARE\Classes is
>+ *          prefixed to this value when using SHCTX.
>+ * @param   _VALOPEN
>+ *          The path and args to launch the application.
>+ * @param   _VALICON
>+ *          The path to an exe that contains an icon and the icon resource id.
I just realized that this comment is bogus. It is the base 0 icon position within the icon list and not the resouce id. Could you update this comment and other comments regarding this?

>+ * @param   _DISPNAME
>+ *          The display name for the handler. If emtpy no value will be set.
>+ * @param   _ISPROTOCOL
>+ *          Sets protocol handler specific registry values when "true".
>+ *
>+ * $R0 = storage for SOFTWARE\Classes
>+ * $R1 = string value of the current registry key path.
>+ * $R2 = _KEY
>+ * $R3 = _VALOPEN
>+ * $R4 = _VALICON
>+ * $R5 = _DISPNAME
>+ * $R6 = _ISPROTOCOL
>+ */
>+!macro AddHandlerValues2
Let's go with a descriptive name along the lines of AddDisableDDEHandlers or AddDDEDisabledHandlers

>+
>+  !ifndef ${_MOZFUNC_UN}AddHandlerValues2
>+    !verbose push
>+    !verbose ${_MOZFUNC_VERBOSE}
>+    !define ${_MOZFUNC_UN}AddHandlerValues2 "!insertmacro ${_MOZFUNC_UN}AddHandlerValues2Call"
>+
>+    Function ${_MOZFUNC_UN}AddHandlerValues2
>+      Exch $R6 ; true/false+empty - protocol handler?
>+      Exch 1
>+      Exch $R5 ; FriendlyTypeName
>+      Exch 2
>+      Exch $R4 ; icon
>+      Exch 3
>+      Exch $R3 ; shell\open\command
>+      Exch 4
>+      Exch $R2 ; reg key
>+      Push $R1 ;
>+      Push $R0 ; base reg class
For consistency I've tried to make it so all macros start with R9 and work their way down for the registers they need

>+      StrCpy $R0 "SOFTWARE\Classes"
>+      StrCmp "$R5" "" +6 +1
>+      ReadRegStr $R1 SHCTX "$R2" "FriendlyTypeName"
>+
>+      StrCmp "$R1" "" +1 +3
>+      WriteRegStr SHCTX "$R0\$R2" "" "$R5"
>+      WriteRegStr SHCTX "$R0\$R2" "FriendlyTypeName" "$R5"
>+
>+      StrCmp "$R6" "true" +1 +8
>+      WriteRegStr SHCTX "$R0\$R2" "URL Protocol" ""
>+      StrCpy $R1 ""
>+      ClearErrors
ClearErrors is unnecessary here and the macro you copied this from. This will require changing the above "StrCmp "$R6" "true" +1 +8" to "StrCmp "$R6" "true" +1 +7"

>+      ReadRegDWord $R1 SHCTX "$R0\$R2" "EditFlags"
>+
Remove this newline.

Comment 17

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

Comment on attachment 397948 [details] [diff] [review]
remove dde support v.3

Still have to finish shared.nsh

>diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>@@ -315,20 +315,26 @@ Section "-Application" APP_IDX
>   ${EndIf}
> 
>   ; Default for creating Desktop shortcut (1 = create, 0 = don't create)
>   ${If} $AddDesktopSC == ""
>     StrCpy $AddDesktopSC "1"
>   ${EndIf}
> 
>   ${LogHeader} "Adding Registry Entries"
>+
>+  ${PreRemoveDeprecatedKeys} ; pre-clean obsolete registry entries
>+
>   SetShellVarContext current  ; Set SHCTX to HKCU
>   ${RegCleanMain} "Software\Mozilla"
>-  ${RegCleanUninstall}
>-  ${UpdateProtocolHandlers}
>+  ${RegCleanUninstall} ; clean uninstall registry entries
>+  ${UpdateProtocolHandlers} ; update handler entries
nit: descriptive comments help but the names of these macros are already as descriptive as the comment.

>+
>+  ; The previous installer adds several regsitry values to both HKLM and HKCU.
>+  ; We now try to add to HKLM and if that fails to HKCU
I added this to make it clear that we no longer write to both HKCU and HKLM as the xpinstall installer did. This has been the case long enough that I think it is ok to remove this comment now.

Comment 18

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

Comment on attachment 397948 [details] [diff] [review]
remove dde support v.3

Finished review.

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -31,16 +31,18 @@
>...
>@@ -201,35 +203,28 @@
>     WriteRegStr SHCTX "$0\.xht"   "" "FirefoxHTML"
>   ${EndIf}
> 
>   ReadRegStr $6 SHCTX "$0\.xhtml" ""
>   ${If} "$6" != "FirefoxHTML"
>     WriteRegStr SHCTX "$0\.xhtml" "" "FirefoxHTML"
>   ${EndIf}
> 
>-  StrCpy $3 "$\"%1$\",,0,0,,,,"
>-
>+  ; Create the root file and protocol handlers:
nit: What is the dfference between file and protocol handlers and "root" file and protocol handlers? I don't think the word root convolutes understanding what handlers are in relation to this code.

>@@ -412,58 +407,74 @@
>... 
>+; Removes various registry entries for reasons noted below prior to installation
>+; or upgrade (does not use SHCTX).
>+!macro PreRemoveDeprecatedKeys
This will break older installs of Firefox that still use DDE since there is no check to see if we are modifying keys that point to this install. Instead, use the new macro in common.nsh to remove any DDE keys / values (btw: Opera just leaves them). Then you don't need to change the name of RemoveDeprecatedKeys or add PreRemoveDeprecatedKeys.

>+  ; Drop support for DDE (bug 491947), remove old entries if they exist.
>+  DeleteRegKey HKCR "FirefoxHTML\shell\open\ddeexec"
>+  DeleteRegKey HKCR "FirefoxURL\shell\open\ddeexec"
>+  DeleteRegKey HKLM "SOFTWARE\Classes\FirefoxHTML\shell\open\ddeexec"
>+  DeleteRegKey HKLM "SOFTWARE\Classes\FirefoxURL\shell\open\ddeexec"
>+  DeleteRegKey HKCU "SOFTWARE\Classes\FirefoxHTML\shell\open\ddeexec"
>+  DeleteRegKey HKCU "SOFTWARE\Classes\FirefoxURL\shell\open\ddeexec"
>+  ClearErrors
>+
>+  ; The ifexec key may have been added by another application so try to
>+  ; delete it to prevent it from breaking this app's shell integration.
>+  ; Also, IE 6 and below doesn't remove this key when it sets itself as the
>+  ; default handler and if this key exists IE's shell integration breaks.
>+  DeleteRegKey HKLM "SOFTWARE\Classes\FirefoxHTML\shell\open\ddeexec\ifexec"
>+  DeleteRegKey HKCU "SOFTWARE\Classes\FirefoxURL\shell\open\ddeexec\ifexec"
>+  ClearErrors
>+!macroend
>+!define PreRemoveDeprecatedKeys "!insertmacro PreRemoveDeprecatedKeys"

I'd like to take another look at the finished patch before approving

Comment 19

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

(In reply to comment #16)
> >+/**
> >+ * Writes common registry values for a handler that DOES NOT use DDE using SHCTX.
> >+ *
> >+ * @param   _KEY
> >+ *          The key name in relation to the HKCR root. SOFTWARE\Classes is
> >+ *          prefixed to this value when using SHCTX.
> >+ * @param   _VALOPEN
> >+ *          The path and args to launch the application.
> >+ * @param   _VALICON
> >+ *          The path to an exe that contains an icon and the icon resource id.
> I just realized that this comment is bogus. It is the base 0 icon position
> within the icon list and not the resouce id. Could you update this comment and
> other comments regarding this?
Actually, this can be the resource id or the position.

Comment 20

[Jim Mathies [:jimm]]

> >@@ -412,58 +407,74 @@
> >... 
> >+; Removes various registry entries for reasons noted below prior to installation
> >+; or upgrade (does not use SHCTX).
> >+!macro PreRemoveDeprecatedKeys
> This will break older installs of Firefox that still use DDE since there is no
> check to see if we are modifying keys that point to this install. Instead, use
> the new macro in common.nsh to remove any DDE keys / values (btw: Opera just
> leaves them). Then you don't need to change the name of RemoveDeprecatedKeys or
> add PreRemoveDeprecatedKeys.

I originally tried to do it this way. The problem is that we want to purge all existing dde keys under FirefoxHTML & FirefoxURL, and then selectively add the empty dde key back in through two calls to the new macro, one for FirefoxHTML and one for FirefoxURL. If I purge everything through the new macro I end up deleting some keys I create. The only way to purge and recreate seemed to be the pre step which also saved us the added step of removing the ifexec keys multiple times. I think the pre step will over time become handy so maybe keep it and just solve the install path issue?

DeleteRegKey HKCR "FirefoxHTML\shell\open\ddeexec"
DeleteRegKey HKCR "FirefoxURL\shell\open\ddeexec"
DeleteRegKey HKLM "SOFTWARE\Classes\FirefoxHTML\shell\open\ddeexec"
DeleteRegKey HKLM "SOFTWARE\Classes\FirefoxURL\shell\open\ddeexec"
DeleteRegKey HKCU "SOFTWARE\Classes\FirefoxHTML\shell\open\ddeexec"
DeleteRegKey HKCU "SOFTWARE\Classes\FirefoxURL\shell\open\ddeexec"

Comment 21

[Jim Mathies [:jimm]]

I guess I could purge just the dde group based on the _KEY (key name) in the new macro assuming we always create / update both of those keys on every install and update.

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #21)
> I guess I could purge just the dde group based on the _KEY (key name) in the
> new macro assuming we always create / update both of those keys on every
> install and update.

If I do this, would it be ok to move the ifexec removal to RemoveDeprecatedKeys? (I think it would but you know this stuff better than I do. :)

Comment 23

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

Can you provide specifics? Unless I'm mistaken the only key value under ddeexec you add / need is the empty string which is added in the new macro. Why can't you first delete then add the empty string? Also, http, https, and ftp will need this done as well for Win XP, Win 2K, and all platforms for apps that read the keys directly. We also need to make sure the ifexec key is removed for all of the handlers during this phase and I don't see any value in not doing it during this time.

Comment 24

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

Actually, nothing should need to be removed with the empty string added but it is a good thing to clean up IMO

Comment 25

[Jim Mathies [:jimm]]

(In reply to comment #23)
> Can you provide specifics? Unless I'm mistaken the only key value under ddeexec
> you add / need is the empty string which is added in the new macro. Why can't
> you first delete then add the empty string? Also, http, https, and ftp will
> need this done as well for Win XP, Win 2K, and all platforms for apps that read
> the keys directly. We also need to make sure the ifexec key is removed for all
> of the handlers during this phase and I don't see any value in not doing it
> during this time.

Actually, making the assumption all handlers will get the update call the ifexec  code can be removed completely -

DeleteRegKey HKCR "$R5\shell\open\ddeexec"
DeleteRegKey HKLM "SOFTWARE\Classes\$R5\shell\open\ddeexec"
DeleteRegKey HKCU "SOFTWARE\Classes\$R5\shell\open\ddeexec"

removes 

HKLM "SOFTWARE\Classes\$R5\shell\open\ddeexec\ifexec"

then we create:

WriteRegStr SHCTX "$R3\$R5\shell\open\ddeexec" "" ""

which propagates back out.

Comment 26

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

and by using the UpdateProtocolHandlers macro (it really should be renamed to UpdateHandlers but not in this bug) you won't change handlers for other installations of Firefox or other apps that handle http, etc.

Comment 27

[Jim Mathies [:jimm]]

This is what I have now for the new AddDDEDisabledHandlers macro:

; Drop support for DDE (bug 491947), remove old entries if they exist.
DeleteRegKey HKCR "$R5\shell\open\ddeexec"
DeleteRegKey HKLM "SOFTWARE\Classes\$R5\shell\open\ddeexec"
DeleteRegKey HKCU "SOFTWARE\Classes\$R5\shell\open\ddeexec"

; The ifexec key may have been added by another application so try to
; delete it to prevent it from breaking this app's shell integration.
; Also, IE 6 and below doesn't remove this key when it sets itself as the
; default handler and if this key exists IE's shell integration breaks.
; DeleteRegKey HKLM "SOFTWARE\Classes\$R5\shell\open\ddeexec\ifexec"
; (Now removed by the above dde cleanup code.)

; Create empty dde key in the appropriate location for pre-vista
; operating systems.
WriteRegStr SHCTX "$R3\$R5\shell\open\ddeexec" "" ""


Keep that ifexec commenting in there or just remove it?

Comment 28

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

(In reply to comment #27)
> This is what I have now for the new AddDDEDisabledHandlers macro:
> 
> ; Drop support for DDE (bug 491947), remove old entries if they exist.
> DeleteRegKey HKCR "$R5\shell\open\ddeexec"
> DeleteRegKey HKLM "SOFTWARE\Classes\$R5\shell\open\ddeexec"
> DeleteRegKey HKCU "SOFTWARE\Classes\$R5\shell\open\ddeexec"
This should respect SHCTX since if we may be default at the user level and not at the system level so only one call should be necessary. If you'd like to try to clean up the leftovers during app update that would need to be done separately which I believe is what you might have been getting at with wanting to split out the remove macro into two macros.

> ; The ifexec key may have been added by another application so try to
> ; delete it to prevent it from breaking this app's shell integration.
> ; Also, IE 6 and below doesn't remove this key when it sets itself as the
> ; default handler and if this key exists IE's shell integration breaks.
> ; DeleteRegKey HKLM "SOFTWARE\Classes\$R5\shell\open\ddeexec\ifexec"
> ; (Now removed by the above dde cleanup code.)
> 
> ; Create empty dde key in the appropriate location for pre-vista
> ; operating systems.
> WriteRegStr SHCTX "$R3\$R5\shell\open\ddeexec" "" ""
> 
> 
> Keep that ifexec commenting in there or just remove it?
Remove it

Comment 29

[Jim Mathies [:jimm]]

Attachment: remove dde support v.4

Comment 30

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

Comment on attachment 398225 [details] [diff] [review]
remove dde support v.4

Almost there...

>+ * Writes common registry values for a handler that DOES NOT use DDE using SHCTX.
>+ *
>+ * @param   _KEY
>+ *          The key name in relation to the HKCR root. SOFTWARE\Classes is
>+ *          prefixed to this value when using SHCTX.
>+ * @param   _VALOPEN
>+ *          The path and args to launch the application.
>+ * @param   _VALICON
>+ *          The application icon resource index or a full file and
>+ *          icon resource index path.
>+ * @param   _DISPNAME
>+ *          The display name for the handler. If emtpy no value will be set.
>+ * @param   _ISPROTOCOL
>+ *          Sets protocol handler specific registry values when "true".
>+ *
>+ * $R3 = storage for SOFTWARE\Classes
>+ * $R4 = string value of the current registry key path.
>+ * $R5 = _KEY
>+ * $R6 = _VALOPEN
>+ * $R7 = _VALICON
>+ * $R8 = _DISPNAME
>+ * $R9 = _ISPROTOCOL
>+ */
>+!macro AddDDEDisabledHandlers
nit: since this only adds one handler it shouldn't be plural so AddDDEDisabledHandler would be better

nit: the comment for the icon resource index / resource id is still a tad off.

I'm going to test this out before r+ing but I suspect the only things left are the two nits above.

Comment 31

[Jim Mathies [:jimm]]

(In reply to comment #30)
> >+!macro AddDDEDisabledHandlers
> nit: since this only adds one handler it shouldn't be plural so
> AddDDEDisabledHandler would be better

no problem, will update.

> 
> nit: the comment for the icon resource index / resource id is still a tad off.

I was under the impression the path was optional and an index into the exe could be provided. Found something on msdn related to com registration that states the path must be there. So I guess something along the lines of "full path to the executable name of the application and the resource index of the icon within the executable."

> I'm going to test this out before r+ing but I suspect the only things left are
> the two nits above.

Sounds good.

Comment 32

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

Comment on attachment 398225 [details] [diff] [review]
remove dde support v.4

Everything looks good. I'd like a chance to verify this works on Win2K if you haven't checked already. How about the following for the comments for the icon.
The path to the binary that contains the icon group for the default icon followed by a comma and either the icon group's resource index or the icon group's resource id prefixed with a minus sign.

It is a tad wordy but I believe it is entirely correct.

Comment 33

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

Comment on attachment 398225 [details] [diff] [review]
remove dde support v.4

r=me contingent that this has been verified to work properly on Win2K. I'm mostly concerned about the empty string which is used to disable DDE actually disables DDE on Win2K. To verify this the HKLM\Software\classes\http keys should be set to IE and the HKCU\Software\classes\http should be set to Firefox. Then launch an url from start -> run without IE or Firefox running.

Comment 34

[James May [:fowl2]]

Won't this cause responsiveness issues when opening URLs from external apps?

Comment 35

[Jim Mathies [:jimm]]

(In reply to comment #34)
> Won't this cause responsiveness issues when opening URLs from external apps?

I didn't notice any major differences just testing manually. We do have to jump through the additional hoop now of launching the exe and using our internal IPC messaging to communicate to the open command, so slower machines or as Rob noted those that are heavily tasked may experience slow downs. I think getting this onto trunk to see what problems bubble to the surface would be useful. If we find a lot of issues, or get a lot of complaints we can revisit.

Comment 36

[Jim Mathies [:jimm]]

(In reply to comment #33)
> (From update of attachment 398225 [details] [diff] [review])
> r=me contingent that this has been verified to work properly on Win2K. I'm
> mostly concerned about the empty string which is used to disable DDE actually
> disables DDE on Win2K. To verify this the HKLM\Software\classes\http keys
> should be set to IE and the HKCU\Software\classes\http should be set to
> Firefox. Then launch an url from start -> run without IE or Firefox running.

Will test again with the final in a 2K image today and post back.

Comment 37

[Jim Mathies [:jimm]]

(In reply to comment #36)
> (In reply to comment #33)
> 
> Will test again with the final in a 2K image today and post back.

Ok, so there's still a minor problem on 2K. The shell was still convinced we wanted it to use DDE. The problem seems to be the empty ddeexec key that we leave in current user. Deleting that by hand fixed the problem.

This is the state of the reg after we have set ourselves to the default through the install w/ie set as the default previously:

HKEY_CLASSES_ROOT:

[HKEY_CLASSES_ROOT\http\shell]

[HKEY_CLASSES_ROOT\http\shell\open]

[HKEY_CLASSES_ROOT\http\shell\open\command]
@="\"C:\\Program Files\\Minefield\\firefox.exe\" -osint -url \"%1\""

[HKEY_CLASSES_ROOT\http\shell\open\ddeexec]
@=""

[HKEY_CLASSES_ROOT\http\shell\open\ddeexec\Application]
@="IExplore"

[HKEY_CLASSES_ROOT\http\shell\open\ddeexec\Topic]
@="WWW_OpenURL"

--

HKEY_LOCAL_MACHINE:

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\http\Shell\open\ddeexec]

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\http\Shell\open\ddeexec\Application]
@="IExplore"

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\http\Shell\open\ddeexec\Topic]
@="WWW_OpenURL"

(I would still like to blow these left over ie keys away personally, but they shouldn't interfer, they just look confusing to anyone opening the regedit.)

--

HKEY_CURRENT_USER:

[HKEY_CURRENT_USER\Software\Classes\http\shell]

[HKEY_CURRENT_USER\Software\Classes\http\shell\open]

[HKEY_CURRENT_USER\Software\Classes\http\shell\open\command]
@="\"C:\\Program Files\\Minefield\\firefox.exe\" -osint -url \"%1\""

[HKEY_CURRENT_USER\Software\Classes\http\shell\open\ddeexec]
@=""

--

Deleting the above ddeexec key fixed an error dialog problem when opening via start -> run. Deleting the local machine ddeexec key didn't change anything but it did purge all the ie jumk out of classes root under our registration.

Rob, any comments? Looks like 2k expect this to go away.

Comment 38

[Jim Mathies [:jimm]]

Actually, the only change is the empty string under ddeexec in current user. the ddeexec key should be empty and should not have a default set.

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #38)
> Actually, the only change is the empty string under ddeexec in current user.
> the ddeexec key should be empty and should not have a default set.

After doing some testing, it turns out XP requires the empty default string or an error dialog is shown. Since we no longer officially support 2K, I'm going to keep the default string so that XP works correctly.

Vista and Win7 don't seem to mind either config and work correctly regardless.

Comment 40

[Jim Mathies [:jimm]]

Attachment: remove dde support v.5

updated with various changes.

Comment 41

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

I was afraid of that. The problem with this on Win2K is that IE may have its entries in HKLM\Software\Classes\http etc. which will include the ddeexec keys while we would have our entries in HKCU\Software\Classes\http. The end result under HKCR is HKCR\http\<our entries> except for HKCR\http\ddeexec\<IE's entries>.

Does this also happen on XP?

Looks like at least Win2K will need to be handled differently that Vista and above.

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #41)
> Looks like at least Win2K will need to be handled differently that Vista and
> above.

The patch I just posted works fone on XP and up. Is there a reliable way to detect 2K in the nsis install? In which case I can just do an if statement on the commented line for 2k:

https://bugzilla.mozilla.org/attachment.cgi?id=398718&action=diff
line 1266

Comment 43

[Jim Mathies [:jimm]]

This looks like it might do the trick:

http://nsis.sourceforge.net/Get_Windows_version

Comment 44

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

WinVer.nsh provides this already and you can see one of many examples of usage here
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/shared.nsh#576

Comment 45

[Jim Mathies [:jimm]]

Attachment: remove dde support v.5

tested on 2k and xp, works good. I think that should do it.

Comment 46

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

Do we know this behavior for the ddeexec keys wasn't introduced with an XP service pack?

Comment 47

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

Comment on attachment 398746 [details] [diff] [review]
remove dde support v.5

AddDDEHandlerValues will need to be called for the case where the OS doesn't support disabling DDE with an empty string.

Comment 48

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

bah... the requestPending etc. crap will still be needed for Win2K if this is the case. We should probably regroup on how best to handle this and figure out which OS / service pack combinations support disabling DDE with this method. Also, installing Opera on a Win2K system might provide a hint on how to disable it on Win2K.

Comment 49

[Jim Mathies [:jimm]]

(In reply to comment #47)
> (From update of attachment 398746 [details] [diff] [review])
> AddDDEHandlerValues will need to be called for the case where the OS doesn't
> support disabling DDE with an empty string.

Why? In this case (2K) we delete the default string and everything works fine without the need for dde.

Comment 50

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

Read comment #41. We can't always delete the value from HKLM due to permissions

Comment 51

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

btw: it is possible that Win2K doesn't use HKCR and instead looks in HKCU then HKLM which would negate what I stated but I would be surprised if it did so.

Comment 52

[Jim Mathies [:jimm]]

(In reply to comment #50)
> Read comment #41. We can't always delete the value from HKLM due to permissions

(In reply to comment #51)
> btw: it is possible that Win2K doesn't use HKCR and instead looks in HKCU then
> HKLM which would negate what I stated but I would be surprised if it did so.

(In reply to comment #50)
> Read comment #41. We can't always delete the value from HKLM due to permissions

I would think that if we write to current user, our ddeexec values (default string or no default string) would propagate to classes root. 

I've noticed different behavior in propagation too, which makes this a bit more complex. For example:

1) 2K propagates when cu / lm are written to but not when the handler is used.
2) XP - ??
2) XPSP2 and up propagate when written and when the handler is used.

Let me go back and try and document all the behavior, and also confirm whether empty ddeexec default values propagate to classes root. 

We could delete classes root once at the beginning of the install to be sure old ie entries are blown away for 2K. Although we might need some sort of handler state machine for tracking internally what we modify. Then at the end it could do all the resetting, deleting, and writing in one shot.

Comment 53

[Jim Mathies [:jimm]]

also, my vmware images were all sp2, so im building up a non-sp xp image so I can test that as well. probably won't post back here till the weekend as this will take some time to fully test.

Comment 54

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

(In reply to comment #52)
>...
> I would think that if we write to current user, our ddeexec values (default
> string or no default string) would propagate to classes root. 
Keep in mind that this is MS and the registry...

HKCR is a combination of HKLM\Software\Classes and HKCU\Software\Classes where *values* in HKCU\Software\Classes win. Hence the need for the empty string on XP (perhaps some service pack) and above to disable dde.

> I've noticed different behavior in propagation too, which makes this a bit more
> complex. For example:
> 
> 1) 2K propagates when cu / lm are written to but not when the handler is used.
I don't know what you mean by this.

> 2) XP - ??
> 2) XPSP2 and up propagate when written and when the handler is used.
> 
> Let me go back and try and document all the behavior, and also confirm whether
> empty ddeexec default values propagate to classes root. 
They do at least on XP but there may be different behavior on previous version of Windows.

> We could delete classes root once at the beginning of the install to be sure
> old ie entries are blown away for 2K. Although we might need some sort of
> handler state machine for tracking internally what we modify. Then at the end
> it could do all the resetting, deleting, and writing in one shot.
The user performing the install may not be running with privs to do this.

Comment 55

[Jim Mathies [:jimm]]

(In reply to comment #54)
> (In reply to comment #52)
> >...
> > I would think that if we write to current user, our ddeexec values (default
> > string or no default string) would propagate to classes root. 
> Keep in mind that this is MS and the registry...
> 
> HKCR is a combination of HKLM\Software\Classes and HKCU\Software\Classes where
> *values* in HKCU\Software\Classes win. Hence the need for the empty string on
> XP (perhaps some service pack) and above to disable dde.

That's what I'm curious about. Simply writing an empty string and then deleting it may well fix the problem though, see below.

> 
> > I've noticed different behavior in propagation too, which makes this a bit more
> > complex. For example:
> > 
> > 1) 2K propagates when cu / lm are written to but not when the handler is used.
> I don't know what you mean by this.

On XP and 2K, CR is written to when we write to LM or CU. On XP however if you go in and manually delete CR entries, the next time a handler is used XP will propagate the entries back to CR from LM/CU. On 2K that didn't seem to be there, if deleted manually, they stayed deleted until LM/CU were re-written to.

> > We could delete classes root once at the beginning of the install to be sure
> > old ie entries are blown away for 2K. Although we might need some sort of
> > handler state machine for tracking internally what we modify. Then at the end
> > it could do all the resetting, deleting, and writing in one shot.
> The user performing the install may not be running with privs to do this.

To delete CR entries?

Comment 56

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

It inherits the permissions from the originating key iirc

Comment 57

[Jim Mathies [:jimm]]

Rob, what's is our target for registration, local machine or current user? Looking at the code here while working on a fix for this, it looks like we default to local machine registration any time we have access -

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#328

Often times an installer will ask if the user wants to target the entire machine or just the user, but we don't seem to have an option like that. Curious if a at some point we decided to default to local machine for better uptake?

Comment 58

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

It sets the base keys needed by Firefox under HKLM (which includes HKLM\Software\Clients\StartMenuInternet if possible and will set them under HKCU if not. Setting as default for the user is done under HKCU
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#469

Comment 59

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

Keep in mind that HKLM keys are needed to set as default on Vista and above and it will prompt for elevation if the keys don't exist / are incorrect

Comment 60

[Jim Mathies [:jimm]]

Attachment: remove dde support v.6

Changes:

- Added commenting as I was working through this. Some of this may seem like over kill but for folks who haven't worked on this it can be pretty invaluable info.

- Added a handy HKLM write access macro and made use of it throughout

- Cleaned up initial install steps. One of the things I noticed is that by default we were always setting the prog ids in HKLM, which was messing up installs of other versions on operating systems prior to Vista.

- Fixed the 2K issue. On 2K, with these changes, we do not set the browser to the default unless we have HKLM access. The install, update, and set as default routines also clean up prog ids, and file and protocol handlers is HKCU to prevent conflict. (I still need to do something about the default prompt on first open. I'm thinking of disabling the prompt and disabling the check button in settings as well which shouldn't be too hard to do.)

I've also put together a test matrix I still need to walk through to be sure everything is working properly on all configs. I'll post back once I complete that.

Comment 61

[Jim Mathies [:jimm]]

Attachment: remove dde support v.7

Fixed a couple typos and a bug in UpdateHandlers.

Comment 62

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

(In reply to comment #60)
>...
> - Added a handy HKLM write access macro and made use of it throughout
note: remember that I had to check a specific key's write access per your request in a separate bug due to other installer's changing perms in the registry.

> - Cleaned up initial install steps. One of the things I noticed is that by
> default we were always setting the prog ids in HKLM, which was messing up
> installs of other versions on operating systems prior to Vista.
I don't necessarily think this is the correct thing to do since the last install IMO should win.

Also, this isn't technically a progid
http://msdn.microsoft.com/en-us/library/aa911706.aspx

> - Fixed the 2K issue. On 2K, with these changes, we do not set the browser to
> the default unless we have HKLM access. The install, update, and set as default
> routines also clean up prog ids, and file and protocol handlers is HKCU to
> prevent conflict. (I still need to do something about the default prompt on
> first open. I'm thinking of disabling the prompt and disabling the check button
> in settings as well which shouldn't be too hard to do.)
This isn't really a fix IMO. This just removes support to make Firefox the default handler on Win2K.

Comment 63

[Jim Mathies [:jimm]]

(In reply to comment #62)
> (In reply to comment #60)
> >...
> > - Added a handy HKLM write access macro and made use of it throughout
> note: remember that I had to check a specific key's write access per your
> request in a separate bug due to other installer's changing perms in the
> registry.

Well the test is the same write key test we had before, I just converted it to a macro. So really nothing has changed.

> > - Cleaned up initial install steps. One of the things I noticed is that by
> > default we were always setting the prog ids in HKLM, which was messing up
> > installs of other versions on operating systems prior to Vista.
> I don't necessarily think this is the correct thing to do since the last
> install IMO should win.

If 3.5 is installed and set as the machine default, and we install Minefield for a single account through the checkbox, we update HKLM's FirefoxURL and FirefoxHTML. That changes the default file handler for all accounts. That didn't seem right -

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#333

Why not update FirefoxHTML/FirefoxURL only in HKCU, and only if we are setting the install as the default? That seemed like a better, more machine friendly config  practice.

> 
> Also, this isn't technically a progid
> http://msdn.microsoft.com/en-us/library/aa911706.aspx
> 

Well, our use may not fit perfectly with the com definition of prog ids, but the closest and most accurate description of FirefoxHTML and FirefoxURL are "version independent ProgIDs". 

http://msdn.microsoft.com/en-us/library/bb166549%28VS.80%29.aspx

I can change the name of the routine though if you want to something else. ProgID to me made the most sense. 

> > - Fixed the 2K issue. On 2K, with these changes, we do not set the browser to
> > the default unless we have HKLM access. The install, update, and set as default
> > routines also clean up prog ids, and file and protocol handlers is HKCU to
> > prevent conflict. (I still need to do something about the default prompt on
> > first open. I'm thinking of disabling the prompt and disabling the check button
> > in settings as well which shouldn't be too hard to do.)
> This isn't really a fix IMO. This just removes support to make Firefox the
> default handler on Win2K.

True, it doesn't solve the problem, it gets around it. But, the fix would require enabling all the DDE stuff again. IMHO, not the right option for a few  reasons - 

1) We're dealing with an install base that's very small. 2K represents about 1.4% of our daily downloads, of those, an even smaller percentage are running under restricted accounts.

2) Technically we don't support 2K any more, so I don't think limitations in 2K should prevent code cleanup and better integration for the operation systems we do support.

3) The fix doesn't break anything, it just limits the ability for 2K users under restricted account from setting 3.6 as the default. They can set it as the default by logging in as an admin and running the install or setting it as the default browser. That seemed like a reasonably trade off.

Comment 64

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

You've convinced me for the most part.

I'm wondering if we could launch helper.exe with runas or have it perform the runas and launch a second instance of itself to set the required keys on Win2k?

Also, this change would need to be in the release notes.

cc'ing beltzner to give him a heads up about this change

Comment 65

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm fine with this, yeah. Rationale in comment 63 and 64 seems quite sound. Thanks for checking.

Comment 66

[Jim Mathies [:jimm]]

(In reply to comment #64)
> You've convinced me for the most part.
> 
> I'm wondering if we could launch helper.exe with runas or have it perform the
> runas and launch a second instance of itself to set the required keys on Win2k?
> 
> Also, this change would need to be in the release notes.
> 
> cc'ing beltzner to give him a heads up about this change

I think a runas in the set default option from the shell code would be good when HKLM access is restricted. With the checkbox hidden I don't think there's much need for something in the installer.

Poking around, looks like Administrator was a default account for 2K. I wonder if it's safe to use that or if I should query somehow for the user info. I'll do some research and see what I can come up with.

Comment 67

[Jim Mathies [:jimm]]

Hey Rob, why do we point the windows ReinstallCommand control panel command to SetAsDefaultAppGlobal? Seems like it should point to SetAsDefaultAppUser?

Comment 68

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

The ReinstallCommand is typically used for system level defaults though some apps have abused it just as some have abused setting as the user default by taking over system level defaults. Also note that the ReinstallCommand is only available to admin users on Vista and above.
http://msdn.microsoft.com/en-us/library/cc144109%28VS.85%29.aspx

Comment 69

[Jim Mathies [:jimm]]

(In reply to comment #68)
> The ReinstallCommand is typically used for system level defaults though some
> apps have abused it just as some have abused setting as the user default by
> taking over system level defaults. Also note that the ReinstallCommand is only
> available to admin users on Vista and above.
> http://msdn.microsoft.com/en-us/library/cc144109%28VS.85%29.aspx

Thanks for the clarification.

I'm testing runas usage for 2K at the moment and doing some final testing, should have a final patch here in a bit.

Comment 70

[Jim Mathies [:jimm]]

try builds - 

http://build.mozilla.org/tryserver-builds/[e-mail]-491947v7

Comment 71

[Jim Mathies [:jimm]]

Attachment: remove dde support v.7

Comment 72

[Jim Mathies [:jimm]]

Comment on attachment 399831 [details] [diff] [review]
remove dde support v.7

Tested on various systems and accounts, all seems to be working well.

Comment 73

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

Jim, I should be able to get to this review today (9/17)

Comment 74

[Jim Mathies [:jimm]]

(In reply to comment #73)
> Jim, I should be able to get to this review today (9/17)

Thanks. One possible change to this after thinking about it would be to ditch the two calls with /SetAsDefaultAppUser for 2K users and instead create a new 2K specific entry in helper.exe that somehow calls back into helper.exe through createprocess with runas. (you suggested this originally.) firing off a new instance with runas through the install was something I wasn't sure how to do, but if it's possible it'd probably be a better solution.

Also, FYI, jump lists currently are nominated for blocking but haven't received it yet, so 1.9.2 blocking bugs are currently the priority.

Comment 75

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

Comment on attachment 399831 [details] [diff] [review]
remove dde support v.7

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
>--- a/browser/components/nsBrowserContentHandler.js
>+++ b/browser/components/nsBrowserContentHandler.js
>@@ -702,20 +702,16 @@ var nsDefaultCommandLineHandler = {
>     if (!iid.equals(nsISupports) &&
>         !iid.equals(nsICommandLineHandler) &&
>         !iid.equals(nsIFactory))
>       throw Components.results.NS_ERROR_NO_INTERFACE;
> 
>     return this;
>   },
> 
>-  // List of uri's that were passed via the command line without the app
>-  // running and have already been handled. This is compared against uri's
>-  // opened using DDE on Win32 so we only open one of the requests.
>-  _handledURIs: [ ],
> #ifdef XP_WIN
>   _haveProfile: false,
> #endif
Could you check if _haveProfile is still needed?

> 
>   /* nsICommandLineHandler */
>   handle : function dch_handle(cmdLine) {
>     var urilist = [];
> 
>@@ -739,35 +735,18 @@ var nsDefaultCommandLineHandler = {
>         cmdLine.preventDefault = true;
>       }
>     }
> #endif
> 
>     try {
>       var ar;
>       while ((ar = cmdLine.handleFlagWithParam("url", false))) {
>-        var found = false;
>         var uri = resolveURIInternal(cmdLine, ar);
>-        // count will never be greater than zero except on Win32.
>-        var count = this._handledURIs.length;
>-        for (var i = 0; i < count; ++i) {
>-          if (this._handledURIs[i].spec == uri.spec) {
>-            this._handledURIs.splice(i, 1);
>-            found = true;
>-            cmdLine.preventDefault = true;
>-            break;
>-          }
>-        }
>-        if (!found) {
>-          urilist.push(uri);
>-          // The requestpending command line flag is only used on Win32.
>-          if (cmdLine.handleFlag("requestpending", false) &&
>-              cmdLine.state == nsICommandLine.STATE_INITIAL_LAUNCH)
>-            this._handledURIs.push(uri)
>-        }
>+        urilist.push(uri);
>       }
>     }
>     catch (e) {
>       Components.utils.reportError(e);
>     }
> 
>     count = cmdLine.length;
> 
>diff --git a/browser/components/shell/src/nsSetDefaultBrowser.js b/browser/components/shell/src/nsSetDefaultBrowser.js
>--- a/browser/components/shell/src/nsSetDefaultBrowser.js
>+++ b/browser/components/shell/src/nsSetDefaultBrowser.js
>@@ -31,36 +31,45 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> /* 
>- * -setDefaultBrowser commandline handler
>+ * -setDefaultBrowser, -setDefaultGlobalBrowser commandline handlers
>  * Makes the current executable the "default browser".
>  */
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> 
> function nsSetDefaultBrowser() {}
> 
> nsSetDefaultBrowser.prototype = {
>   handle: function nsSetDefault_handle(aCmdline) {
>     if (aCmdline.handleFlag("setDefaultBrowser", false)) {
>       var shell = Cc["@mozilla.org/browser/shell-service;1"].
>                   getService(Ci.nsIShellService);
>+      // 1st param: claim all file types - true
>+      // 2nd param: claim for entire computer - false
>+      shell.setDefaultBrowser(true, false);
>+    } else
>+    if (aCmdline.handleFlag("setDefaultGlobalBrowser", false)) {
>+      var shell = Cc["@mozilla.org/browser/shell-service;1"].
>+                  getService(Ci.nsIShellService);
>+      // 1st param: claim all file types - true
>+      // 2nd param: claim for entire computer - true
>       shell.setDefaultBrowser(true, true);
Have you verified this works correctly on other platforms?
You are also changing the original behavior for setDefaultBrowser which I don't think you want to do.

>     }
>   },
> 
>-  helpInfo: "  -setDefaultBrowser   Set this app as the default browser.\n",
>+  helpInfo: "  -setDefaultBrowser   Set as the default browser for this user.\n  -setDefaultGlobalBrowser   Set as the default browser for the entire computer.\n",
way too long... this should be valid
helpInfo: "  -setDefaultBrowser   Set as the default browser for this user.\n" +
          "  -setDefaultGlobalBrowser   Set as the default browser for the entire computer.\n",

I suggest you make these changes as a separate bug.

Comment 76

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

Comment on attachment 399831 [details] [diff] [review]
remove dde support v.7

This new patch introduces several new issues that I don't believe were present before... I'm going to point out the main one below along with some comment nits so you can address this before the review is finished. I'll go through the remainder as time permits as well.

Also, I'd prefer to keep the runas logic in the helper if possible. I believe it is and will try to provide code for it.

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -29,88 +29,150 @@
> # use your version of this file under the terms of the MPL, indicate your
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
>+; code in browser to disable prompt
Not sure what this means.

>+
>+; Default program registration overview:
>+;
>+; Prog IDs (FirefoxHTML, FirefoxURL):
>+; [Function RegisterProgIDs]
>+; 
>+;        HKCU    HKLM
>+; 2K       No(1)  Yes*
>+; XP      Yes*    Yes(2)
>+; Vista   Yes*    Yes(2)
>+; Win7    Yes*    Yes(2)
>+; 
>+; File Handlers (.htm, .html, .shtml, .xht, .xhtml)
>+; [Function SetFileHandlers]
>+; Note: These rely on the FirefoxHTML prog id
>+; 
>+;        HKCU    HKLM
>+; 2K       No(1)  Yes*
>+; XP      Yes*    Yes(2)
>+; Vista   Yes*    Yes(2)
>+; Win7    Yes*    Yes(2)
>+; 
>+; Protocol Handlers (ftp, http, https)
>+; [Function SetProtocolHandlers]
>+; 
>+;        HKCU    HKLM
>+; 2K       No(1)  Yes*
>+; XP      Yes*    Yes(2)
>+; Vista   Yes*    Yes(2)
>+; Win7    Yes*    Yes(2)
>+; 
>+; Vista capabilities entries (3)
>+; [Function SetStartMenuInternet]
>+; Note: These rely on Prog IDs
>+; 
>+;        HKCU    HKLM
>+; Vista    -      Yes*
>+; Win7     -      Yes*
>+; 
>+; * - preferred location
>+; 1 - 2K can't be set as the default unless we have HKLM access. CU
Let's stick with HKCU since it is used everywhere else.
>+;     entries conflict with other DDE related entries of IE.
>+; 2 - Normally we default to HKCU registration. The SetAsDefaultAppGlobal
>+;     command line option is used to set the default for all users which
>+;     is rarely used.
should be "which is used by administrators to set the system default browser."

>+; 3 - Vista and up use app registration apis for setting program defaults.
Should make it clear that we also set the pre Vista registry keys due to bug 369703

>@@ -167,23 +229,68 @@
>...
>   ${EndUnless}
>   ${Unless} ${FileExists} "$QUICKLAUNCH\${BrandFullName}.lnk"
>     CreateShortCut "$QUICKLAUNCH\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}" "" "$INSTDIR\${FileMainEXE}" 0
>     ShellLink::SetShortCutWorkingDirectory "$QUICKLAUNCH\${BrandFullName}.lnk" "$INSTDIR"
>   ${EndUnless}
> !macroend
> !define ShowShortcuts "!insertmacro ShowShortcuts"
> 
>-; Adds the protocol and file handler registry entries for making Firefox the
>-; default handler (uses SHCTX).
>-!macro SetHandlers
>+; Clean up handler registration completely.
>+!macro PurgeHandlers
>+  ; Purge any old prog id keys in CU & LM.
Here and elsewhere... let's stick with HKCU and HKLM since these are used everywhere else.

>...
>+  ${RegForceCleanHandler} ".htm" ; Does not use SHCTX
>+  ${RegForceCleanHandler} ".html"
>+  ${RegForceCleanHandler} ".shtml"
>+  ${RegForceCleanHandler} ".xht"
>+  ${RegForceCleanHandler} ".xhtml"
Looks like you remove these even when we don't own them.

Also, we shouldn't be removing everything from these keys... only the values we set.

>+
>+  ; Purge any protocol keys in CU & LM.
Let's stick with HKCU and HKLM since these are used everywhere else.

>+  ${RegForceCleanHandler} "ftp" ; Does not use SHCTX
>+  ${RegForceCleanHandler} "http"
>+  ${RegForceCleanHandler} "https"
see above

>@@ -565,37 +655,68 @@ Function SetAsDefaultAppUserHKCU
>     ${If} ${FileExists} "$0"
>       ${GetLongPath} "$0" $0
>       ${If} "$0" == "$INSTDIR"
>         WriteRegStr HKCU "Software\Clients\StartMenuInternet" "" "$R9"
>       ${EndIf}
>     ${EndIf}
>   ${EndUnless}
> 
>-  SetShellVarContext current  ; Set SHCTX to the current user (e.g. HKCU)
>-  ${SetHandlers}
>+  ; Track whether HKLM is writable
>+  ${LocalMachineWritable} $HKLMWritable
>+
>+  ${If} "$HKLMWritable" == "false"
>+    ${If} ${IsWin2000}
>+      ; No access to HKLM. 2K cannot support current user registration
>+      ; now that dde is disabled. If we are calling in from Fx, we'll
>+      ; call in here twice, once as admin, and once as the user. Purge
>+      ; user keys here so they do not confict with HKLM.
>+      ${PurgeHandlers}
>+      goto EndRegistration 
>+    ${EndIf}
>+  ${EndIf}
>+
>+  ; On 2K, alway default to HKLM, otherwise, default HKCU.
>+  ${If} ${IsWin2000}
>+    SetShellVarContext all
>+  ${Else}
>+    SetShellVarContext current
>+  ${EndIf}
>+  ${RegisterProgIDs} ; uses SHCTX
>+  ${SetFileHandlers} ; uses SHCTX
>+  ${SetProtocolHandlers} ; uses SHCTX
>+  EndRegistration:
>+
>+  ; Touch up some file and content type registry entries we rely on for .xhtml.
>+  ${FixClassKeys} ; Does not use SHCTX
>+
>+  ; Touch up app icon issues from other installs if needed.
The issue isn't actually due to the other install. It is due to MS Office not properly updating the path properly when there are multiple install locations.

Comment 77

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

In case it isn't obvious why we can't just delete the handler's root key and everything underneath it for starters think about open with keys... every user / app set open with key would be deleted using the method in this patch.

Comment 78

[Jim Mathies [:jimm]]

(In reply to comment #76)
> (From update of attachment 399831 [details] [diff] [review])
> This new patch introduces several new issues that I don't believe were present
> before... I'm going to point out the main one below along with some comment
> nits so you can address this before the review is finished. I'll go through the
> remainder as time permits as well.
> 
> Also, I'd prefer to keep the runas logic in the helper if possible. I believe
> it is and will try to provide code for it.
> 
> >diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
> >--- a/browser/installer/windows/nsis/shared.nsh
> >+++ b/browser/installer/windows/nsis/shared.nsh
> >@@ -29,88 +29,150 @@
> > # use your version of this file under the terms of the MPL, indicate your
> > # decision by deleting the provisions above and replace them with the notice
> > # and other provisions required by the GPL or the LGPL. If you do not delete
> > # the provisions above, a recipient may use your version of this file under
> > # the terms of any one of the MPL, the GPL or the LGPL.
> > #
> > # ***** END LICENSE BLOCK *****
> > 
> >+; code in browser to disable prompt
> Not sure what this means.

That's a left over todo comment, will removed.

> Let's stick with HKCU since it is used everywhere else.

(in all cases, will do)

> >...
> >+  ${RegForceCleanHandler} ".htm" ; Does not use SHCTX
> >+  ${RegForceCleanHandler} ".html"
> >+  ${RegForceCleanHandler} ".shtml"
> >+  ${RegForceCleanHandler} ".xht"
> >+  ${RegForceCleanHandler} ".xhtml"
> Looks like you remove these even when we don't own them.

The only time this would happen is if we are in the process of taking them over.

> 
> Also, we shouldn't be removing everything from these keys... only the values we set.

Well, I'm not clearing these to set something in most cases, I want them completely empty so the system, (2k in this case) falls back on HKLM. But I get what you're saying. I can try to selectively clear certain items and leave anything else.

Comment 79

[Jim Mathies [:jimm]]

(In reply to comment #76)
> (From update of attachment 399831 [details] [diff] [review])
> This new patch introduces several new issues that I don't believe were present
> before... I'm going to point out the main one below along with some comment
> nits so you can address this before the review is finished. I'll go through the
> remainder as time permits as well.
> 
> Also, I'd prefer to keep the runas logic in the helper if possible. I believe
> it is and will try to provide code for it.
> 

If you have a use of that in there somewhere that I can look at that would be great, otherwise I can figure it out.

Comment 80

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

I recall doing this a while back using the system dll but it may be possible using the UAC dll which encompasses more than UAC capabilities.

Comment 81

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

Comment on attachment 399831 [details] [diff] [review]
remove dde support v.7

>+  ${If} "$HKLMWritable" == "false"
>+    ${If} ${IsWin2000}
>+      ; No access to HKLM. 2K cannot support current user registration
>+      ; now that dde is disabled. If we are calling in from Fx, we'll
>+      ; call in here twice, once as admin, and once as the user. Purge
>+      ; user keys here so they do not confict with HKLM.
>+      ${PurgeHandlers}
Would this ever get called except when you don't have write access to HKLM (e.g. $HKLMWritable is false)?

Comment 82

[Jim Mathies [:jimm]]

(In reply to comment #81)
> (From update of attachment 399831 [details] [diff] [review])
> >+  ${If} "$HKLMWritable" == "false"
> >+    ${If} ${IsWin2000}
> >+      ; No access to HKLM. 2K cannot support current user registration
> >+      ; now that dde is disabled. If we are calling in from Fx, we'll
> >+      ; call in here twice, once as admin, and once as the user. Purge
> >+      ; user keys here so they do not confict with HKLM.
> >+      ${PurgeHandlers}
> Would this ever get called except when you don't have write access to HKLM
> (e.g. $HKLMWritable is false)?

Yeah this is the little bit of code I didn't really like. It's in SetAsDefaultAppUserHKCU, so users might trigger it manually. Hence the reason why I like the idea of having helper spawn runas to register in HKLM, and then if that succeeds, call PurgeHandlers for HKCU to blow away everything there. (for 2K)

Comment 83

[Jim Mathies [:jimm]]

SetAsDefaultAppUser could detect 2K, and call a new routine specific to that platform that did all this, and SetAsDefaultAppUserHKCU could be left alone.

Comment 84

[Jim Mathies [:jimm]]

Attachment: remove dde support v.8

Updated. I removed all the purging code and instead just made sure to write data to the right locations.

One note - GetOptions in the installer - funny routine, it will match 'XYZ' and 'XYZA' to 'XYZ' for some reason, which explains the funny '2KSetAsDefaultAppUser' command line switch.

Comment 85

[Mark Smith [:mcs]]

I just experienced a DDE-related problem with Firefox for the first time.  I assume the v.8 patch would fix it.  It looks like it just needs a final review?  Do you anticipate any problems after DDE is removed? (I saw someone -- maybe Jim Mathies -- mention in a much older bug that some applications depend on it).

Comment 87

[Jim Mathies [:jimm]]

Rob, any current feelings on this? Maybe update this and land it on trunk once we branch 4.0?

Comment 88

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

I'm a tad concerned about supporting both the Win2K and everything else paths. Having said that, I think it will be ok to do this after things calm down after the 4.0 branch

Comment 89

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

btw: When we switch to msvc 2010 we'll drop support for Win2k and this will hopefully happen after Firefox 4.

Comment 90

[Jim Mathies [:jimm]]

Comment on attachment 406222 [details] [diff] [review]
remove dde support v.8

dropping this request for now.

Comment 91

[johneasy]

According to Bug #495488 Firefox has been having this problem since before 2009-06-02, And Firefox has not fixed it YET!!!!!!!

This is as bad as Microsoft and I am leaving Firefox until they get this FIXED............

Comment 92

[colin glenn]

Attachment: Desktop Launch URL Error

Would this DDE issue cause this (see image) when starting a fresh Firefox session from either an associated file extension or via a desktop URL to a website?

This only happens if Firefox detected updates to any extensions and does the pause on launch to display them so the user could choose to install or skip.

Comment 93

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

Removing Windows DDE support should prevent that dialog from being displayed.

Comment 94

[Jim Mathies [:jimm]]

(In reply to comment #89)
> btw: When we switch to msvc 2010 we'll drop support for Win2k and this will
> hopefully happen after Firefox 4.

We stopped officially supporting 2k in 4.0.

+; Default program registration overview:
+;
+; Prog IDs (FirefoxHTML, FirefoxURL):
+; [Function RegisterProgIDs]
+; 
+;        HKCU    HKLM
+; 2K       No(1)  Yes*
+; XP      Yes*    Yes(2)
+; Vista   Yes*    Yes(2)
+; Win7    Yes*    Yes(2)
+; 
+; File Handlers (.htm, .html, .shtml, .xht, .xhtml)
+; [Function SetFileHandlers]
+; Note: These rely on the FirefoxHTML prog id
+; 
+;        HKCU    HKLM
+; 2K       No(1)  Yes*
+; XP      Yes*    Yes(2)
+; Vista   Yes*    Yes(2)
+; Win7    Yes*    Yes(2)
+; 
+; Protocol Handlers (ftp, http, https)
+; [Function SetProtocolHandlers]
+; 
+;        HKCU    HKLM
+; 2K       No(1)  Yes*
+; XP      Yes*    Yes(2)
+; Vista   Yes*    Yes(2)
+; Win7    Yes*    Yes(2)
+; 
+; App capabilities entries
+; [Function SetStartMenuInternet]
+; Note: These rely on Prog IDs
+; 
+;        HKCU    HKLM
+; Vista    -      Yes*
+; Win7     -      Yes*
+; 
+; * - preferred location
+; 1 - 2K can't be set as the default unless we have HKLM access. HKCU
+;     entries conflict with other DDE related entries of IE.
+; 2 - Normally we default to HKCU registration. The SetAsDefaultAppGlobal
+;     command line option can be used by administrators to set the system
+;     default browser in HKLM.

We can still work on 2K as long as we have HKLM access. If we don't we can't register on 2K with these patches. That seems like an acceptable trade-off. 

I'd love to get this finished up and landed in 7.0.

Comment 95

[Phoenix]

(In reply to Jim Mathies [:jimm] from comment #94)
> I'd love to get this finished up and landed in 7.0.
Seems like 10.0 or later...

Comment 96

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

Attachment: remove dde support v.9

The only potentially controversial change I have made is disabling DDE in nsWindowsShellService.cpp when Firefox is already the default browser so users that already have Firefox set as the default browser but didn't perform an update will automatically disable DDE. I was thinking about using a pref so this check is only performed once. I *think* we will want to do a similar thing for the open command since the -requestPending command line arg is being removed and will cause these users to be prompted to set Firefox as the default when it is already the default.

Comment 97

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

Attachment: remove dde shell integration v.10

The main change is that when Firefox is already the default browser we set the HKCU keys to disable shell integration and update the shell\open\command for protocols when needed. This handles the case where a secondary user on the system didn't get these updated.

I didn't spend much time trying to clean up the code. I would very much like to convert this entirely to a JavaScript component and would prefer spending the time cleaning it up converting it to JavaScript instead. This way we can get rid of this extra app specific dll entirely!

Comment 98

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

(In reply to Robert Strong [:rstrong] (do not email) from comment #97)
> Created attachment 604314 [details] [diff] [review]
> remove dde shell integration v.10
> 
> The main change is that when Firefox is already the default browser we set
> the HKCU keys to disable shell integration and update the shell\open\command
> for protocols when needed. This handles the case where a secondary user on
> the system didn't get these updated.
In case it isn't clear, this makes it so we don't reprompt to become the default browser when we are already default.

Comment 99

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

Filed bug 734331 to convert shell integration into a JavaScript component.

Comment 100

[Martin Sch]

A few times when I closed firefox, I found it in the process explorer still running.
An the command line was ""C:\Program Files\Mozilla Firefox\firefox.exe" -requestPending -osint -url "http://someforum/showthread.php?t=123&goto=newpost"

This was an URL from thunderbird which I clicked. So it seems this starts a process which hangs then?

I'm running windows 7.

Comment 101

[colin glenn]

The issue I got with this is that when I'm using any hyperlinking process that calls on the browser and is telling the browser to go to this website, the browser should NOT be telling / asking me about any pending updates that need to be installed beforehand. My request comes first, ignore the other stuff until it is convenient to install them. Inform me about it LATER, but got to my url FIRST.

Comment 102

[Jim Mathies [:jimm]]

Comment on attachment 604314 [details] [diff] [review]
remove dde shell integration v.10

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

I tried a few tests going from older versions to an install built using this code. In all cases the dde keys went away and program launch still worked as expected.

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +1564,5 @@
> +      ;
> +      ; To disable dde, an empty shell/ddeexec key must be created in current
> +      ; user or local machine. Unfortunately, settings have various different
> +      ; behaviors depending on the windows version. The following code attempts
> +      ; to address these differecnes.

nit - 'differecnes'

Comment 103

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/09362c5dceaf

Comment 104

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/09362c5dceaf

Comment 105

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

Anthony / Juan: just a heads up that we have changed the way Firefox is launched when clicking urls external to Firefox (the behavior the user sees should be the same) in this bug. If you can, please keep a lookout for any fallout.

Comment 106

[Simona Badau, Desktop QA]

(In reply to Robert Strong [:rstrong] (do not email) from comment #105)
> Anthony / Juan: just a heads up that we have changed the way Firefox is
> launched when clicking urls external to Firefox (the behavior the user sees
> should be the same) in this bug. If you can, please keep a lookout for any
> fallout.

Mozilla/5.0 (Windows NT 6.0; rv:14.0) Gecko/20120401 Firefox/14.0a1

Tested different scenarios in opening external links using the latest Nightly on Win Vista - all the links opened without given any fails or error messages:

1. Launched local HTML files from my desktop and different applications (Thunderbird, Messenger);
2. Launched different links from My Computer, Start Search field;
3. Launched local HTML files after downloading an update (without restarting Firefox);
4. Launched local HTML files after crashing Firefox;
5. Launched local HTML files without closing Nightly.

Did this bug fix changed anything else that should be verified?

Comment 107

[colin glenn]

The bug check that needs to be checked is a launch of a desktop url shortcut and there's a pending update to an add-on, that's what does it to me every time.

Comment 108

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

colin, since you are able to reproduce it on your system and the vast majority of systems don't have a problem when using the steps you outlined in comment #107 could you check if you are still able to reproduce with a nightly build?

http://nightly.mozilla.org/

Comment 109

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

(In reply to Simona B [QA] from comment #106)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #105)
> > Anthony / Juan: just a heads up that we have changed the way Firefox is
> > launched when clicking urls external to Firefox (the behavior the user sees
> > should be the same) in this bug. If you can, please keep a lookout for any
> > fallout.
> 
> Mozilla/5.0 (Windows NT 6.0; rv:14.0) Gecko/20120401 Firefox/14.0a1
> 
> Tested different scenarios in opening external links using the latest
> Nightly on Win Vista - all the links opened without given any fails or error
> messages:
> 
> 1. Launched local HTML files from my desktop and different applications
> (Thunderbird, Messenger);
> 2. Launched different links from My Computer, Start Search field;
> 3. Launched local HTML files after downloading an update (without restarting
> Firefox);
> 4. Launched local HTML files after crashing Firefox;
> 5. Launched local HTML files without closing Nightly.
> 
> Did this bug fix changed anything else that should be verified?
That is sufficient though as pointed out by colin, if you were able to reproduce the problem where the system's dde isn't working properly (for example, bug 495988) then you would be able to verify that as well. Thanks!

Comment 110

[Noèl Köthe]

running Windows 7 Enterprise with Firefox ESR 24.5.0 in 2014 and still hit by this bug.:(

http://kb.mozillazine.org/Windows_error_opening_Internet_shortcut_or_local_HTML_file_-_Firefox#Registry_edit help me even it is always talking about Vista.

The ddeexec keys were still there and need to be deleted.:(