1063121 - Cannot change window size mode and window controls disappear after restarting in full screen mode

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: regression

Steps To Reproduce:
1. Key press F11 into fullscreen
2. Ctrl+w to quit browser
3. Restart Browser
   --- Restored previous window size mode(fullscreen) as expected
4. Key press F11 to exit fullscreen

Actual Results:
 Cannot change window size mode.
 No window control buttons

Expected Results:
 The window size mode should change to normal mode.


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/beddd6d4bcdf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118025935
Bad:
http://hg.mozilla.org/mozilla-central/rev/f2adb62d07eb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118075715
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=beddd6d4bcdf&tochange=f2adb62d07eb

Comment 1

[Alice0775 White]

Add step

Steps To Reproduce:
0. Enabled Title bar

Comment 2

[:Gijs (out for now; he/him)]

Alice, is there any chance you'd be able to look at when this regressed on the UX branch? I believe you can use "-r ux" with mozregression to use it against the ux branch. The branch was reset in April 2013 so presumably this happened sometime between then and November. I expect this is a widget issue, and so I think even a one-day regression window would really help.

(I would check myself but I'm not back to my regular Windows machine until Monday)

Comment 3

[:Gijs (out for now; he/him)]

CC'ing the world for ideas/help... :-)

Comment 4

[Alice0775 White]

It may be necessary to repeat str to reproduce the problem.

Regression window(ux)
Good:
http://hg.mozilla.org/projects/ux/rev/bfb5c27c566b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910040201
Bad:
http://hg.mozilla.org/projects/ux/rev/339479a60c1c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911040201
Pushlog:
http://hg.mozilla.org/projects/ux/pushloghtml?fromchange=bfb5c27c566b&tochange=339479a60c1c

Suspect: 
7f26161e7dd6	Dão Gottwald — Bug 882009 - Make the tabs toolbar margin respect the tabsintitlebar attribute. r=Gijs
ed0e0347b778	Dão Gottwald — Bug 882009 - Invert updateTitlebarDisplay / TabsInTitlebar dependency. f=MattN r=Gijs

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm tracking for all active releases but given that this hasn't blown up, depending on the fix and when it is available, we may or may not choose to take this fix in 33 or on ESR31.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Dao, can you help here? Thanks

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Gijs, I hope you had good holidays. Sorry for the n-i on the day you come back but could you help here? thanks

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Gijs, I hope you had good holidays. Sorry for the n-i on the day you come
> back but could you help here? thanks

I'm busy with other work until at least Tuesday/Wednesday, I'm afraid. I'll try to look then if Dão doesn't get a chance before then, so leaving needinfo.

Out of curiosity, Alice, does this only reproduce with the titlebar on/off, or in both cases?

Comment 9

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> > Gijs, I hope you had good holidays. Sorry for the n-i on the day you come
> > back but could you help here? thanks
> 
> I'm busy with other work until at least Tuesday/Wednesday, I'm afraid. I'll
> try to look then if Dão doesn't get a chance before then, so leaving
> needinfo.
> 
> Out of curiosity, Alice, does this only reproduce with the titlebar on/off,
> or in both cases?

w/ needinfo...

Comment 10

[Alice0775 White]

It does not happen with titlebar off.

Comment 11

[:Gijs (out for now; he/him)]

Interestingly, I can't reproduce this very well on Windows 8, in the sense that the window control buttons work fine - but the titlebar disappears. :-\

I'll look if I can reproduce more completely on Windows 7.

Comment 12

[:Gijs (out for now; he/him)]

I also can't reproduce this fully on Windows 7 - I see the titlebar disappearing, but the window controls remain, and are functional (though the titlebar fog overlaps them).

Comment 13

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #12)
> I also can't reproduce this fully on Windows 7 - I see the titlebar
> disappearing, but the window controls remain, and are functional (though the
> titlebar fog overlaps them).

Regarding window control buttons, It is reproduced on Basic and Classic windows 7 visual style, not on Aero.

Comment 14

[:Gijs (out for now; he/him)]

FWIW, I strongly suspect this is a widget bug. I spent some time debugging this on and off over the last week (busy with other things...) but the attributes set on the window element (which control most of this for the XUL/CSS) and the visibility of the titlebar XUL element is the same in the working and non-working cases. I'd suggest actually debugging this on the widget level.

Comment 15

[:Gijs (out for now; he/him)]

... in fact, looking at this code, AIUI:

1) nsWindow::MakeFullscreen ( http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#2837 ) gets called for both going in and going out of fullscreen
2) That calls nsBaseWidget::MakeFullscreen ( http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseWidget.cpp#691 )
3) That calls Resize() *if and only if* there are previous dimensions stored (which there won't be for windows that start in fullscreen)
4) That then deals with setting the titlebar / non-client region, and a bunch of other stuff ( http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#1426 ).

I guess this used to work because windows might not have started out in fullscreen before Dão's refactoring, meaning mOriginalBounds wasn't null (NB: I've not tested this assumption).

Jim, does this make sense, or am I missing something? Can we fix this by making MakeFullscreen in nsBaseWidget always call Resize(), or having nsWindow call Resize() and/or the things it does itself if it realizes that is necessary?

Comment 16

[:Gijs (out for now; he/him)]

At least on win8, mOriginalBounds is also non-null in the started-in-fullscreen case, so there goes that theory.

Comment 17

[:Gijs (out for now; he/him)]

The universe decided this isn't fun enough yet: if I set a breakpoint in nsWindow::SetSizeMode, the stepping / timing differences make the bug go away.

Comment 18

[:Gijs (out for now; he/him)]

OK, so I'm getting closer, finally. First off, comment #14 was wrong - initially, the chromemargin attribute is set on the window. It doesn't get changed until you exit fullscreen mode. This is because there's a check in TabsInTitlebar._update for window.fullScreen, and it doesn't run at all in that case.

1) re-adding http://hg.mozilla.org/projects/ux/rev/ed0e0347b778#l1.12 fixes the chromemargin situation and the bug.
2) removing the fullscreen check at the top of TabsInTitlebar._update fixes the chromemargin situation and *doesn't* fix the bug

(2) not working is what I find surprising. (1) strikes me as a bit of a wallpaper, although we could certainly use it to un-fubar things in the short term if people think that's a good idea, potentially only for this case (window opens with sizemode=fullscreen and drawintitlebar=false).

On the widget side, it seems nsWindow::mCustomNonClient doesn't ever get unset from its initial value of "true" in the (2) and the existing case, whereas it does in the (1) case. That seems like a bug to me, but in fact, it seems it never gets unset unless you set the attribute to -1,-1,-1,-1, which the code says means "request a reset" ( http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#2293 ). I also randomly ran into this snippet: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#6117 which I see triggering a no-op call to SetSizeModeChanged in the broken case when switching from fullscreen to normal mode, but not in the "correct" case. I don't know why.

Seems to me like the ideal solution involves making the Windows code deal with removing the attribute at any time - but then, I'm fairly sure that we don't do anything special for this when hitting the titlebar button in customize mode, and that seems to work fine...

Matt, Mike, I recall that you dealt with this stuff extensively, any thoughts considering the above?

Comment 19

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #18)
> Matt, Mike, I recall that you dealt with this stuff extensively, any
> thoughts considering the above?

I would check if this creates new reflows upon window with browser_windowopen_reflows.js and also check the impact on tpaint/ts_paint.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Matthew N. [:MattN] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #18)
> > Matt, Mike, I recall that you dealt with this stuff extensively, any
> > thoughts considering the above?
> 
> I would check if this creates new reflows upon window with
> browser_windowopen_reflows.js and also check the impact on tpaint/ts_paint.

It shouldn't if we limit this to fullscreen + tabsintitlebar disabled, I would have thought?

But really, I'm mostly curious about whether you think this points to something in Windows-widget-land that needs fixing. I really don't understand why removing chromemargin when we do isn't "good enough" so to speak. Thoughts on that particular bit? :-)

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Too late in the 33 cycle!

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Gijs - You look to be the most active in the bug. Can you take it and continue to drive it? Note that 34 is now on beta. If we want a fix in 34 (I'm not sure that this is necessary), this bug will need a fix in the next couple of weeks.

Comment 23

[:Gijs (out for now; he/him)]

(In reply to Lawrence Mandel [:lmandel] from comment #22)
> Gijs - You look to be the most active in the bug. Can you take it and
> continue to drive it? Note that 34 is now on beta. If we want a fix in 34
> (I'm not sure that this is necessary), this bug will need a fix in the next
> couple of weeks.

For the latter question: Necessary, no, but desirable, yes. We've been shipping this since 29, so if we ship another release with this unfixed, that won't kill us, but ideally we should figure this out and fix it.

Matt brought this up while I was in MV last week and mentioned he didn't know the answer to comment #20 without having to dig/step through the code himself. I'll leave the needinfo for me and try to make time to do more digging later today. I suspect there *is* an issue in widget/win32 here, but I'll need to look at it in more detail to be sure.

Comment 24

[:Gijs (out for now; he/him)]

Here's some alternative STR that mess up:

0) ensure titlebar is disabled
1) go into fullscreen
2) toggle titlebar on
3) go out of fullscreen

Not sure if the cause is the same yet. :-\

Comment 25

[:Gijs (out for now; he/him)]

Ah, finally, some progress. It seems that removing chromemargin calls ResetChromeMargin just fine, so that part works correctly.

Unfortunately, the first thing ResetChromeMargin does is check if mHideChrome is set to true:

http://hg.mozilla.org/mozilla-central/annotate/7e3c85754d32/widget/windows/nsWindow.cpp#l2293

If so, it bails out. mHideChrome is updated when going in and out of fullscreen (through widget's MakeFullscreen methods):

http://hg.mozilla.org/mozilla-central/annotate/7e3c85754d32/widget/windows/nsWindow.cpp#l2738

whereas chromemargin is updated through TabsInTitlebar, from a resize event caught by tabbrowser.xml. These race, which explains comment 17.

The reason the "early" removal of the attribute worked is because at that point the window isn't fullscreen yet, and so mHideChrome is still false.

Just fixing the widget code to update the margins when switching mHideChrome back to false should be enough to fix this, without any browser frontend fixes, so I'm moving this bug accordingly. I'm also making this reflect reality by taking.

Comment 26

[:Gijs (out for now; he/him)]

Attachment: dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=727a51283b9d

Comment 27

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #24)
> Here's some alternative STR that mess up:
> 
> 0) ensure titlebar is disabled
> 1) go into fullscreen
> 2) toggle titlebar on
> 3) go out of fullscreen
> 
> Not sure if the cause is the same yet. :-\

This doesn't seem to be fixed, so presumably that's a different bug... :-(

Comment 28

[Jim Mathies [:jimm]]

Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

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

::: widget/windows/nsWindow.cpp
@@ +2299,5 @@
>  
> +  if (mHideChrome) {
> +    mFutureMarginsOnceChromeShows = margins;
> +    mFutureMarginsToUse = true;
> +    return NS_ERROR_INVALID_ARG;

Seems like this should be NS_OK since the call succeeds, but delays actually setting the margins?

Comment 29

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #28)
> Comment on attachment 8513495 [details] [diff] [review]
> dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,
> 
> Review of attachment 8513495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +2299,5 @@
> >  
> > +  if (mHideChrome) {
> > +    mFutureMarginsOnceChromeShows = margins;
> > +    mFutureMarginsToUse = true;
> > +    return NS_ERROR_INVALID_ARG;
> 
> Seems like this should be NS_OK since the call succeeds, but delays actually
> setting the margins?

Yes.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8689c2f2f7cb

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8689c2f2f7cb

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Gijs - This fix looks like it has stuck on m-c. 34 and 35 are marked as affected. Do you want to propose for uplift?

Comment 32

[:Gijs (out for now; he/him)]

Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

Approval Request Comment
[Feature/regressing bug #]: 
[User impact if declined]: odd behaviour of titlebar when coming out of fullscreen if browser started in fullscreen, if the titlebar is enabled
[Describe test coverage new/current, TBPL]: nope :-(
[Risks and why]: low to medium. The risk is different regressions relating to tracking window modes and/or what part of the window needs to be non-client-margin'd (ie where we think the titlebar is/isn't). Considering the patch, I would say that risk is quite low, but if there is something we've not thought about, the impact might be non-trivial. OTOH, we still have beta baking time left, and this is an easy, small change to uplift and therefore also to back out.
[String/UUID change made/needed]: nope

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

Beta+
Aurora+

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f51cca1cda64
https://hg.mozilla.org/releases/mozilla-beta/rev/f6b893ef9186

Comment 35

[Andrei Vaida [:avaida]]

I was able to reproduce the bug on Windows 7 64-bit (Basic theme) using Nightly 35.0a1 (2014-09-04).

This is now verified fixed on:
 * Firefox 34 Beta 8 (build1 / 20141110195804),
 * Aurora 35.0a2 (2014-11-11),
 * Nightly 36.0a1 (2014-11-11),
using Windows 7 64-bit and Windows 8.1 64-bit.

I noticed a few inconsistencies/potential issues on *latest nightly with e10s* though, not sure if they're related to this or have separate bugs on file (couldn't find any):
1. [Win7] With aero, the tab strip displays a black gradient as background after resizing the window. 
    * screenshot: http://i.imgur.com/C6oIlfd.png
    * note: perhaps this is similar to Bug 951646
2. [Win7, Win8.1] The title bar cannot be enabled while the browser is in full screen.
    * str: enter full screen by pressing F11 → click menu → Customize → click Title Bar from the bottom of the palette → click Exit Customize
3. [Win8.1] The window is rendered black after switching a few times between full screen and windowed
    * screenshot: http://i.imgur.com/pa2CZ5c.png 
    * note:  I was unable to determine exact STR for this bug, nor reproduce it more than once

Gijs, what's your take on these?

Comment 36

[:Gijs (out for now; he/him)]

(In reply to Andrei Vaida, QA [:avaida] from comment #35)
> I was able to reproduce the bug on Windows 7 64-bit (Basic theme) using
> Nightly 35.0a1 (2014-09-04).
> 
> This is now verified fixed on:
>  * Firefox 34 Beta 8 (build1 / 20141110195804),
>  * Aurora 35.0a2 (2014-11-11),
>  * Nightly 36.0a1 (2014-11-11),
> using Windows 7 64-bit and Windows 8.1 64-bit.
> 
> I noticed a few inconsistencies/potential issues on *latest nightly with
> e10s* though, not sure if they're related to this or have separate bugs on
> file (couldn't find any):
> 1. [Win7] With aero, the tab strip displays a black gradient as background
> after resizing the window. 
>     * screenshot: http://i.imgur.com/C6oIlfd.png
>     * note: perhaps this is similar to Bug 951646

This deserves its own bug, yes. Indeed potentially related to bug 951646. Please file a bug with detailed STR, including the about:support details of the machine(s) on which you can repro. Probably belongs in Core::Graphics.

> 2. [Win7, Win8.1] The title bar cannot be enabled while the browser is in
> full screen.
>     * str: enter full screen by pressing F11 → click menu → Customize →
> click Title Bar from the bottom of the palette → click Exit Customize

There's not meant to be a titlebar in fullscreen, if that's what you mean. Never. As noted in comment #24, this does break once you go back to normal mode... :-\

This definitely deserves its own bug. It'd be great if you could file that.

> 3. [Win8.1] The window is rendered black after switching a few times between
> full screen and windowed
>     * screenshot: http://i.imgur.com/pa2CZ5c.png 
>     * note:  I was unable to determine exact STR for this bug, nor reproduce
> it more than once

Wat. That seems like a separate graphics bug... :-(

I would be very surprised if any of the graphics issues were related to the one fixed here, though - so they don't need to block this one. (2) is probably related in some way, but was the same before this fix, so we didn't regress that.

Comment 37

[Andrei Vaida [:avaida]]

Thanks for the feedback, Gijs. Just to wrap things up here, I've filed Bug 1098204 for Issue#1, Bug 1098282 for Issue#2 and Bug 1098294 for Issue#3.

Comment 38

[[e-mail]]

Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

>+  // Indicates we need to apply margins once toggling chrome into showing:
>+  bool                  mFutureMarginsToUse;
This variable is not properly initialised. Things seem to work OK if you're drawing in the title bar, because you set it to true when you enter full screen, but if you're not, then you're out of luck.

Comment 39

[[e-mail]]

(In reply to comment #38)
> (From update of attachment 8513495 [details] [diff] [review])
> >+  // Indicates we need to apply margins once toggling chrome into showing:
> >+  bool                  mFutureMarginsToUse;
> This variable is not properly initialised. Things seem to work OK if you're
> drawing in the title bar, because you set it to true when you enter full
> screen, but if you're not, then you're out of luck.

Filed bug 1102855.