Closed Bug 791019 Opened 8 years ago Closed 7 years ago

Default file type association is broken on Windows 8

Categories

(Firefox :: Shell Integration, defect, P1)

x86_64
Windows 8
defect

Tracking

()

VERIFIED FIXED
Firefox 18
Tracking Status
firefox15 --- wontfix
firefox16 + verified
firefox17 + verified
firefox18 + verified

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [win8] [completed-elm])

Attachments

(7 files, 12 obsolete files)

888 bytes, patch
Felipe
: review+
Details | Diff | Splinter Review
2.06 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
2.37 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
3.01 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
5.20 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
8.20 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
68.55 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
Before Windows 8 RTM, we had file type associations working as of bug 764515 which is on Beta right now.  As of Windows 8 RTM, prompts that we do for file type defaults no longer set us as the default for those file types.  So right now on Beta file type associations like .html are completely broken.  Windows 8 will already be shipped once we release Beta.

In Windows 8 pre RTM, protocol association was protected by a cryptographic hash but not file type association.  As of RTM both file type association and protocol association are protected by a cryptographic hash.  The only way to set the default browser when there is a cryptographic hash is by asking the OS to prompt the user.

you can set protocol association which sets the cyrptographic hash with a little flyout window that lists all the browsers and the user just selects one.  This is what we were doing as of bug 764515, and we we're setting the file type associations via helper.exe.  I should mention that you can also produce a flyout for file type association, but only for one at a time, and it doesn't include protocol defaults.

What IE10 does, for default browser is it shows that protocol association flyout on the startup check, but if you go to options and set the defaults that way, it launches control panel which can set both protocol + file type association at the same time.  (Control Panel\Programs\Default Programs\Set Default Programs)

I recommend that we do the same as IE10 here, and that we also uplift it to aurora + beta so that file type defaults are not broken in win8.
Attached patch Patch v1 - Advanced pref changes (obsolete) — Splinter Review
Attachment #660897 - Flags: review?(gavin.sharp)
Attached patch Patch v1 - Interface changes (obsolete) — Splinter Review
Attachment #660898 - Flags: review?(gavin.sharp)
Attachment #660899 - Flags: review?(jmathies)
Attachment #660900 - Flags: review?(jmathies)
Attached patch Patch v1 - Startup check changes (obsolete) — Splinter Review
Attachment #660901 - Flags: review?(gavin.sharp)
Attached patch Patch v1 - Update existing test (obsolete) — Splinter Review
Attachment #660902 - Flags: review?(gavin.sharp)
Keywords: dev-doc-needed
Comment on attachment 660898 [details] [diff] [review]
Patch v1 - Interface changes

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

::: browser/components/shell/public/nsIShellService.idl
@@ +24,5 @@
> +
> +  /**
> +   * Determines whether or not Firefox is the "Default Browser."
> +   * 
> +   * @param aForAllTypes  true if the check should be made for HTTP and HTML.

afaict from your 'other platforms' patch this only applies to windows. Probably want to mark it as such here and maybe move it to the second parameter.
Attachment #660899 - Flags: review?(jmathies) → review+
> afaict from your 'other platforms' patch this only applies to windows. Probably 
> want to mark it as such here and maybe move it to the second parameter.

Gavin, is documenting it enough for now given that these patches need to be uplifted to beta? I could file a followup for m-c landing only that is to remove the comment and actually make the implementations.
The comment would just say that aForAllTypes may be ignored for some platforms.
Comment on attachment 660900 [details] [diff] [review]
Patch v1 - Shell win32 widget code

This part looks ok to me with your other changes approved.
Attachment #660900 - Flags: review?(jmathies) → review+
Blocks: 790667
Blocks: 791501
Preemptively adding qawanted and verifyme, since this change (which we want in FF16) worries me.
Keywords: qawanted, verifyme
Thanks, sounds good.  I think it would be pretty bad to not have this and bug 790667 on Firefox Beta since it'll be the version people have when Windows 8 is live.  It would be good for QA to verify that defaults handling on other OS still work fine.  And of course that it works as expected in Windows 8 as well.
Comment on attachment 660897 [details] [diff] [review]
Patch v1 - Advanced pref changes

Found out Gavin is on PTO right now, he suggested Felipe for the review.

Felipe, we're trying to get this on mozilla-beta (v16) since Windows 8 will be officially released when people have v16 installed.
Attachment #660897 - Flags: review?(gavin.sharp) → review?(felipc)
Attachment #660898 - Flags: review?(gavin.sharp) → review?(felipc)
Attachment #660901 - Flags: review?(gavin.sharp) → review?(felipc)
Attachment #660902 - Flags: review?(gavin.sharp) → review?(felipc)
Comment on attachment 660897 [details] [diff] [review]
Patch v1 - Advanced pref changes

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

::: browser/components/preferences/advanced.js
@@ +733,5 @@
> +#ifdef XP_WIN
> +    if (this._interval && selectedIndex === 1) {
> +      clearInterval(this._interval);
> +    }
> +#endif

I don't know how this works on Windows 8, but when Firefox is selected as the default browser, does it mean that the control panel has then been closed? Or can they still make changes and so we should keep the interval running?
Comment on attachment 660901 [details] [diff] [review]
Patch v1 - Startup check changes

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

::: browser/components/nsBrowserGlue.js
@@ +469,5 @@
> +            // In Windows 8, the UI for selecting default protocol is much
> +            // nicer than the UI for setting file type associations. So we
> +            // only show the protocol association screen on Windows 8.
> +            shell.setDefaultBrowser(!isWin8OrHigher, false);
> +#else

This section would be clearer if there was only one call to setDefaultBrowser, and a variable called claimAllTypes inited to true which the #ifdef block can change.
Comment on attachment 660898 [details] [diff] [review]
Patch v1 - Interface changes

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

As discussed over IRC we'll just add a second optional parameter to the existing isDefaultBrowser function instead of defining a new one
Attachment #660898 - Flags: review?(felipc) → review-
(In reply to :Felipe Gomes from comment #15)
> Comment on attachment 660897 [details] [diff] [review]
> Patch v1 - Advanced pref changes
> 
> Review of attachment 660897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/advanced.js
> @@ +733,5 @@
> > +#ifdef XP_WIN
> > +    if (this._interval && selectedIndex === 1) {
> > +      clearInterval(this._interval);
> > +    }
> > +#endif
> 
> I don't know how this works on Windows 8, but when Firefox is selected as
> the default browser, does it mean that the control panel has then been
> closed? Or can they still make changes and so we should keep the interval
> running?

They could still make changes, I can keep it alive.
Attached patch Patch v2 - Advanced pref changes (obsolete) — Splinter Review
Attachment #660897 - Attachment is obsolete: true
Attachment #660897 - Flags: review?(felipc)
Attachment #662394 - Flags: review?(felipc)
Attachment #660902 - Attachment is obsolete: true
Attachment #660902 - Flags: review?(felipc)
Attachment #662395 - Flags: review?(felipc)
Attached patch Patch v2 - Startup check changes (obsolete) — Splinter Review
Attachment #660901 - Attachment is obsolete: true
Attachment #660901 - Flags: review?(felipc)
Attachment #662396 - Flags: review?(felipc)
Attachment #660898 - Attachment is obsolete: true
Attachment #662397 - Flags: review?(felipc)
Updated for interface change, carrying forward r+
Attachment #660900 - Attachment is obsolete: true
Attachment #662398 - Flags: review+
Updated for review comments. Carried forward r+.
Attachment #660899 - Attachment is obsolete: true
Attachment #662399 - Flags: review+
Comment on attachment 662394 [details] [diff] [review]
Patch v2 - Advanced pref changes

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

the _interval variable is no longer necessary
Attachment #662394 - Flags: review?(felipc) → review+
Attachment #662395 - Flags: review?(felipc) → review+
Attachment #662397 - Flags: review?(felipc) → review+
Comment on attachment 662396 [details] [diff] [review]
Patch v2 - Startup check changes

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

::: browser/components/nsBrowserGlue.js
@@ +470,5 @@
> +                              .getProperty("version");
> +              claimAllTypes = (parseFloat(version) < 6.2);
> +            } catch (ex) { }
> +#endif
> +            shell.setDefaultBrowser(false, claimAllTypes);

wrong param order. r+ with this fixed
Attachment #662396 - Flags: review?(felipc) → review+
Attached patch Patch v3 - Advanced pref changes (obsolete) — Splinter Review
Removed _interval vars. Carrying forward r+.

Thanks for the reviews :D
Attachment #662394 - Attachment is obsolete: true
Good catch! Carrying forward r+.
Attachment #662398 - Attachment is obsolete: true
Attachment #662407 - Flags: review+
Attachment #662401 - Flags: review-
Attachment #662401 - Flags: review+
Attachment #662401 - Flags: review-
Attachment #662403 - Flags: review+
Attachment #662399 - Attachment is obsolete: true
Attachment #662396 - Attachment is obsolete: true
Attachment #662399 - Attachment is obsolete: false
Removed the is default browser call from the set. It was causing problems since control panel is async now.  It was added before because the flyout was modal. Carrying forward r+.
Attachment #662407 - Attachment is obsolete: true
Attachment #662734 - Flags: review+
:%s/async/modeless
Tested more and found / fixed a couple bugs:
- After setDefaultBrowser call I set the default browser index based on a follow up call to isDefaultBrowser.  Instead of assuming it'll be default.
- Cc and Ci were not defined so replaced with Components.classes and Components.interfaces
Carrying forward r+.
Attachment #662401 - Attachment is obsolete: true
Attachment #663097 - Flags: review+
Just noticed I was returning NS_OK instead of rv for the set default browser call, carrying forward r+.
Attachment #662734 - Attachment is obsolete: true
Attachment #663099 - Flags: review+
Comment on attachment 662397 [details] [diff] [review]
Patch v2 - Interface changes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): - --- Microsoft releasing win8 while beta is out --- 
User impact if declined: Users will not be able to set filetype defaults
Testing completed (on m-c, etc.): I've tested with tbpl builds only
Risk to taking this patch (and alternatives if risky): Low for pre win8 OS. Medium for win8, but this should be offset with testing, and the alternative is being broken on win8
String or UUID changes made by this patch: none

Note: I'm only requesting on this patch, but the request should be seen as for all patches involved in this bug.
Attachment #662397 - Flags: approval-mozilla-beta?
Attachment #662397 - Flags: approval-mozilla-aurora?
We'll wait for this to be on trunk a little longer and, same as with the other bug, can you please either nominate all patches for approval or do a rollup patch.  Sorry to seem like a stickler but with this amount of patches, if there were any regressions, we need to be able to know what exactly was a non-obsolete patch at the time of approval, that was then given the go-ahead to land.  Thanks in advance for helping keep the release process consistent.
I'll do a rollup patch of the involved 4 bug's patches (bug 791019, bug 790667, bug 737833, bug 791687).  All of them are related and are all dependencies for this.
Blocks: 737833, 791687
No longer blocks: 791501
Also I'll update the tracking flags and post the changeset once it lands in all 4 bugs (if it gets approval).
Attachment #662397 - Flags: approval-mozilla-beta?
Attachment #662397 - Flags: approval-mozilla-aurora?
Carrying forward r+ from the other bugs.
Attachment #663621 - Flags: review+
Comment on attachment 663621 [details] [diff] [review]
Aurora and Beta rollup for bug 791019, bug 790667, bug 737833, bug 791687

[Approval Request Comment]
Bug caused by (feature/regressing bug #): - This is not introduced by a particular bug, but due to Microsoft releasing windows 8
User impact if declined: Users will not be able to set filetype defaults
Testing completed (on m-c, etc.): I've tested with local builds and tinderbox builds from m-c.
Risk to taking this patch (and alternatives if risky): Low as I tested everything myself, but QA should independently verify as well and I'll post a follow up comment with some test case ideas.
String or UUID changes made by this patch: none

This rollup patch includes the patches from: bug 791019, bug 790667, bug 737833, bug 791687.  They are dependencies for this bug to land.
Attachment #663621 - Flags: approval-mozilla-beta?
Attachment #663621 - Flags: approval-mozilla-aurora?
Test case ideas for QA-ing this bug:
1. On a Win8 RTM machine: set IE10 as default. Install Firefox and open the browser, on the startup default check it should show a flyout and prompt for default browser.  Firefox should now be your HTTP default but not your HTML default (verify independently both through control panel, via entering a url and double clicking an html file).
2. On a Win8 RTM machine: set IE10 as deafult. Install Firefox and open the browser, do not set the default on startup. Go to options and set the default browser. It should launch control panel's defaults screen. (Firefox will not be auto selected)
3. On a Win8 RTM machine: set IE10 as default. Install Firefox and open the browser, on the startup default check select Firefox as the default.  Then go to Firefox options, it should still say you aren't the default, because this screen sets all defaults.  Once you set all defaults the Firefox Options UI should update at that point and indicate you have the defaults.
4. On a Win8 RTM machine: When you set HTTP defaults only, it should not prompt you anymore on startup.
5. On a Win8 RTM machine: When you set all defaults, it should not prompt you anymore on startup.
6. On Win7 and Win8: Try to start Firefox via the pinned taskbar and make sure it works. (Do this test before and after setting Firefox as the default)
7. On a Win8 RTM machine: Do an update before having the defaults set. Make sure defaults don't get set and make sure taskbar icon still works.
8. Same as #7 but set as default before doing the nightly update. 
9. On Win XP, Win Vista, Win7, OSX, Linux: Make sure setting default browser on startup sets all defaults (not just HTTP). Should work in the same way it used to with an older build.
10. On Win XP, Win Vista, Win7, OSX, Linux: Make sure setting default browser in options sets all defaults (not just HTTP). Should work in the same way it used to with an older build.
11-N. Anything else you can think of

I'd also appreciate feedback on if we should use this screen instead for the Win8 default setting in Firefox options:
Control Panel\Programs\Default Programs\Set Default Programs\Set Program Associations
Instead of what we are using ( Control Panel\Programs\Default Programs\Set Default Programs )
You can get to the program association one by clicking on Firefox and then clicking on choose defaults for this program.
Since we're running late in the release cycle, this would be changed for v17 but not v16 in a different bug.
I think it would be a good idea to use the Firefox specific Set Default Programs screen instead.
Blocks: 791501
How many betas are left? When would this need to land by if given approval?
We have a FF16 beta 5 goto build tomorrow (land code by today or latest tomorrow morning), which is the last beta that we would prefer to take changes in which are really needed for the release. The final beta 6 (goto build Monday, Oct 1)would be very risky as that is the last beta in the cycle and we would like this code to be similar to the release code. 

We are gonna work with QA to get this tested at the earliest .
Any update on this? I was hoping to land this in for beta 5.
fwiw, I did some basic testing (in addition to last weeks testing on m-c) on win8 with mozilla-beta with the rolled up patch and everything is working correctly.
Comment on attachment 663621 [details] [diff] [review]
Aurora and Beta rollup for bug 791019, bug 790667, bug 737833, bug 791687

[Triage Comment]
Let's land on branches since we'd like to take this in Beta 5. If QA comes back with a major regression, we can back out as necessary.
Attachment #663621 - Flags: approval-mozilla-beta?
Attachment #663621 - Flags: approval-mozilla-beta+
Attachment #663621 - Flags: approval-mozilla-aurora?
Attachment #663621 - Flags: approval-mozilla-aurora+
I've completed the testing using the latest nightly and an older nightly with the fix so I could test the scenarios in 7 and 8 from comment #41. Overall it seems to work.

1. Good
Notes: Only http, https, www... are recognized as things that need to be opened in a browser.
2. Good
3. Good
4. Good?
* If you select No to the Firefox prompt but use the Control Panel to set http, then you get prompt next time you launch the browser.
5. Good
6. Good?
If you set it as default, Firefox is pinned in the taskbar automatically, and launching from the taskbar works.
* If you dismiss the prompt from Firefox to set as default, Firefox does not get pinned. So you have to pin it while it is running.
7. Good
8. Good
9. Good
10. Good
11. Uninstalling resets defaults (back to IE)

Things to note: 

- On Windows 7 all default program options for the nightly get set when you select Yes on the Firefox prompt, and only three of the eight get set when you do the same in Windows 8.

- On Windows 8 when you launch Firefox for the first time and select Yes to make it your default, next time you open it you don't get prompted, but if you go to the Options/Advanced you'll see the button "Make Nightly My Default Browser" and that will only go away after you select all the default options in the Control Panel.
Keywords: qawanted
That sounds good overall, but here are some more specific questions.

> 4. Good?
> * If you select No to the Firefox prompt but use the Control Panel to set http, 
> then you get prompt next time you launch the browser.

When you set the default through Firefox options which launches Control Panel? or Control panel directly?

> 6. Good?
> If you set it as default, Firefox is pinned in the taskbar automatically, and
>  launching from the taskbar works.
> * If you dismiss the prompt from Firefox to set as default, Firefox does not 
> get pinned. So you have to pin it while it is running.

Firefox should actually be pinned by the installer itself.  Could you verify with a firefox-beta build before my patch and after to see if it's the same? Both should use a fresh copy of Windows 8 for the test.  For that reason using a VM would probably be best so you can reset state back to a fresh installation instantly.  This may be an issue but I think it is unrelated to these patches.

> - On Windows 7 all default program options for the nightly get set when you
> select Yes on the Firefox prompt, and only three of the eight get set when you 
> do the same in Windows 8.

That is expected and correct.  The flyout only sets the default protocol handlers (http and https) as well as the unprotected file extensions, in our case that's only shtml.


> - On Windows 8 when you launch Firefox for the first time and select Yes to 
> make it your default, next time you open it you don't get prompted, but if you
>  go to the Options/Advanced you'll see the button "Make Nightly My Default 
> Browser" and that will only go away after you select all the default options 
> in the Control Panel.

That is expected and correct.  That flyout should only set HTTP/HTTPS defaults. Control panel will show you need to set them until both protocol and HTML are set as default.
(In reply to Brian R. Bondy [:bbondy] from comment #49)
> That sounds good overall, but here are some more specific questions.
> 
> > 4. Good?
> > * If you select No to the Firefox prompt but use the Control Panel to set http, 
> > then you get prompt next time you launch the browser.
> 
> When you set the default through Firefox options which launches Control
> Panel? or Control panel directly?
> 

After selecting No in the Firefox prompt, I go to the Control Panel directly by pressing the Start key and typing "default programs."

> > 6. Good?
> > If you set it as default, Firefox is pinned in the taskbar automatically, and
> >  launching from the taskbar works.
> > * If you dismiss the prompt from Firefox to set as default, Firefox does not 
> > get pinned. So you have to pin it while it is running.
> 
> Firefox should actually be pinned by the installer itself.  Could you verify
> with a firefox-beta build before my patch and after to see if it's the same?
> Both should use a fresh copy of Windows 8 for the test.  For that reason
> using a VM would probably be best so you can reset state back to a fresh
> installation instantly.  This may be an issue but I think it is unrelated to
> these patches.
> 

Using Fx16.0b4 I observe the same behavior: If I opt to not set Firefox as my default browser, the application is not pinned when I close it. Only if I opt Yes does the app get pinned automatically.

I've been using a clean VM for testing.
(In reply to juan becerra [:juanb] from comment #50)
> (In reply to Brian R. Bondy [:bbondy] from comment #49)
> > That sounds good overall, but here are some more specific questions.
> > 
> > > 4. Good?
> > > * If you select No to the Firefox prompt but use the Control Panel to set http, 
> > > then you get prompt next time you launch the browser.
> > 
> > When you set the default through Firefox options which launches Control
> > Panel? or Control panel directly?
> > 
> 
> After selecting No in the Firefox prompt, I go to the Control Panel directly
> by pressing the Start key and typing "default programs."
> 

Please post a new bug for this, I don't think this is a show stopper and we can get it in for v17 if fixable.



> > > 6. Good?
> > > If you set it as default, Firefox is pinned in the taskbar automatically, and
> > >  launching from the taskbar works.
> > > * If you dismiss the prompt from Firefox to set as default, Firefox does not 
> > > get pinned. So you have to pin it while it is running.
> > 
> > Firefox should actually be pinned by the installer itself.  Could you verify
> > with a firefox-beta build before my patch and after to see if it's the same?
> > Both should use a fresh copy of Windows 8 for the test.  For that reason
> > using a VM would probably be best so you can reset state back to a fresh
> > installation instantly.  This may be an issue but I think it is unrelated to
> > these patches.
> > 
> 
> Using Fx16.0b4 I observe the same behavior: If I opt to not set Firefox as
> my default browser, the application is not pinned when I close it. Only if I
> opt Yes does the app get pinned automatically.
> 

Please post a new issue for this since it isn't related to these patches.


> I've been using a clean VM for testing.
OK great!
Thanks for all the testing!
Were you able to verify defaults handling still works the same as before on OSX and Linux? This is something I haven't had time to test myself.
I spent a little bit of time on the latest Mac OS X and Ubuntu testing default handling, as instructed in 9 and 10 in comment #48. I tried to make sure I covered basic user scenarios similar to Win 7 and Win 8. For example, I created text and spreadsheet documents with hyperlinks that opened the default browser when clicked. In Ubuntu with Unity you can type "maps.google.com" and that will bring up the default browser or open a new tab if already opened. I also tried double clicking on html documents which opened the default browser. Clicking on links from other programs also opened the default browser (clicking on "contact support" links for several apps in iTunes).

I did not go in-depth, but I tried to cover the most common cases.
great, thanks!  Please CC me on those 2 new bugs whenever you get a chance to post them.
bbondy, when reviewing this patches landing on beta, to be sure if SeaMonkey needed changes, I noticed a bug:

http://hg.mozilla.org/releases/mozilla-beta/rev/4130a141a36b#l1.48

Note the order of params, compare that with http://hg.mozilla.org/releases/mozilla-beta/rev/4130a141a36b#l4.33 and you should see the issue right away.
(In reply to Justin Wood (:Callek) from comment #56)
> bbondy, when reviewing this patches landing on beta, to be sure if SeaMonkey
> needed changes, I noticed a bug:

err ignore me set!==get
ya the "is default" function has different ordering than the "set default" because the "set deafult" already existed with it as first param, and we wanted to add the new optional param for the "is default".
I think at some point in my patch I confused to 2 as well and felipe caught it, so no worries :)
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0

Verified the fix on the above build using the guidance from comment #41 and it works ok (except the issues that Juan mentioned). Also, I verified that setting the default browser works as expected on other OSes (Mac 10.7.4, Win 7, Win XP).
QA Contact: mihaela.velimiroviciu
By the way I haven't seen any follow up bugs posted, were they posted?  If so could you CC me?  See Comment 51 for details on the 2 bugs.
Depends on: 798166
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0

Verified the Win 8 fix on latest Firefox beta version (17b5): setting Firefox as default works as expected. Bug #796038 and bug #796044 which were logged based on comment #51 are also fixed and verified.
Setting default browser works fine on other OSes, as well (Mac 10.7.5, Win 7).
Couldn't reproduce on Windows 8 x64, FF 18b5. It looks like it works as expected.
Verified Fixed.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.