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

RESOLVED FIXED in Firefox 30

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: amorvincitomnes, Assigned: jimm)

Tracking

28 Branch
Firefox 30
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 affected, firefox29 affected, firefox30 fixed)

Details

(Whiteboard: p=2 s=it-30c-29a-28b.2 r=ff30 [qa-])

Attachments

(3 attachments, 9 obsolete attachments)

(Reporter)

Description

5 years ago
Posted file 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.
(Reporter)

Comment 1

5 years ago
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
Whiteboard: [triage]
(Assignee)

Comment 2

5 years ago
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?
(Reporter)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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.
(Reporter)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
(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.
Blocks: metrobacklog
No longer blocks: metrov1backlog
Whiteboard: [triage]
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

5 years ago
Duplicate of this bug: 969409
Whiteboard: p=0
(Assignee)

Comment 9

5 years ago
(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.
Assignee: nobody → jmathies
Blocks: metrov1backlog, 967403
No longer blocks: metrobacklog
Keywords: crash
Summary: Firefox for Touch crashes during launch → Switching from desktop to metro fails if the device doesn't support directx feature level 9.3 or higher
Whiteboard: p=0 → [release28] p=2
(Assignee)

Comment 11

5 years ago
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
Flags: needinfo?(msamuel)
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.
Flags: needinfo?(msamuel)
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Attachment #8376716 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8377639 - Attachment description: part 2 - modify autralis front end check to use nsIWinMetroUtils → part 2 - modify australis front end check to use nsIWinMetroUtils
(Assignee)

Comment 16

5 years ago
Posted patch part 2 for beta (obsolete) — Splinter Review
(Assignee)

Comment 17

5 years ago
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.
Attachment #8377638 - Flags: feedback?(netzen)
Status: NEW → ASSIGNED
Priority: -- → P1
QA Contact: kamiljoz
Whiteboard: [release28] p=2 → [release28] p=2 s=it-30c-29a-28b.2 r=ff28
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?
Attachment #8377638 - Flags: feedback?(netzen)
(Assignee)

Comment 19

5 years ago
(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.
> 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.
(Assignee)

Comment 21

5 years ago
(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.
I didn't think about the Execute call change, I'm fine if you want to go with it in that case.
(Assignee)

Comment 23

5 years ago
Updated per comments.
Attachment #8377638 - Attachment is obsolete: true
Attachment #8381544 - Flags: review?(netzen)
(Assignee)

Updated

5 years ago
Attachment #8377639 - Flags: review?(msamuel)
(Assignee)

Updated

5 years ago
Attachment #8377672 - Flags: review?(msamuel)
(Assignee)

Comment 24

5 years ago
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 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.
Attachment #8381544 - Flags: review?(netzen) → review+
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
Attachment #8377639 - Flags: review?(msamuel) → feedback+
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.
Attachment #8377672 - Flags: review?(msamuel) → feedback+
(Assignee)

Comment 29

5 years ago
Posted patch part 2 for beta (obsolete) — Splinter Review
(Assignee)

Comment 30

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

new try push after I fixed a silly coding error.
(Assignee)

Comment 31

5 years ago
Attachment #8382929 - Attachment is obsolete: true
Attachment #8382933 - Attachment is obsolete: true
Attachment #8383195 - Flags: review?(msamuel)
(Assignee)

Comment 32

5 years ago
Posted patch part 2 for beta (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8383197 - Flags: review?(msamuel)
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
Attachment #8383195 - Flags: review?(msamuel) → review+
Attachment #8383197 - Flags: review?(msamuel) → review+

Comment 36

5 years ago
Shouldn't the Australis tests also be updated?
(Assignee)

Comment 37

5 years ago
(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.
(Assignee)

Comment 38

5 years ago
(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
(Assignee)

Updated

5 years ago
Blocks: 978127
https://hg.mozilla.org/mozilla-central/rev/8d0fa796f925
https://hg.mozilla.org/mozilla-central/rev/a905fb8f85b0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
For testing and verification.  Reopen if any defects found.
Flags: needinfo?(kamiljoz)
(Assignee)

Updated

5 years ago
Flags: needinfo?(jmathies)
(Assignee)

Comment 41

5 years ago
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.
Flags: needinfo?(jmathies)
(Assignee)

Updated

5 years ago
Flags: needinfo?(jmathies)
(Assignee)

Updated

5 years ago
Depends on: 979007
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
(Assignee)

Comment 43

5 years ago
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).
Attachment #8381544 - Flags: approval-mozilla-beta?
Attachment #8381544 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jmathies)
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.
Flags: needinfo?(jmathies)
(Assignee)

Comment 45

5 years ago
Bah, these are all supposed to go away. I didn't know about the file menu.
Flags: needinfo?(jmathies)
Would you like me to create a new issue or just re-open this one and get it fixed in this ticket?
Depends on: 980366
(Assignee)

Comment 47

5 years ago
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.
Attachment #8381544 - Flags: approval-mozilla-beta?
Attachment #8381544 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 48

5 years ago
Posted patch patch for beta (obsolete) — Splinter Review
Attachment #8383197 - Attachment is obsolete: true
Attachment #8386857 - Flags: review?(gavin.sharp)
(Assignee)

Comment 49

5 years ago
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.
Attachment #8386857 - Flags: review?(gavin.sharp)
(Assignee)

Comment 50

5 years ago
Posted patch patch for beta (obsolete) — Splinter Review
Attachment #8386857 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
No longer blocks: 978127
Depends on: 978127
Removing release blocking tags based on Jim's comments.
Blocks: metrobacklog
No longer blocks: metrov1backlog
Whiteboard: [release28] p=2 s=it-30c-29a-28b.2 r=ff28 → p=2 s=it-30c-29a-28b.2 r=ff30
(Assignee)

Comment 53

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

We missed the beta window.
Attachment #8386858 - Attachment is obsolete: true
(Assignee)

Comment 54

5 years ago
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
Whiteboard: p=2 s=it-30c-29a-28b.2 r=ff30 → p=2 s=it-30c-29a-28b.2 r=ff30 [qa-]
Flags: needinfo?(kamiljoz) → needinfo?
Flags: needinfo?
You need to log in before you can comment on or make changes to this bug.