(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo Reporter
	Description • 9 years ago

When starting Firefox from the tablet mode launcher, the launcher briefly disappears, Windows shows the desktop picture and then the launcher re-appears. The Firefox window does actually open, but it is in the background and one has to switch to it manually.

	:Gijs (he/him) Assignee
	Comment 1 • 9 years ago

Which version of Firefox is this? This sounds like effectively bug 1165303 which is fixed in 40 and higher.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo Reporter
	Comment 2 • 9 years ago

I think it was essentially the same issue.
Interestingly enough, it doesn't happen in 38 either at the moment, which makes me think that MS might have fixed things on their end as well.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 3 • 9 years ago

I'm still seeing this issue on a Microsoft Surface Pro 2 device running Windows 10 64bit (10240).

It reproduces every single time with:
*latest Nightly 42.0a1
*latest DevEdition 41.0a2
*Firefox 40.0b8
*Firefox 39, build ID: 20150630154324.

This might have regressed with the newer Windows 10 builds.

	:Gijs (he/him) Assignee
	Comment 4 • 9 years ago

I can reproduce this on 10162 as well. Maybe it was different from bug 1165303? Though comment #2 just seems to indicate Windows itself has flipflopped on this in some way.

This doesn't happen with Chrome. Going to continue to investigate.

	:Gijs (he/him) Assignee
	Comment 5 • 9 years ago

Minor updates:

1) doesn't happen when launched from terminal. Or MSVS. Or anywhere else that I can see. No idea what's different about the start menu.
2) extending the fix of bug 1165303 to never do its little repainting workaround doesn't seem to help
3) we don't get any different commandline arguments (that I can tell)
4) e10s or not doesn't seem to make a difference

Going to try to log some stuff (somehow, as I can't control the commandline parameters so printf ends up in /dev/null by default...) tomorrow and see if that can shed some light on what's going on.

	:Gijs (he/him) Assignee
	Comment 6 • 9 years ago

So I've tried logging all the relevant messages I could think of (WM_SIZE, WM_MOVE, WM_SETFOCUS, WM_KILLFOCUS, WM_NCACTIVATE, WM_ACTIVATE, WM_SETTEXT, WM_SHOWWINDOW, WM_ACTIVATEAPP) and I don't see anything different between opening Firefox from the start screen or from another app (where the former works, and the latter doesn't). I've tried logging some of the focus calls as well, that didn't seem to be different either.

The one thing I did notice is that if you pin firefox to the taskbar and keep the taskbar icons around in tablet mode, then:

1) launching while another app is open works
2) launching while the start screen is open has the same problem as using the start screen tile itself


I've tried to poke at that by using "sleep" in the terminal and launching after the sleep while the start screen is up, but in that case there seems to be no difference between having another application open or having the start screen up - Firefox opens in the background either way. Unlike the broken startscreen case though, this seems like it's expected. The Firefox icon appears in the taskbar, but the display doesn't flash or anything like that - whereas when I open Firefox from the start screen, the start screen first animates out to show my desktop background, and then comes back into view (instead of showing the opened app, which is what it does for other applications).

I'm ending up a bit stuck here, and am not sure how to proceed further. Jim, do you have ideas what could help or what could be going on here? Maybe we should try flagging our Windows/MS contacts for help?

	:Gijs (he/him) Assignee
	Comment 7 • 9 years ago

For reference, this is some of my debug logging: https://pastebin.mozilla.org/8841702

	Jim Mathies [:jimm]
	Comment 8 • 9 years ago

I tried disabling all that WM_SETTEXT code (the handler and SetTitle), which improved the loading experience with less flickering, but in the end the window still ended up in the background. Also should add that I'm still stuck on build 10162, I'll try to get this tablet updated soon.

	Jim Mathies [:jimm]
	Comment 9 • 9 years ago

nc client drawing doesn't seem to impact this.

	Jim Mathies [:jimm]
	Comment 10 • 9 years ago

Hmm, so in a debug build, which opens a terminal window for logging really early, I don't see the problem.

In metrofx when we failed to display a window and take focus quickly enough Windows would just kill the app. It's possible that that kind of timeout is being applied here and we're hitting it thanks to our sucky cold start + window creation time. Windows might be giving up on use because we don't create a window soon enough.

This said, I don't see this as intermittent so I'm not sure. If the above scenario is correct I'd expect to get the right result occasionally. So far I haven't seen that.

	Jim Mathies [:jimm]
	Comment 11 • 9 years ago

In chrome canary I see something even weirder - I see a fresh fullscreen blank window display briefly, then close, then the browser window fades in.

	Jim Mathies [:jimm]
	Comment 12 • 9 years ago

Microsoft edge on the other hand handles this more like the old metro apps - it displays a splash screen with the 'e' in the center like in 8, then the browser pops in from behind that.

	Jim Mathies [:jimm]
	Comment 13 • 9 years ago

We should look at window creation, it's pretty clear something early on in startup is breaking this. Maybe we create another visible window briefly and then hide that and Windows thinks that's the browser? Just throwing out some ideas.

	Jim Mathies [:jimm]
	Comment 14 • 9 years ago

Adding the following code fixes the core problem here but breaks a bunch of other stuff. Looks like my guess was either spot on or really close.

diff --git a/widget/windows/nsWindow.cpp b/widget/windows/nsWindow.cpp
--- a/widget/windows/nsWindow.cpp
+++ b/widget/windows/nsWindow.cpp
@@ -569,7 +569,9 @@ nsWindow::Create(nsIWidget *aParent,
     NS_WARNING("nsWindow CreateWindowEx failed.");
     return NS_ERROR_FAILURE;
   }
-
+  if (mWindowType == eWindowType_toplevel) {
+    ::ShowWindow(mWnd, SW_SHOWNORMAL);
+  }
   if (mIsRTL && WinUtils::dwmSetWindowAttributePtr) {
     DWORD dwAttribute = TRUE;    
     WinUtils::dwmSetWindowAttributePtr(mWnd, DWMWA_NONCLIENT_RTL_LAYOUT, &dwAttribute, sizeof dwAttribute);


I need to get back to e10s work. ni me if you need any additional help.

	:Gijs (he/him) Assignee
	Comment 15 • 9 years ago

(In reply to Jim Mathies [:jimm] from comment #14)
> Adding the following code fixes the core problem here but breaks a bunch of
> other stuff. Looks like my guess was either spot on or really close.

> +    ::ShowWindow(mWnd, SW_SHOWNORMAL);

It sounds like you're saying the issue is how quickly we can get a main window to come up (specifically: call ShowWindow)? Is that right?
Can we detect if we get into this state somehow and force-show the window when it does come up later?


Dão, I know we basically delay doing this in order to get layout and a number of other things right, and to avoid a "flash of unstyled content" or other issues with doing a re-layout or somesuch. Any ideas as to what we could do here (how hard/desirable is a splash screen, or could we get away with showing the window earlier, even if not quite as early as in Jim's strawman patch), assuming my understanding of what Jim is saying is correct? If you don't know the answer to some of that, do you know who might?

	Dão Gottwald [:dao]
	Comment 16 • 9 years ago

Showing the window earlier sounds a bit scary. Well, not really scary, but it might regress things we explicitly wanted to avoid. I don't know the details there though.

vlad or Rob Strong should know about splash screens, I think. We used to have one for a Windows CE Firefox that never shipped if I remember correctly...

	Jim Mathies [:jimm]
	Comment 17 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Jim Mathies [:jimm] from comment #14)
> > Adding the following code fixes the core problem here but breaks a bunch of
> > other stuff. Looks like my guess was either spot on or really close.
> 
> > +    ::ShowWindow(mWnd, SW_SHOWNORMAL);
> 
> It sounds like you're saying the issue is how quickly we can get a main
> window to come up (specifically: call ShowWindow)? Is that right?


No not really I didn't reach a conclusion. I'm saying calling that api on the main window sooner fixed the "browser window launches in the background" bug. We might confuse windows 10 somehow through some sort of startup bug - we create a lot of windows for things during startup, all hidden afaik, but maybe not. It could also be the timing of the main window as my fix suggests. It could also be something else - I still see a lot of flickering associated with those set text calls which concerns me. The whole startup process & window creation needs an audit to figure out what's going wrong. You can can use a simple xulrunner app to test our widget level code too to see if it's fx specific.

My overall point is that something is busted and windows doesn't like it. We need to investigate. I can try to look at this over the next week or so but no promises.

	Justin Dolske [:Dolske]
	Comment 18 • 9 years ago

(In reply to Dão Gottwald [:dao] from comment #16)

> vlad or Rob Strong should know about splash screens, I think. We used to
> have one for a Windows CE Firefox that never shipped if I remember
> correctly...

We did -- bug 502526. But looks like bug 653333 removed it all.

It might be worth considering a bandaid solution here, especially if it can be made specific to Windows 10 in tablet mode.

	:Gijs (he/him) Assignee
	Comment 19 • 9 years ago

(In reply to Jim Mathies [:jimm] from comment #17)
> My overall point is that something is busted and windows doesn't like it. We
> need to investigate. I can try to look at this over the next week or so but
> no promises.

If you have time to look, that would be super helpful. Apart from the xulrunner (or firefox -app, I guess) suggestion, I'm not really super-sure how to proceed with investigating this further.

	:Gijs (he/him) Assignee
	Comment 20 • 9 years ago

Bug 1179395 - do not get individual titlebar button measurements on win8+ because we never use them, r?jimm

	:Gijs (he/him) Assignee
	Comment 21 • 9 years ago

The patch comes with many thanks to the Microsoft folks that gave us more details about why this was happening.

Essentially, we create and quickly destroy a window here: https://dxr.mozilla.org/mozilla-central/rev/87e23922be375985d0b1906ed5ba5f095f323a38/widget/windows/nsUXThemeData.cpp#174-204 . The destruction of that window is detected by the start menu and causes us to get backgrounded.

We can avoid doing this on Win8+ because the measures we need the window for never get used except on non-aero/non-compositor themes/runtimes, which don't exist on win8+.

I am also hoping that this might actually help our startup time a teeny weeny bit.

	Jim Mathies [:jimm]
	Comment 22 • 9 years ago

Comment on attachment 8654065 [details]
MozReview Request: Bug 1179395 - do not get individual titlebar button measurements on win8+ because we never use them, r?jimm

Please move this check here - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#480

which is called here - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#675

	:Gijs (he/him) Assignee
	Comment 23 • 9 years ago

Comment on attachment 8654065 [details]
MozReview Request: Bug 1179395 - do not get individual titlebar button measurements on win8+ because we never use them, r?jimm

(In reply to Jim Mathies [:jimm] from comment #22)
> Please move this check here - 
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#480
> 
> which is called here - 
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#675

I don't think that works. We still use the buttonbox info on glass / post-aero (on win8 as well, I checked...), which is created here:

https://dxr.mozilla.org/mozilla-central/rev/87e23922be375985d0b1906ed5ba5f095f323a38/widget/windows/nsUXThemeData.cpp#159-169

So I don't think we can just not call that function at all. Am I missing something?

	Jim Mathies [:jimm]
	Comment 24 • 9 years ago

Comment on attachment 8654065 [details]
MozReview Request: Bug 1179395 - do not get individual titlebar button measurements on win8+ because we never use them, r?jimm

ahh! nice catch.

	Jim Mathies [:jimm]
	Comment 25 • 9 years ago

I took this for a spin on w10, didn't see any issues with the other default themes available, fwiw.

	Pulsebot
	Comment 26 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/0c9b89e0f985

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0c9b89e0f985

	Cornel Ionce [:noni] [Hubs QA]
	Comment 28 • 9 years ago

Confirming the fix using latest 43.0a1 Nightly, build ID: 20150831030209.

	:Gijs (he/him) Assignee
	Comment 29 • 9 years ago

Comment on attachment 8654065 [details]
MozReview Request: Bug 1179395 - do not get individual titlebar button measurements on win8+ because we never use them, r?jimm

Approval Request Comment
[Feature/regressing bug #]: Windows 10
[User impact if declined]: This makes Firefox seem unresponsive when starting it in win10 tablet mode.
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: very low, one-line change that removes checks for unused moz-appearance values that create a separate window on win8 and later.
[String/UUID change made/needed]: nope

	Sylvestre Ledru [:Sylvestre]
	Comment 30 • 9 years ago

Comment on attachment 8654065 [details]
MozReview Request: Bug 1179395 - do not get individual titlebar button measurements on win8+ because we never use them, r?jimm

Low risk, has been verified, potentially high impact, taking it.

	Carsten Book [:Tomcat]
	Comment 31 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/452f6270615d

	Carsten Book [:Tomcat]
	Comment 32 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/67a8fcd3a7d5

	Cornel Ionce [:noni] [Hubs QA]
	Comment 33 • 9 years ago

Also confirming the fix on:
* latest Aurora, build ID 20150909004021
* Firefox 41.0b8, build ID 20150907144446