Closed
Bug 353089
Opened 17 years ago
Closed 17 years ago
Fix / remove ddeexec hack
Categories
(Firefox :: Shell Integration, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
(Keywords: verified1.8.1.2)
Attachments
(3 files)
20.08 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
21.59 KB,
patch
|
Details | Diff | Splinter Review | |
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
![]() |
Assignee | |
Comment 1•17 years ago
|
||
btw: this applies to all versions of Windows and not just Vista
Comment 2•17 years ago
|
||
(see bug 246078 for some context)
![]() |
Assignee | |
Comment 3•17 years ago
|
||
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?
Comment 4•17 years ago
|
||
Do we need to support DDEExec at all? Can't we just have the OS launch "firefox.exe <URL>" and do the remoting ourself?
![]() |
Assignee | |
Comment 5•17 years ago
|
||
Some third party apps require dde.
Comment 6•17 years ago
|
||
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.
![]() |
Assignee | |
Comment 7•17 years ago
|
||
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.
Comment 8•17 years ago
|
||
This sounds great, thanks for the clarification. I'd love the patch to include this summary in comments ;-)
![]() |
Assignee | |
Comment 9•17 years ago
|
||
Minus whitespace changes to make the review easier.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #239156 -
Flags: review?(benjamin)
![]() |
Assignee | |
Comment 10•17 years ago
|
||
![]() |
Assignee | |
Comment 11•17 years ago
|
||
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.
![]() |
Assignee | |
Comment 12•17 years ago
|
||
Needed for Vista support since the ddexec hack breaks when registered as the default app using the methods provided with Vista.
Blocks: 336469
Comment 13•17 years ago
|
||
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+
Comment 14•17 years ago
|
||
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.
![]() |
Assignee | |
Comment 15•17 years ago
|
||
Typo fixed... the observer impl. should be handled in a separate bug.
Comment 16•17 years ago
|
||
okay. see bug 353353 for the unused nsIObserver impl.
![]() |
Assignee | |
Comment 17•17 years ago
|
||
Checked in to trunk
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 18•17 years ago
|
||
![]() |
Assignee | |
Comment 20•17 years ago
|
||
(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.
![]() |
Assignee | |
Updated•17 years ago
|
![]() |
Assignee | |
Comment 22•17 years ago
|
||
note: though this bug was not Vista specific it always affected Vista so had to be fixed for 2.0.0.2
Comment 23•17 years ago
|
||
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
Comment 24•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 25•16 years ago
|
||
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?
Comment 26•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 27•16 years ago
|
||
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
Comment 28•16 years ago
|
||
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).
Comment 29•16 years ago
|
||
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.
Comment 30•16 years ago
|
||
I have absolutely the same behavior as Scott, and it started about at the same time as his.
Comment 31•16 years ago
|
||
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.
Comment 32•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 33•16 years ago
|
||
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.
Description
•