Closed Bug 353089 Opened 17 years ago Closed 17 years ago

Fix / remove ddeexec hack

Categories

(Firefox :: Shell Integration, defect)

Product:
Firefox
Component:
Shell Integration
Version:
2.0 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

(Keywords: verified1.8.1.2)

Attachments

(3 files)

patch - whitespacee
20.08 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
patch
21.59 KB, patch
Details | Diff | Splinter Review
patch as checked in
21.77 KB, patch
Details | Diff | Splinter Review 
For the file and protocol handlers on startup of the app we add the ddeexec registry keys and on shutdown of the app we remove the ddeexec registry keys. What ever this is trying to workaround should be fixed and this code removed.

http://lxr.mozilla.org/mozilla/source/browser/components/shell/src/nsWindowsShellService.cpp#306
btw: this applies to all versions of Windows and not just Vista
(see bug 246078 for some context)
So, a quick synopsis and check if my thinking is correct (I haven't touched dde in years)

When an app is not registered to receive dde messages the OS launches the executable as defined in the open\command registry key for the handler. Example reg key:
HHTP
  shell
    open
      command 
          (Default) -> C:\PROGRA~1\MOZILL~1\FIREFOX.EXE -url "%1" (path to exe along with params)
      ddeexec
          (Default) -> "%1",,0,0,,,, (default dde params)
        Application
            (Default) -> Firefox (app dde name)
        Ifexec (Not present with Firefox)
            (Default) -> (dde params to send when the app needs to be launched)
        Topic
            (Default) -> WWW_OpenURL (dde topic)

If the app is launched and the Ifexec key is not present it sends the value of Default under ddeexec. If the Ifexec key is present it sends that value instead.

A simple solution to this would be to add the Ifexec key with params that tell nsNativeAppSupportWin.cpp not to open a window. For example, specifying ,,0,0,,,, for the default value of Ifexec and checking if there is an url and if there isn't don't open a new window. Since the Topic is WWW_OpenURL I think this would be acceptable since if there is no url specified to open the caller shouldn't expect a new window to open. I also have this working in my tree.

Thoughts?
Do we need to support DDEExec at all? Can't we just have the OS launch "firefox.exe <URL>" and do the remoting ourself?
Some third party apps require dde.
I'm not sure I understand your proposed sequence when opening the app the "first" time. Can you present it as a timeline in terms of our startup sequence? I really want to avoid situations where there is a firefox process hanging around without any windows open.
The way the OS does this is as follows:
1) checks if the dde name is registered.

2.a) if it is registered sends the WWW_OpenURL topic with the default params and we are done.

2.b) if it isn't registered launches the exe defined in the verb's command reg key.
3.a) if the ifExec key is not present sends the WWW_OpenURL topic with the default params and we are done.
3.b) if the ifExec key is present sends the WWW_OpenURL topic with the params under ifExec and we are done.

Our startup would be along the lines of:
1) If the app is running same our current behavior and we are done.
2) If the app is not running it is launched with the -url argument.
Note: to comply with the DDE spec we shouldn't be launching with the argument. We should let DDE send the WWW_OpenURL topic with the params to open the page. The problem with being compliant with the spec is that some apps will for all practical purposes hand DDE so I suggest we continue to use the argument to launch which is our current behavior.
3) We receive a WWW_OpenURL DDE message with the params from the ifExec key which would not include the url. We would check if the url is present and if it isn't not open a new window.

So, this is very similar to how we work today when the app is already running. In this case the only difference is when the app is launched from the OS we will receive a DDE message without an url and then ignore it.
This sounds great, thanks for the clarification. I'd love the patch to include this summary in comments ;-)
Blocks: 344481
Minus whitespace changes to make the review easier.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #239156 - Flags: review?(benjamin)
Comment on attachment 239156 [details] [diff] [review]
patch - whitespacee

A couple of notes to help with the review

>Index: browser/components/shell/src/nsWindowsShellService.cpp
...
>@@ -140,19 +140,19 @@
> // HKCU\SOFTWARE\Classes\<protocol>\
>-//   DefaultIcon                    (default)         REG_SZ  <appname>,1
>+//   DefaultIcon                    (default)         REG_SZ  <appname>,0
For Vista this matters since it displays the icon when typing it in start -> run. This is also consistent with what IE does for protocols.

>-//   shell\open\ddeexec             (default)         REG_SZ  "%1",,-1,0,,,,
>+//   shell\open\ddeexec             (default)         REG_SZ  "%1",,0,0,,,,
This comment was incorrect.
>@@ -188,20 +188,22 @@
...
> #define CLS_HTML "FirefoxHTML"
>-#define VAL_ICON "%APPPATH%,1"
>+#define VAL_URL_ICON "%APPPATH%,0"
>+#define VAL_FILE_ICON "%APPPATH%,1"
See note about icons above.

>+  // DDE settings
>+  { MAKE_KEY_NAME2(CLS, "HTTP", DDE), "", DDE_COMMAND, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "HTTP", DDE, "Application"), "", DDE_NAME, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "HTTP", DDE, "Topic"), "", "WWW_OpenURL", NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "HTTP", DDE, "ifexec"), "", DDE_IFEXEC, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME2(CLS, "HTTPS", DDE), "", DDE_COMMAND, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "HTTPS", DDE, "Application"), "", DDE_NAME, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "HTTPS", DDE, "Topic"), "", "WWW_OpenURL", NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "HTTPS", DDE, "ifexec"), "", DDE_IFEXEC, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME2(CLS, "FTP", DDE), "", DDE_COMMAND, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "FTP", DDE, "Application"), "", DDE_NAME, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "FTP", DDE, "Topic"), "", "WWW_OpenURL", NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME3(CLS, "FTP", DDE, "ifexec"), "", DDE_IFEXEC, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME2(CLS, "GOPHER", DDE), "", DDE_COMMAND, NO_SUBSTITUTION | NON_ESSENTIAL },
>+  { MAKE_KEY_NAME3(CLS, "GOPHER", DDE, "Application"), "", DDE_NAME, NO_SUBSTITUTION | NON_ESSENTIAL },
>+  { MAKE_KEY_NAME3(CLS, "GOPHER", DDE, "Topic"), "", "WWW_OpenURL", NO_SUBSTITUTION | NON_ESSENTIAL },
>+  { MAKE_KEY_NAME3(CLS, "GOPHER", DDE, "ifexec"), "", DDE_IFEXEC, NO_SUBSTITUTION },
>+  { MAKE_KEY_NAME2(CLS, "CHROME", DDE), "", DDE_COMMAND, NO_SUBSTITUTION | NON_ESSENTIAL },
>+  { MAKE_KEY_NAME3(CLS, "CHROME", DDE, "Application"), "", DDE_NAME, NO_SUBSTITUTION | NON_ESSENTIAL },
>+  { MAKE_KEY_NAME3(CLS, "CHROME", DDE, "Topic"), "", "WWW_OpenURL", NO_SUBSTITUTION | NON_ESSENTIAL },
>+  { MAKE_KEY_NAME3(CLS, "CHROME", DDE, "ifexec"), "", DDE_IFEXEC, NO_SUBSTITUTION },
This was removed in bug 246078 and this adds it back. :)

The remainder is mainly cleanup from the changes made in bug 246078.
Needed for Vista support since the ddexec hack breaks when registered as the default app using the methods provided with Vista.
Blocks: 336469
Comment on attachment 239156 [details] [diff] [review]
patch - whitespacee

If there are any browser-specific bits of DDE code left, could you file a followup bug about them? I think I got most of everything fixed up to use the generic nsICommandLine remoting code, but I'm getting hazy about whether there was anything left to do (I know there were issues with mac handling that need to be dealt with still).
Attachment #239156 - Flags: review?(benjamin) → review+
Blocks: 352420
you can probably remove the nsIObserver impl.

Also there is a typo in :

+ * and the new mthods are incompatible with removing the ddeexec registry key.


Typo fixed... the observer impl. should be handled in a separate bug.
okay.  see bug 353353 for the unused nsIObserver impl.
Checked in to trunk
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Also needed the patch in Bug 353398
Depends on: 353398
(In reply to comment #13)
> (From update of attachment 239156 [details] [diff] [review] [edit])
> If there are any browser-specific bits of DDE code left, could you file a
> followup bug about them? I think I got most of everything fixed up to use the
> generic nsICommandLine remoting code, but I'm getting hazy about whether there
> was anything left to do (I know there were issues with mac handling that need
> to be dealt with still).
Sure... we do have the WWW_OpenURL DDE Request handler in toolkit/xre/nsNativeAppSupportWin.cpp that is only used by the browser but nothing else I know of. Not sure if we need to do anything about this.
Depends on: 355650
Blocks: 369465
No longer blocks: 352420
note: though this bug was not Vista specific it always affected Vista so had to be fixed for 2.0.0.2
v.fixed with Firefox 2.0.0.2 rc2: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070208 Firefox/2.0.0.2, compared registry keys created/deleted by 2.0.0.1 on startup/shutdown with those created by Firefox 2.0.0.2 rc2, and the ddeexec keys are set in the right places and remain set on shutdown.

NOTE: I was not able to observe any odd behavior on XP after installing 2.0.0.1 as Admin user, setting to default browser and then trying to launch as Limited user.  So if anyone else can verify this with a use case, that'll be great... since I only looked at the registry settings.

I just know that things are working as users might expect with the latest 2.0.0.2 rc2 build. :-)
Keywords: fixed1.8.1.2 → verified1.8.1.2
Depends on: 370053
Your fix broke every single one of MY computers -- XP Pro (3 of them), XP x64, and Vista Home Premium. Starting with 2.0.0.2 release, every time I upgrade Firefox I have to go and delete the ddeexec entries. Everything works, auto-update happens, then "boom" I get the "windows error opening internet shortcut" problem again.
Scott, that should not happen and hasn't happened or been reported by anyone else. Are you doing anything unusual that you can think of besides deleting these keys that may cause this to happen on your systems?
Nothing besides installing firefox! Ever since I installed ff 2.0.0.2 (by automatic update), this has been happening. It was until recently that someone pointed be to a website that showed me how to FIX the problem (by deleting the registry keys). So as of yesterday, for example, everything worked fine. I verified that there was no problem, then did NOTHNG besides auto-updating to 2.0.0.4. *Immediately* thereafter, I verified the problem was back. Something in the process of downloading and installing the update restored the offending keys.
Those keys are needed for OS integration and the fix you mention was due to a bug present prior to the 1.0 release. I have no idea what could be different about your system... have you tried running Firefox in safe mode in case an extension is causing this?
Status: RESOLVED → VERIFIED
No, I had not thought of that. And your suspicion is correct. The behavior seems to be caused by the Google toolbar for Firefox (v3.0.20070217W).
Others are reporting the same problem to google (e.g. http://groups.google.com/group/FFToolbar-Group-Bugs/browse_thread/thread/072935cb2d9acabe/77cdc339cf169b6f#77cdc339cf169b6f)
but there is no feedback from google on the problem.
I have absolutely the same behavior as Scott, and it started about at the same time as his. 
Same issues here, also limited to the same timeframe.  Please consider re-opening this ticket.

I can remove the DDE options which seems to fix the issues, but once I restart Firefox, they are all set back again.  At the simplest, an option should be available somewhere to enable/disable hooking into DDE.
I agree completely with Mark. Until someone can convince google to fix their **** code, there needs to be a setting for Firefox to not restore these registry entries. For my (and others') purposes, Firefox works fine without DDE.
Regretfully it isn't as simple as just not adding these keys. IE adds dde keys to HKLM\Software\Classes\ http|https|ftp, etc. and when the user is not an administrator and sets Firefox as the default browser we have to add the dde keys under HKCU\Software\Classes\ to override IE's keys.

note: improvements to the DDE code is planned for Firefox 3 in Bug 392155
You need to log in before you can comment on or make changes to this bug.