772714 - right-click crashes metrofx when running on the desktop on win7

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

I get - 

###!!! ASSERTION: Attempting to load winrt libs in non-metro environment. (Winrt variable type placed in global scope?): '!IsWinRTDLLPresent(pdli, kwinrtprelim)', file f:/Mozilla/firefox/elm/toolkit/library/nsDllMain.cpp, line 90

Not sure when this got introduced. Hopefully there's a work around.

Comment 1

[Jim Mathies [:jimm]]

xul!__delayLoadHelper2+0x00000000000000E7 (f:\dd\vctools\delayimp\delayhlp.cpp, line 286)
xul!_tailMerge_api_ms_win_core_winrt_string_l1_1_0_dll+0x000000000000000D
xul!Platform::String::String+0x000000000000002F (c:\program files (x86)\microsoft visual studio 11.0\vc\include\vccorlib.h, line 1520)
xul!mozilla::widget::SecondaryTileController::IsTilePinned+0x0000000000000080 (f:\mozilla\firefox\elm\widget\windows\winrt\secondarytilecontroller.cpp, line 108)
xul!NS_InvokeByIndex_P+0x0000000000000027 (f:\mozilla\firefox\elm\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, line 71)
xul!CallMethodHelper::Invoke+0x0000000000000041 (f:\mozilla\firefox\elm\js\xpconnect\src\xpcwrappednative.cpp, line 3071)
xul!CallMethodHelper::Call+0x00000000000000CA (f:\mozilla\firefox\elm\js\xpconnect\src\xpcwrappednative.cpp, line 2405)
xul!XPCWrappedNative::CallMethod+0x00000000000001D4 (f:\mozilla\firefox\elm\js\xpconnect\src\xpcwrappednative.cpp, line 2371)
xul!XPC_WN_CallMethod+0x000000000000027C (f:\mozilla\firefox\elm\js\xpconnect\src\xpcwrappednativejsops.cpp, line 1474)

Looks like something in bringing up the app bar & IsTilePinned. We might be able to use the XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Desktop test to avoid stuff like this. Although we wouldn't want to do anything that would change behavior of tests.

Comment 2

[Brian R. Bondy [:bbondy]]

So this happens only when using -metrodesktop? Or when launching firefox without command line args?

Comment 3

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> So this happens only when using -metrodesktop? Or when launching firefox
> without command line args?

Release elm build with the -metrodesktop command line option on Win7. The older desktop browser is fine. 

I think what's happening is that right-click triggers the app bar which somehow hits code in the tile controller that creates winrt strings. That triggers a load of the winrt string lib which doesn't exist on win7. The delay load hook catches that and asserts.

Comment 4

[Jim Mathies [:jimm]]

Attachment: fix

Recent front end changes are now triggering this on start up.

GetSnappedState should probably fail, but the front end doesn't deal well with the unexpected exception.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 641790 [details] [diff] [review]
fix

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

Sorry I was going to work on this but I can't get VS2012 working on my Win7 machine.

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +149,5 @@
>  nsWinMetroUtils::GetSnappedState(PRInt32 *aSnappedState)
>  {
>    NS_ENSURE_ARG_POINTER(aSnappedState);
> +  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Desktop) {
> +    *aSnappedState = 0;

Value 0 is already used for FullScreenLandscape, so maybe we should return -1 in this case.  That or some big value that Windows won't reuse anytime soon.  Current values are [0, 3] inclusive.

http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.viewmanagement.applicationviewstate.aspx

Comment 6

[Brian R. Bondy [:bbondy]]

re: the review comment, Jim explained that we want the UI to think it's full without extra front end code.  Although this doesn't say if we're in portrait or landscape though.

That's fine w/ me overall but would prefer to update the "= 0" to "= (PRInt32) ApplicationViewState::FullScreenLandscape ".

Comment 7

[Jim Mathies [:jimm]]

Attachment: patch

Comment 8

[Jim Mathies [:jimm]]

I ended up fixing the front end and failing.

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 641859 [details] [diff] [review]
patch

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

This seems best, thanks!

Comment 10

[Jim Mathies [:jimm]]

https://hg.mozilla.org/projects/elm/rev/417ee5ff7d36

Comment 11

[Jim Mathies [:jimm]]

oops, not on mc yet.