Last Comment Bug 354005 - Setting the app as the OS default is broken on Vista
: Setting the app as the OS default is broken on Vista
Status: RESOLVED FIXED
[vista]
: verified1.8.1.2
Product: Firefox
Classification: Client Software
Component: Shell Integration (show other bugs)
: 2.0 Branch
: x86 Windows Vista
: -- normal with 1 vote (vote)
: ---
Assigned To: Robert Strong [:rstrong] (use needinfo to contact me)
:
:
Mentors:
Depends on: 372236 380496
Blocks: 336469 353944 354897 355650 355661 356241 357626 358116 367165 369465 396063
  Show dependency treegraph
 
Reported: 2006-09-23 20:00 PDT by Robert Strong [:rstrong] (use needinfo to contact me)
Modified: 2012-03-14 03:39 PDT (History)
13 users (show)
mbeltzner: blocking1.8.1.1-
dveditz: blocking1.8.1.2+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Installer portion - mostly done (32.50 KB, patch)
2006-10-09 00:23 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch -w (OS Integration) (45.80 KB, patch)
2007-01-11 00:28 PST, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch (Installer) (48.31 KB, patch)
2007-01-11 00:31 PST, Robert Strong [:rstrong] (use needinfo to contact me)
moco: review+
Details | Diff | Splinter Review
v.2 patch -w (OS Integration) (44.96 KB, patch)
2007-01-12 10:53 PST, Robert Strong [:rstrong] (use needinfo to contact me)
doug.turner: review+
benjamin: superreview+
Details | Diff | Splinter Review
Everything for Seth (150.62 KB, patch)
2007-01-26 08:33 PST, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
branch patch (173.53 KB, patch)
2007-02-05 21:30 PST, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
Branch patch with everything plus the kitchen sink (170.19 KB, patch)
2007-02-05 23:04 PST, Robert Strong [:rstrong] (use needinfo to contact me)
moco: review+
Details | Diff | Splinter Review
followup patch for branch (missed on initial checkin) (6.32 KB, patch)
2007-02-08 01:36 PST, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review

Description Robert Strong [:rstrong] (use needinfo to contact me) 2006-09-23 20:00:02 PDT
I modified nsWindowsShellService.cpp so it supports setting the app as the default and all registry values are properly set but the OS displays a dialog stating that the app is hung / must be closed. I suspect we will need to use a separate exe (NSIS?) as IE does to provide this.
Comment 1 Robert Strong [:rstrong] (use needinfo to contact me) 2006-09-24 00:02:56 PDT
Other reasons to use a separate exe to do this...
when the profile manager is set to always display on startup the profile manager is displayed.
when the app is upgraded but hasn't been started yet the extension compatibility wizard is displayed.
with UAC turned on the process that is launched is given an elevated token. If the process is already running I believe this will fail. If by chance it doesn't fail the app is granted an elevated token which may be a security risk.
Comment 2 Robert Strong [:rstrong] (use needinfo to contact me) 2006-09-25 02:56:58 PDT
Taking... I have this partially completed.
Comment 3 Robert Strong [:rstrong] (use needinfo to contact me) 2006-09-25 17:03:34 PDT
Note: trying to set the app as the default for the OS is broken when using UAC and the app is already running presumably. I recall seeing an msdn article where I believe it stated that the exe's token would not be modified if the exe is already running. If I run across the article again I'll post a link in this bug unless of course someone else beats me to it.
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2006-09-25 17:04:24 PDT
s/presumably//
Comment 5 Robert Strong [:rstrong] (use needinfo to contact me) 2006-10-09 00:23:20 PDT
Created attachment 241681 [details] [diff] [review]
Installer portion - mostly done
Comment 6 Brian Polidoro 2006-11-06 06:50:35 PST
*** Bug 359643 has been marked as a duplicate of this bug. ***
Comment 7 Daniel Veditz [:dveditz] 2006-11-20 16:13:05 PST
Blocking for now, but incomplete Vista bugs might have to wait for 1.8.1.2
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2006-11-23 14:20:14 PST
No longer blocking 1.8.1.1 as per recent discussion with drivers around Vista readiness requirements.
Comment 9 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-11 00:28:34 PST
Created attachment 251156 [details] [diff] [review]
patch -w (OS Integration)

Doug, this patch only contains the OS Integration changes. I'm going to ask bsmedberg to review it if it passes the muster.
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-11 00:31:32 PST
Created attachment 251157 [details] [diff] [review]
patch (Installer)
Comment 11 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-11 00:34:06 PST
Comment on attachment 251157 [details] [diff] [review]
patch (Installer)

fyi: the string changes aren't part of the branch patch.
Comment 12 Doug Turner (:dougt) 2007-01-11 18:53:09 PST
Comment on attachment 251156 [details] [diff] [review]
patch -w (OS Integration)

-w, i am sure that the spacing is better that it was and I am happy that more of this blames to you :-)

what happened to the XPI key that was set in gSettings?

in CheckArgShell(), the comment says that you need to use lowercase, but that isn't the case.'

In nsAppRunner.cpp, i think you should move the two XP_WIN blocks into a single function.  

Also, i am somewhat nervous about the guesstimate test.  Maybe this is something we can put in to a pref (not sure if all.js is available at this point or not).  This would allow us to tweak this value in the field if need-be.

I need to give CheckArgShell() a bit more of a look over tomorrow morning (or tonight).

Why are you changing the mMutexName to include MOZ_MUTEX_NAMESPACE.  Is this for vista or something?

Just to be sure -- the argument passed to a xul app named "-requestPending" will only be used "internally" to restart the application.  If this is the case, do you want to name it something more obscure?
Comment 13 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-11 19:01:43 PST
(In reply to comment #12)
> (From update of attachment 251156 [details] [diff] [review])
> ...
> what happened to the XPI key that was set in gSettings?
It wasn't used... it is my understanding it was something that was planned but never implemented.

> in CheckArgShell(), the comment says that you need to use lowercase, but that
> isn't the case.'
will fix
 
> In nsAppRunner.cpp, i think you should move the two XP_WIN blocks into a single
> function.  
agreed
 
> Also, i am somewhat nervous about the guesstimate test.  Maybe this is
> something we can put in to a pref (not sure if all.js is available at this
> point or not).  This would allow us to tweak this value in the field if
> need-be.
I was thinking of just bumping it up to around 100 instead of a pref. This code path will seldom be hit and the small amount of additional time should be fine when it is hit.

> I need to give CheckArgShell() a bit more of a look over tomorrow morning (or
> tonight).
cool and thanks

> Why are you changing the mMutexName to include MOZ_MUTEX_NAMESPACE.  Is this
> for vista or something?
I almost left it out of the patch... it adds the namespace which will fallback to local when it isn't present... it is just good coding habit.

> Just to be sure -- the argument passed to a xul app named "-requestPending"
> will only be used "internally" to restart the application.  If this is the
> case, do you want to name it something more obscure?
No, it is to tell the command line handler to not launch the url since a dde request will follow with the url.
Comment 14 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-12 02:12:25 PST
(In reply to comment #12)
...
> in CheckArgShell(), the comment says that you need to use lowercase, but that
> isn't the case.'
fyi: I just verified that it must be lowercase
Comment 15 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-12 10:53:32 PST
Created attachment 251297 [details] [diff] [review]
v.2 patch -w (OS Integration)

addresses dougt's comments.
Comment 16 Benjamin Smedberg [:bsmedberg] 2007-01-17 12:13:27 PST
Comment on attachment 251297 [details] [diff] [review]
v.2 patch -w (OS Integration)

It would be really great to have a testsuite for this code... it's probably not especially susceptible to unit-testing, but an uber-complete Litmus testsuite.
Comment 17 Marcia Knous [:marcia - use ni] 2007-01-17 13:36:47 PST
bsmedberg: my plan is to have a Vista specific testsuite at the top level of Litmus, and this functionality will be included in my testing.

(In reply to comment #16)
> (From update of attachment 251297 [details] [diff] [review])
> It would be really great to have a testsuite for this code... it's probably not
> especially susceptible to unit-testing, but an uber-complete Litmus testsuite.
> 

Comment 18 Doug Turner (:dougt) 2007-01-17 16:26:47 PST
Comment on attachment 251297 [details] [diff] [review]
v.2 patch -w (OS Integration)

This is really good stuff. I mostly have nits, which I will not bother with.

We need lots of edge case testing on the trunk. Rob, can you make sure that you give the community a heads-up.

r=dougt
Comment 19 (not reading, please use seth@sspitzer.org instead) 2007-01-18 22:45:46 PST
Comment on attachment 251157 [details] [diff] [review]
patch (Installer)

r=sspitzer

we really need to get robert a pony for this one.

rstrong sat down and explained this patch to me last week, and going over it with fresh eyes, I don't see any new issues.

some issues he raised while explaining the patch to me:

1)  should this code be disabled for thunderbird?

+      WriteRegStr SHCTX "$R5\shell\open\ddeexec" "" "$\"%1$\",,0,0,,,,"
+      WriteRegStr SHCTX "$R5\shell\open\ddeexec" "NoActivateHandler" ""
+      WriteRegStr SHCTX "$R5\shell\open\ddeexec\Application" "" "${DDEApplication}"
+      WriteRegStr SHCTX "$R5\shell\open\ddeexec\Topic" "" "WWW_OpenURL"

2)  do we have a spin off bug about the issue of "when firefox is launched from the installer (which has elevated privileges), does firefox have elevated privileges?"

3) Q:  for the software update scenario, do we have a plan to update the registry to match what the installer does?  (Yes, see bug #367165)

4)  finally, there was one issue that robert raised about what happens with side-by-side installs and the registry on vista.  I believe the decision was "the last install wins", but I forgot what this was in reference too.  Robert, do you remember?  (if so, can you log a spin off bug on that, even if we don't plan on addressing it?)
Comment 20 (not reading, please use seth@sspitzer.org instead) 2007-01-18 23:05:24 PST
5)  do we have a spin off bug about the issue of "when firefox is launched from updater.exe (which has elevated privileges), does firefox have elevated
privileges?"

as with the other spin off bug about elevated privs in comment #19, I believe the answer is yes, so i'll go look for those bugs.
Comment 21 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-19 14:18:54 PST
bug 367540 for the installer launching the app with elevated privs
bug 364483 for the updater launching the app with elevated privs
Comment 22 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-26 08:33:13 PST
Created attachment 252920 [details] [diff] [review]
Everything for Seth

Seth, the combination patch you asked for
Comment 23 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-30 01:13:24 PST
Checked in to trunk
Comment 24 -fullmetaljacket- 2007-01-30 02:38:24 PST
i can still reproduce the problem i posted on https://bugzilla.mozilla.org/show_bug.cgi?id=359643 (which had been marked dup of this bug)

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a2pre) Gecko/20070130 Minefield/3.0a2pre ID:2007013001 [cairo]

Comment 25 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-30 02:59:26 PST
This bug is to handle setting as the OS default (e.g. clicking a link in an email program, etc.) and I haven't looked at that bug. I'll take a look at bug 359643 as time permits and reopen that bug if I can reproduce, etc. It doesn't appear to be directly related to this though... also, some programs actually directly call IE on Windows which may be what you are seeing.
Comment 26 Carsten Book [:Tomcat] 2007-01-30 15:17:00 PST
I can verify fixed- this with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a2pre) Gecko/2007013004 Minefield/3.0a2pre, its working fine with links as example from thunderbird and also with the Links inside the Avast! Program (Bug 359643 comment #2)
Comment 27 Marcia Knous [:marcia - use ni] 2007-01-30 18:12:56 PST
Please see http://wiki.mozilla.org/Firefox:1.5.0.10-2.0.0.2:Test_Plan:Vista_Focused_Testing for results of my Vista testing. I tested with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a2pre) Gecko/2007013004 Minefield/3.0a2pre. I had the most success in the scenario with QA machine 4, running with a fresh profile and a new install. Where things got mucky in my testing was the machines that had existing profiles/possible cruft lying around.

Rob- is it expected that a user would have to restart the first time after setting the app as default?
Comment 28 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-30 18:20:25 PST
(In reply to comment #27)
> Please see
> http://wiki.mozilla.org/Firefox:1.5.0.10-2.0.0.2:Test_Plan:Vista_Focused_Testing
> for results of my Vista testing. I tested with Mozilla/5.0 (Windows; U; Windows
> NT 6.0; en-US; rv:1.9a2pre) Gecko/2007013004 Minefield/3.0a2pre. I had the most
> success in the scenario with QA machine 4, running with a fresh profile and a
> new install. Where things got mucky in my testing was the machines that had
> existing profiles/possible cruft lying around.
There are no profile issues that I can think of but there are side by side issues and possibly upgrade issues... that is much more likely what is going on since you would have either had a side by side or an upgrade with an existing profile present.

> Rob- is it expected that a user would have to restart the first time after
> setting the app as default?
How did you launch the app... from software update, from the installer, or from the shortcut?
Comment 29 Marcia Knous [:marcia - use ni] 2007-01-30 21:35:56 PST
App was launched from the installer, at which point I set it as the default browser. I did not test this scenario from software update or from the shortcut, but I can do that tomorrow when I am beside my Vista machines x4.

(In reply to comment #28)
> (In reply to comment #27)
> > Please see
> > http://wiki.mozilla.org/Firefox:1.5.0.10-2.0.0.2:Test_Plan:Vista_Focused_Testing
> > for results of my Vista testing. I tested with Mozilla/5.0 (Windows; U; Windows
> > NT 6.0; en-US; rv:1.9a2pre) Gecko/2007013004 Minefield/3.0a2pre. I had the most
> > success in the scenario with QA machine 4, running with a fresh profile and a
> > new install. Where things got mucky in my testing was the machines that had
> > existing profiles/possible cruft lying around.
> There are no profile issues that I can think of but there are side by side
> issues and possibly upgrade issues... that is much more likely what is going on
> since you would have either had a side by side or an upgrade with an existing
> profile present.
> 
> > Rob- is it expected that a user would have to restart the first time after
> > setting the app as default?
> How did you launch the app... from software update, from the installer, or from
> the shortcut?
> 

Comment 30 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-30 21:43:22 PST
(In reply to comment #29)
> App was launched from the installer, at which point I set it as the default
> browser. I did not test this scenario from software update or from the
> shortcut, but I can do that tomorrow when I am beside my Vista machines x4.
I believe that software update has the can't open link from other apps on launch after update bug that we saw this morning. Please first verify whether that is reproduceable and then check out if what you wrote in this bug is. Also try setting defaults on first run after software update and try clicking links from other apps on launch from the installer. I suspect both of these are due to being elevated on launch from both software update and the installer. Also, be sure to update from a build before the vista patches landed.
Comment 31 -fullmetaljacket- 2007-01-30 22:39:24 PST
(In reply to comment #25)
>some programs actually directly call IE on Windows which may be what you are >seeing.

i tried testing it on windows xp.

links on yahoo messenger launches the default browser, either ie or firefox (whichever the default browser i set). thus, yahoo messenger does not directly calling ie.

same goes to free download manager (i haven't tried it yet with avats!)




Comment 32 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-30 22:50:07 PST
Please take your comments over to bug 359643... as stated in comment #25 this is about OS Integration where you can click a http link in an email app, etc. and it opens the default browser. If you can do that which is inferred by your other comments and Yahoo IM is still not opening the Firefox when it is set as default then you are experiencing a different bug.
Comment 33 Mike Pesce (:By-Tor) 2007-01-31 11:14:46 PST
Since this check-in was made, clicking on a link from an external app with XP opens Minefield with my default homepage and an about:blank page.  The desired link is not opened.  If Minefield is already running, it will work properly however.
Comment 34 Marcia Knous [:marcia - use ni] 2007-01-31 16:00:34 PST
Mike: I don't see this using XP. I am using Thunderbird as the external app - with Minefield closed links open fine. Which external app(s) were you using? If it turns out others can reproduce this, will morph to a new bug so we don't clutter this one with a lot of noise.

(In reply to comment #33)
> Since this check-in was made, clicking on a link from an external app with XP
> opens Minefield with my default homepage and an about:blank page.  The desired
> link is not opened.  If Minefield is already running, it will work properly
> however.
> 

Comment 35 Robert Strong [:rstrong] (use needinfo to contact me) 2007-01-31 16:02:09 PST
Try verifying that the app is set as default in tools -> options -> main tab -> check now button
Comment 36 Carsten Book [:Tomcat] 2007-01-31 16:10:25 PST
(In reply to comment #34)
> Mike: I don't see this using XP. I am using Thunderbird as the external app -
> with Minefield closed links open fine. 

also wfm with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a2pre) Gecko/20070118 Minefield/3.0a2pre ID:2007011804 [cairo]
tried with the following apps:
Skype
MSN Messenger
ICQ
Thunderbird
Avast

Comment 37 Marcia Knous [:marcia - use ni] 2007-01-31 16:14:01 PST
Confirming. In the instance I note in Comment 34, Minefield is indeed set as default when you investigate via "Check Now."
(In reply to comment #35)
> Try verifying that the app is set as default in tools -> options -> main tab ->
> check now button
> 

Comment 38 Mike Pesce (:By-Tor) 2007-02-01 12:45:46 PST
(In reply to comment #34)
> Mike: I don't see this using XP. I am using Thunderbird as the external app -
> with Minefield closed links open fine. Which external app(s) were you using? If
> it turns out others can reproduce this, will morph to a new bug so we don't
> clutter this one with a lot of noise.

It's every app that can launch a link (xchat, The Bat!, RSSBandit are just a few.)
The has only happened since this patch landed, so I can't think of anything else that would cause this.  What I have found since originally posting about this is, if I disable an extension - any extension - then restart Minefield, the first link launched will work.  But, subsequently, everything else will just launch my homepage and about:blank.  This may indeed be a different bug.  I'm sorry if I'm causing undue clutter.  Just fyi, this problem persists in: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070201 Minefield/3.0a2pre ID:2007020108 [cairo]
Comment 39 Robert Strong [:rstrong] (use needinfo to contact me) 2007-02-05 21:30:26 PST
Created attachment 254120 [details] [diff] [review]
branch patch
Comment 40 Robert Strong [:rstrong] (use needinfo to contact me) 2007-02-05 23:04:58 PST
Created attachment 254128 [details] [diff] [review]
Branch patch with everything plus the kitchen sink
Comment 41 (not reading, please use seth@sspitzer.org instead) 2007-02-05 23:09:53 PST
Comment on attachment 254128 [details] [diff] [review]
Branch patch with everything plus the kitchen sink

r=sspitzer
Comment 42 (not reading, please use seth@sspitzer.org instead) 2007-02-05 23:17:19 PST
fix landed on branch.
Comment 43 Robert Strong [:rstrong] (use needinfo to contact me) 2007-02-08 01:36:00 PST
Created attachment 254403 [details] [diff] [review]
followup patch for branch (missed on initial checkin)

this was missed on initial branch checkin and already has approvals
Comment 44 Carsten Book [:Tomcat] 2007-02-09 10:00:48 PST
verified fixed for 1.8.1.2 using Build identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/2007020823 Firefox/2.0.0.2

I`ve set 1.8.1.2 as default browser and links from other programs and start search and external apps are handled by firefox
Comment 45 Robert Strong [:rstrong] (use needinfo to contact me) 2007-05-14 11:33:12 PDT
note: I highly suspect that bug 380496 will have to be fixed on Oracle's end per bug 380496 comment #2
Comment 46 Jens Martin Schlatter 2012-03-14 03:39:46 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.