969831 - Switching from desktop to metro fails if the device doesn't support directx feature level 9.3 or higher

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect, P1)

Description

[David S]

Attachment: DxDiag.txt

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140205162153

Steps to reproduce:

I am running Firefox on a 32-bit Intel Atom N570-based machine (Dell Inspiron Duo-dxdiag is attached), and when I select "Relaunch in Firefox for Windows 8 Touch", the splash screen appears but then I get kicked back to the Start Screen.


Actual results:

I get returned to the Windows 8 Start Screen (the app crashes without ever displaying the Top Sites,Bookmarks, etc. screen). Firefox in Metro Mode still appears in the list of running apps when I swipe in from the left, but if I select it, the same thing occurs again: splash screen, then crash.


Expected results:

I'd hoped to use the browser in Metro mode.

Comment 1

[David S]

The graphics driver installed is only compatible with WDDM 1.0, is that the source of the problem?
Reference: http://msdn.microsoft.com/en-us/library/windows/apps/dn344644.aspx

Comment 2

[Jim Mathies [:jimm]]

If you open up regedit you can check - 

1) run regedit.exe
2) browser to the HKEY_CURRENT_USER\Software\Mozilla\Firefox folder
3) check for a value key of 'MetroD3DAvailable'

If that's present, what's the value?

Comment 3

[David S]

Hi Jim,

That key is not present, the only key in HKEY_CURRENT_USER\Software\Mozilla\Firefox is MetroLastAHE. Value of that one is 0.

Comment 4

[Jim Mathies [:jimm]]

Ok that's interesting. My first suggestion would be to make sure the beta build is set as the default browser. 

https://wiki.mozilla.org/Firefox/Windows_8_Integration#Switching_Default_Browsers

If this doesn't help, maybe you can help us by generating a little debug output from the firefox launcher - 

https://wiki.mozilla.org/Firefox/Windows_8_Integration#Diagnosing_Command_Execute_Handler_Issues

and post the logs here from DebugView. This will tell us a bit more about what's going on when you try to launch firefox.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #4)
> If this doesn't help, maybe you can help us by generating a little debug
> output from the firefox launcher - 
> 
> https://wiki.mozilla.org/Firefox/
> Windows_8_Integration#Diagnosing_Command_Execute_Handler_Issues
> 
> and post the logs here from DebugView. This will tell us a bit more about
> what's going on when you try to launch firefox.

I just remembered that this debugging feature isn't in beta, it's only made it up to aurora. If you want to attempt generating this output, you'll need to install a different build - Nightly or Aurora. Or you can wait a a week, this code should get uplifted to beta later this week.

Comment 6

[David S]

Oops, I realized I was working too fast and checked on the wrong computer. Sorry about that! I checked on the computer that's actually experiencing the problem and the key MetroD3DAvailable key is present, value is 0.

Comment 7

[Jim Mathies [:jimm]]

(In reply to David S from comment #6)
> Oops, I realized I was working too fast and checked on the wrong computer.
> Sorry about that! I checked on the computer that's actually experiencing the
> problem and the key MetroD3DAvailable key is present, value is 0.

Ok, then yes, your hardware doesn't support what's needed to run metrofx. We do a series of checks and set that key to zero once they all fail - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CEHHelper.cpp#141

The one thing I don't understand though is tapping the tile should have opened the desktop browser rather than leaving you at the start screen. We'll have to check to make sure that's still working.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #7)
> The one thing I don't understand though is tapping the tile should have
> opened the desktop browser rather than leaving you at the start screen.
> We'll have to check to make sure that's still working.

This isn't working as expected in certain cases. Plus, we always show the "switch to metro" option in desktop even if the hardware cant handle running metro. I think we should try to get this cleaned up before we roll out to release.

Comment 10

[Jim Mathies [:jimm]]

Attachment: part 1 - don't switch to metro if hardware can't support it

Comment 11

[Jim Mathies [:jimm]]

Hey marina, is there a way to prevent the display of the win8 metro switch menu option / australis widget from showing based on a check of a registry key or call down to metro utils? I was messing around with removing this code in my local build, but the option keeps showing up - 

http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableWidgets.jsm#827

Comment 12

[Marina Samuel [:emtwo]]

Hey Jim, I think there should be no problem with making a call to metro utils here alongside the "hasWindowsTouchInterface" check. What code were you removing? I quickly tested changing the code there to say "!Services.sysinfo.getProperty("hasWindowsTouchInterface")" and then ran "mach build browser/components" and that made the menu button not show up for me.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Marina Samuel [:emtwo] from comment #12)
> Hey Jim, I think there should be no problem with making a call to metro
> utils here alongside the "hasWindowsTouchInterface" check. What code were
> you removing? I quickly tested changing the code there to say
> "!Services.sysinfo.getProperty("hasWindowsTouchInterface")" and then ran
> "mach build browser/components" and that made the menu button not show up
> for me.

That did work, thanks. I think I built in browser/metro by accident when I was testing.

Comment 14

[Jim Mathies [:jimm]]

Attachment: part 1 - add a supported attribute to nsIWinMetroUtils

Comment 15

[Jim Mathies [:jimm]]

Attachment: part 2 - modify australis front end check to use nsIWinMetroUtils

Comment 16

[Jim Mathies [:jimm]]

Attachment: part 2 for beta

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 8377638 [details] [diff] [review]
part 1 - add a supported attribute to nsIWinMetroUtils

I think this should suffice. Thought about copying the d3d check code down to here but I don't think it's needed. Between these two checks everyone should be covered.

Cases where I think 'supported' might incorrectly return false -

1) first launch prior to creating a window where the user launched firefox.exe via the command line (automated tests would trigger this for the first browser window). Shouldn't be a problem.
2) cases where d2d is disabled, and this browser isn't the default such that the ceh isn't in use.
3) ??

can't really think of anything else.

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 8377638 [details] [diff] [review]
part 1 - add a supported attribute to nsIWinMetroUtils

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

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +786,5 @@
>      SetRequestMet();
>      return E_FAIL;
>    }
>  
> +  if (!IsDX10Available() && mRequestType == METRO_RESTART) {

Do you think this should just be  if (!IsDX10Available()) ?

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +367,5 @@
> +                      NS_LITERAL_STRING("Software\\Mozilla\\Firefox"),
> +                      nsIWindowsRegKey::ACCESS_READ);
> +    if (NS_SUCCEEDED(rv)) {
> +      uint32_t value = 0;
> +      rv = regKey->ReadIntValue(NS_LITERAL_STRING("MetroD3DAvailable"), &value);

I'm a bit concerned that if this isn't set yet, then we won't show a switch button.
I'm thinking on new systems that get past the gfx.direct3d.last_used_feature_level_idx check and land here.
Maybe it would be better to show the button when it doesn't exist? Or do a real check here to deterministically figure it out?

Comment 19

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #18)
> Comment on attachment 8377638 [details] [diff] [review]
> part 1 - add a supported attribute to nsIWinMetroUtils
> 
> Review of attachment 8377638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
> @@ +786,5 @@
> >      SetRequestMet();
> >      return E_FAIL;
> >    }
> >  
> > +  if (!IsDX10Available() && mRequestType == METRO_RESTART) {
> 
> Do you think this should just be  if (!IsDX10Available()) ?
> 
> ::: widget/windows/winrt/nsWinMetroUtils.cpp
> @@ +367,5 @@
> > +                      NS_LITERAL_STRING("Software\\Mozilla\\Firefox"),
> > +                      nsIWindowsRegKey::ACCESS_READ);
> > +    if (NS_SUCCEEDED(rv)) {
> > +      uint32_t value = 0;
> > +      rv = regKey->ReadIntValue(NS_LITERAL_STRING("MetroD3DAvailable"), &value);
> 
> I'm a bit concerned that if this isn't set yet, then we won't show a switch
> button.
> I'm thinking on new systems that get past the
> gfx.direct3d.last_used_feature_level_idx check and land here.
> Maybe it would be better to show the button when it doesn't exist? Or do a
> real check here to deterministically figure it out?

I am not sure about this either but I think as a general rule they have to launch via the ceh, and the ceh should always do the MetroD3DAvailable check. In which case the MetroD3DAvailable flag would be set. I outlined a couple corner case exceptions in comment 17.

The one way to be sure would be to duplicate the MetroD3DAvailable check code up in CEHHelper down in widget or maybe gfxWindowsPlatform. It's not that big of a block, so it's no big deal, just a little duplication.

Comment 20

[Brian R. Bondy [:bbondy]]

> I am not sure about this either but I think as a general rule they have to launch via the ceh, and the 
> ceh should always do the MetroD3DAvailable check. In which case the MetroD3DAvailable flag would be set. 
> I outlined a couple corner case exceptions in comment 17.

The value is set in the IsDX10Available call, and only when ahe == AHE_IMMERSIVE and we are the default browser.  So that means that we wouldn't be showing someone the button to switch to Metro in the Australis menu in that case right?

http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp#294

I think duplicating is the right way to do it, that part of code shouldn't be hit too often.

Comment 21

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #20)
> > I am not sure about this either but I think as a general rule they have to launch via the ceh, and the 
> > ceh should always do the MetroD3DAvailable check. In which case the MetroD3DAvailable flag would be set. 
> > I outlined a couple corner case exceptions in comment 17.
> 
> The value is set in the IsDX10Available call, and only when ahe ==
> AHE_IMMERSIVE and we are the default browser.  So that means that we
> wouldn't be showing someone the button to switch to Metro in the Australis
> menu in that case right?

part 1 adds the call to Execute(), so it should be set.

> http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/
> commandexecutehandler/CommandExecuteHandler.cpp#294
> 
> I think duplicating is the right way to do it, that part of code shouldn't
> be hit too often.

I'm ok with this. I think I'll try to share this via a new exported header / cpp so we don't have two code paths to maintain.

Comment 22

[Brian R. Bondy [:bbondy]]

I didn't think about the Execute call change, I'm fine if you want to go with it in that case.

Comment 23

[Jim Mathies [:jimm]]

Attachment: part 1 - add a supported attribute to nsIWinMetroUtils

Updated per comments.

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8381544 [details] [diff] [review]
part 1 - add a supported attribute to nsIWinMetroUtils

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

::: browser/app/nsBrowserApp.cpp
@@ -32,5 @@
>  #include "nsStringGlue.h"
>  
>  // Easy access to a five second startup delay used to get
>  // a debugger attached in the metro environment. 
> -// #define DEBUG_delay_start_metro

doh, removed in my local patch.

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 8381544 [details] [diff] [review]
part 1 - add a supported attribute to nsIWinMetroUtils

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

nit cleanup trailing whitespaces in patch

::: browser/app/nsBrowserApp.cpp
@@ +32,5 @@
>  #include "nsStringGlue.h"
>  
>  // Easy access to a five second startup delay used to get
>  // a debugger attached in the metro environment. 
> +#define DEBUG_delay_start_metro

nit: I think you didn't mean to include this

::: browser/metro/shell/commandexecutehandler/CEHHelper.cpp
@@ +140,5 @@
>    if (GetDWORDRegKey(metroDX10Available, isDX10Available)) {
>      return isDX10Available;
>    }
> +  bool check = D3DFeatureLevelCheck();
> +  SetDWORDRegKey(metroDX10Available, check);

nit: this may give a conversion warning, fix if so.

Comment 26

[Marina Samuel [:emtwo]]

Comment on attachment 8377639 [details] [diff] [review]
part 2 - modify australis front end check to use nsIWinMetroUtils

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

I believe we need to make changes in a couple of other places too where a similar check is happening:

http://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1683 for Metro bookmarks showing up on Firefox,
http://dxr.mozilla.org/mozilla-central/source/browser/components/places/tests/unit/head_bookmarks.js#66 and an associated test case,
http://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#80 and perhaps for telemetry too

Comment 27

[Marina Samuel [:emtwo]]

Comment on attachment 8377672 [details] [diff] [review]
part 2 for beta

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

Ditto, I think we should check for metro.supported when deciding whether to display the bookmarks folder as well.

Comment 28

[Jim Mathies [:jimm]]

Attachment: part 2 - modify australis front end check to use nsIWinMetroUtils

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=ab9caa09a7f8

Comment 29

[Jim Mathies [:jimm]]

Attachment: part 2 for beta

Comment 30

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=5961efee54b9

new try push after I fixed a silly coding error.

Comment 31

[Jim Mathies [:jimm]]

Attachment: part 2 - modify australis front end check to use nsIWinMetroUtils

Comment 32

[Jim Mathies [:jimm]]

Attachment: part 2 for beta

Comment 33

[Jim Mathies [:jimm]]

beta try build - 
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=7a5a39a59599

Comment 34

[Marina Samuel [:emtwo]]

Comment on attachment 8383195 [details] [diff] [review]
part 2 - modify australis front end check to use nsIWinMetroUtils

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

Looks good

Comment 35

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/8d0fa796f925
remote:   https://hg.mozilla.org/integration/fx-team/rev/a905fb8f85b0

Comment 36

[:Gijs (he/him)]

Shouldn't the Australis tests also be updated?

Comment 37

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #36)
> Shouldn't the Australis tests also be updated?

Do you mean non-australis tests? This landed on mc, so I'm assuming the tests I updated were australis related.

Comment 38

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #37)
> (In reply to :Gijs Kruitbosch from comment #36)
> > Shouldn't the Australis tests also be updated?
> 
> Do you mean non-australis tests? This landed on mc, so I'm assuming the
> tests I updated were australis related.

Ah, I see what you're referring to. Yeah looks like these need to be updated as well.

http://mxr.mozilla.org/mozilla-central/search?find=%2F&string=isInWin8

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d0fa796f925
https://hg.mozilla.org/mozilla-central/rev/a905fb8f85b0

Comment 40

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 41

[Jim Mathies [:jimm]]

To test set the the following DWORD reg value to zero - 

HKEY_CURRENT_USER/SOFTWARE/Mozilla/Firefox/MetroD3DAvailable = 0

That should put the browser in a state where it thinks the hardware can't support the metro backend.

Comment 42

[Kamil Jozwiak [:kjozwiak]]

QA notes when going through testing and verification:

Once Bug #979007 lands, the following steps can be used for verification:

- Turn off acceleration via the options panel
- Clear the pref 'gfx.direct3d.last_used_feature_level_idx'
- Once that's completed, see comment #41 to change the appropriate reg key

Comment 43

[Jim Mathies [:jimm]]

Comment on attachment 8381544 [details] [diff] [review]
part 1 - add a supported attribute to nsIWinMetroUtils

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Switch to metro ux work in desktop firefox.
User impact if declined:
Some users who have hardware that can't support rendering metro will still see the "switch to metro" option in the drop down options and in the fx button on beta. If they click this the browser will fail to start.  
Testing completed (on m-c, etc.): yes.
Risk to taking this patch (and alternatives if risky): moderate level. I had to rework the code that decided whether the option should be displayed in the ui, including reworking some complex d3d detection code. Generally though this has been confirmed on mc as working correctly, and we haven't seen any regressions.
String or IDL/UUID changes made by this patch: none.

This request is for the patches here (mc patches, australis specific patches for aurora, the beta fix for the fx button, plus the follow up fix in bug 979007).

Comment 44

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using the following Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-06-03-02-01-mozilla-central/

- Used the test cases and the information from both comment #41 and comment #42
- Ensured that once I launched Firefox, the "Windows 8 Touch" button isn't visible under the Australis options panel
- Ensured that the "Windows 8 Touch" button is not appearing under the "Customization" screen under the Australis options panel
- Ensured that launching firefox through the Windows Metro Start menu launched fxdesktop and not fxmetro
- Ensured that launching firefox through the "Apps" list under Windows Metro launched fxdesktop and not fxmetro
- Ensured that the Firefox shortcut on the desktop and under the taskbar launched fxdesktop and not fxmetro
- Went through the above test cases with both the X1 Carbon and the Surface Pro 2

Jim, while the "Windows 8 Touch" button has been removed from the Australis options panel, I did notice that it's still available under the "File" menu if the user enables the "Menu Bar" (Relaunch in Nightly for Windows 8 Touch). We should probably remove this if the user's machine doesn't support fxmetro, it doesn't make sense removing the Australis portion and keeping the entry under the file menu. Let me know if this is valid or if there's a reason it's being left behind. I'll create a new bug if it's a problem.

Comment 45

[Jim Mathies [:jimm]]

Bah, these are all supposed to go away. I didn't know about the file menu.

Comment 46

[Kamil Jozwiak [:kjozwiak]]

Would you like me to create a new issue or just re-open this one and get it fixed in this ticket?

Comment 47

[Jim Mathies [:jimm]]

Comment on attachment 8381544 [details] [diff] [review]
part 1 - add a supported attribute to nsIWinMetroUtils

canceling these for now since we're missing the menu item.

Comment 48

[Jim Mathies [:jimm]]

Attachment: patch for beta

Comment 49

[Jim Mathies [:jimm]]

Comment on attachment 8386857 [details] [diff] [review]
patch for beta

sorry, that was missing the original patch. I need to run this through try anyway to see if this causes any test failures.

This work is looking less likely for beta uplift. I guess it depends on when we cut the final test release vs. finishing this up and getting review.

Comment 50

[Jim Mathies [:jimm]]

Attachment: patch for beta

Comment 51

[Jim Mathies [:jimm]]

beta try push:
https://tbpl.mozilla.org/?tree=Try&rev=6bab209cec3d

Comment 52

[Marco Mucci [:MarcoM]]

Removing release blocking tags based on Jim's comments.

Comment 53

[Jim Mathies [:jimm]]

Comment on attachment 8386858 [details] [diff] [review]
patch for beta

We missed the beta window.

Comment 54

[Jim Mathies [:jimm]]

Aurora try run with all the relevant work - 

https://tbpl.mozilla.org/?tree=Try&rev=200d35779050

Bug 980366 - Remove windows touch menu item in desktop when metro is not supported. r=gavin
Bug 979007 - When attempting to read and write 'supported' associated registry value in nsWinMetroUtils, use the right access mask. r=tabraldes
Bug 978127 - Update various win8 specific australis customized ui tests to use new Services.metro.supported prop. r=gijs
Bug 969831 - Use the new feature level check for desktop browser 'switch to metro' ux elements. r=msamuel
Bug 969831 - Share code for checking minumum d3d feature level required for running metro and expose this information (including cached check results) via nsIWinMetroUtils. r=bbondy