630276 - Neither exiting full-screen video nor closing full-screen window restore menubar/dock

Status: RESOLVED FIXED

Description

[Joe Drew (not getting mail)]

If you full-screen a video, then exit full-screen by pushing escape, the OS X menubar and dock remain hidden. Switching windows from Firefox to another application will restore them; getting them back permanently involves switching windows "several times" (haven't quite figured that part out yet, though the dock and menubar do eventually come back).

Comment 1

[Chris Pearce [:cpearce (GMT+13)]]

Does this happen on trunk too?

Comment 2

[Joe Drew (not getting mail)]

Sorry I didn't specify - this *is* trunk.

Comment 3

[Joe Drew (not getting mail)]

This is probably a softblocker.

Comment 4

[Mehdi Mulani [:mmm] (I don't check this)]

issue also seems to happen if you close a window while in full screen mode.

Comment 5

[Mehdi Mulani [:mmm] (I don't check this)]

taking it over for now.

tested it on nightlies, looks like it breaks in 2011-01-19 but is good in 2011-01-18, this is the corresponding window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b521854c006d&tochange=809aded51aad

Comment 6

[Mehdi Mulani [:mmm] (I don't check this)]

It looks like http://hg.mozilla.org/mozilla-central/rev/acb48b922e91 causes the problem, not sure why though. CC'ing Olli to see if he can shed some light on the problem.

Comment 7

[Frank Yan (:fryn)]

Gavin, why did this become a non-blocking Windows bug?

Comment 8

[Olli Pettay [:smaug]]

huh, I have no idea how Bug 625311 could have caused that problem.
We really can't back that out.

How does fullscreen video work? Do we create a new widget for it?
What keeps that widget alive? Are we not calling nsBaseWidget::OnDestroy() ?

Comment 9

[Olli Pettay [:smaug]]

(In reply to comment #8)
> huh, I have no idea how Bug 625311 could have caused that problem.
> We really can't back that out.
...or we'd need to fix the issue in some other way.

Comment 10

[Olli Pettay [:smaug]]

Hmm, is it possible that something in fullscreen video UI relies on widget to be
deleted almost immediately after hiding the window?

Comment 11

[Mehdi Mulani [:mmm] (I don't check this)]

From debugging this morning, it seems like something holds a reference to the
nsThebesDeviceContext which isn't released normally but is later GC'ed and
destroyed. After this, the widget is finally destroyed and consequently the
menubar and dock are restored.

Instead of solving that pointer problem, we could move the |if (mFullscreen)|
block from ~nsCocoaWindow to nsCocoaWindow::Destroy. Not sure how risky that
would be though.

CC'ing Josh because it seems like he works on that section.

Comment 12

[Markus Stange [:mstange]]

Moving it to Destroy sounds like a good idea.

Comment 13

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v1, moves isFullscreen check to Destroy.

Patch works in both cases, when closing a full screen window and when closing a full screen video. (In the second case, the video and tab are still around)

Running this through on try to make sure it doesn't break anything.

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 511203 [details] [diff] [review]
Patch v1, moves isFullscreen check to Destroy.

Looks good, thanks!

At some point we should make sure that we also handle the window being hidden and re-shown during its lifetime, and only hide the OS chrome while the fullscreen window is visible, but that can wait. Fullscreenable windows are currently guaranteed to be destroyed right after being hidden, so this will work for now.

Comment 15

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #14)
> Comment on attachment 511203 [details] [diff] [review]
> Patch v1, moves isFullscreen check to Destroy.
> 
> Looks good, thanks!
> 
> At some point we should make sure that we also handle the window being hidden
> and re-shown during its lifetime, and only hide the OS chrome while the
> fullscreen window is visible, but that can wait. Fullscreenable windows are
> currently guaranteed to be destroyed right after being hidden, so this will
> work for now.

Sweet!

Is your review enough to check this in, or should I wait for Josh to give it a look?

Comment 16

[Markus Stange [:mstange]]

Better wait for Josh to give it another rubber stamp; he's the overlord here.

Comment 17

[Josh Aas]

Comment on attachment 511203 [details] [diff] [review]
Patch v1, moves isFullscreen check to Destroy.

Thanks!

Comment 18

[Mehdi Mulani [:mmm] (I don't check this)]

http://hg.mozilla.org/mozilla-central/rev/9e27e7d04699