1186890 - [e10s] Navigation doesn't exit fullscreen correctly

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Vasilica Tamas, QA [:vtamas]]

Reproducible on: Firefox 42.0a1 with e10s enabled across all platforms

STR
1. Start Firefox.
2. Open a PDF file (e.g. http://cp.literature.agilent.com/litweb/pdf/5989-8139EN.pdf )
3. Click the "Presentation Mode" button to enter in full screen mode.
4. Right click to open the context menu.
5. Click on “Reload current page” button.

ER
The Presentation mode is cancelled when a PDF document is refreshed in fullscreen mode in a non-e10s window. 

AR
- The PDF document remains in presentation mode
- The background colour changes in gray
- The pdf toolbar is displayed

Additional notes:
- This issue is reproducible on Firefox 42.0a1 (2015-07-22) under Windows 8.1 32-bit, Mac OS X 10.10.4 and Ubuntu 13.10 64-bit.
- This issue is e10s specific.


Regression window:
(m-c)
Last good revision: c6bbf8f1b02b (2015-05-26)
First bad revision: ff2e07228041 (2015-05-27)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c6bbf8f1b02b&tocha
nge=ff2e07228041

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

It is expected to fully exit fullscreen when navigation happens, which includes refreshing.

This is a Core bug, not a Firefox one.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

TBH, I don't know how everything even works before this regression... But this patch should be useful anyway.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Ping :dao. Did you somehow ignore the patch here?

Comment 5

[Justin Dolske [:Dolske]]

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

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

::: browser/base/content/tab-content.js
@@ +650,5 @@
> +          // If we receive any fullscreen change event, and find we are
> +          // actually not in fullscreen, also ask the parent to exit to
> +          // ensure that the parent always exits fullscreen when we do.
> +          sendAsyncMessage("DOMFullscreen:Exit");
> +        }

Hmm, so, what's actually happening when reproducing this?

Presumably we're not _entering_ fullscreen. And so I'd have expected MozDOMFullscreen:Exit to fire before the :Exited, where we should have done the same sendAsyncMessage().

So is that event not firing? Or is its sendAsyncMessage() not actually exiting fullscreen? Seems like a proper fix would be to address whichever of those is happening, instead of just bandaiding over that bug?

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Justin Dolske [:Dolske] from comment #5)
> Comment on attachment 8639220 [details] [diff] [review]
> patch
> 
> Review of attachment 8639220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tab-content.js
> @@ +650,5 @@
> > +          // If we receive any fullscreen change event, and find we are
> > +          // actually not in fullscreen, also ask the parent to exit to
> > +          // ensure that the parent always exits fullscreen when we do.
> > +          sendAsyncMessage("DOMFullscreen:Exit");
> > +        }
> 
> Hmm, so, what's actually happening when reproducing this?

Nothing happens. The parent process isn't aware that the content process has exited fullscreen.

> Presumably we're not _entering_ fullscreen. And so I'd have expected
> MozDOMFullscreen:Exit to fire before the :Exited, where we should have done
> the same sendAsyncMessage().

MozDOMFullscreen:Exit is designed to be fired if the page itself asks to exit fullscreen. Probably a name like :RequestExit would be better for that event.

When the content accidentally exit fullscreen state because of navigation or fullscreen element removal, the parent process won't be notified. It seems I ignored these situations when I change the whole logic, and that's why I said I don't understand how everything worked before the regression.

> So is that event not firing? Or is its sendAsyncMessage() not actually
> exiting fullscreen? Seems like a proper fix would be to address whichever of
> those is happening, instead of just bandaiding over that bug?

No, that event is not designed to fire at this case. When the fullscreen change has happened, only :Exited should fire.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)

> > Presumably we're not _entering_ fullscreen. And so I'd have expected
> > MozDOMFullscreen:Exit to fire before the :Exited, where we should have done
> > the same sendAsyncMessage().
> 
> MozDOMFullscreen:Exit is designed to be fired if the page itself asks to
> exit fullscreen. Probably a name like :RequestExit would be better for that
> event.

Ah, indeed. That's confusing. Can you maybe just go ahead and rename that? (These events are all chrome-only, right?)

In fact, is there even a reason to have two different Exit/Exited events? Why not just have a single, common event?

Finally, I'll note that it would be most helpful to have a 1-2 line comment for each case in receiveMessage / handleEvent explaining what each is for. Ditto for the related code in browser-fullScreen.js. It's hard to understand each, as well as the overall control flow.

> that's why I
> said I don't understand how everything worked before the regression.

Would be worth understanding how this worked before! Maybe we're missing something else.


Oh, and a test would be great here.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Justin Dolske [:Dolske] from comment #7)
> Ah, indeed. That's confusing. Can you maybe just go ahead and rename that?
> (These events are all chrome-only, right?)

Yes, they are all chrome-only. I'll try to rename them.

> In fact, is there even a reason to have two different Exit/Exited events?
> Why not just have a single, common event?

Becasue we want to change the window size before toggling the document state, so that we can:
1. have the transition for fullscreen change,
2. have a stable viewport size when we notify the content we finish the fullscreen change

Comment 9

[Dão Gottwald [:dao]]

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

Seems like dolske dived into this and is prepared to finish the review. If I misunderstood and those were just drive-by comments, feel free to redirect the request to me.

FWIW, I had the same feeling that this is a band-aid and that a "proper" fix might be more appropriate.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Justin Dolske [:Dolske] from comment #7)
> Oh, and a test would be great here.

I believe we have had tests for this... just disabled on e10s...

I'll try to enable Fullscreen API tests on e10s later in bug 1191597.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Justin Dolske [:Dolske] from comment #7)
> > that's why I
> > said I don't understand how everything worked before the regression.
> 
> Would be worth understanding how this worked before! Maybe we're missing
> something else.

OK, it is probably not a regression at all. I tested the Nightly on 2015-05-25 (the day before "Last good revision" in comment 0), but navigation doesn't exit fullscreen either. The reporter thought it worked probably because the PDF viewer started using a different approach at that day. This bug exists far before that.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Probably I should create a wiki page to describe the whole flow.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

I've created a wiki page to briefly describe the basic control flow of fullscreen stuffs: https://wiki.mozilla.org/Gecko:Fullscreen_API . Hopefully it could make things easier to understand.

The section E10S / Exiting Fullscreen Unexpectedly describes the steps I proposed in the patch here.

Based on this, Dolske, could you continue review this patch?

Comment 14

[Justin Dolske [:Dolske]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #13)
> I've created a wiki page to briefly describe the basic control flow of
> fullscreen stuffs: https://wiki.mozilla.org/Gecko:Fullscreen_API . Hopefully
> it could make things easier to understand.

Thanks for documenting this and explaining here. Makes a lot more sense to me.

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/dee3e26cc1c0f4deeb49eaa06b03a3c06ac4927f
changeset:  dee3e26cc1c0f4deeb49eaa06b03a3c06ac4927f
user:       Xidorn Quan <[e-mail]>
date:       Fri Aug 07 13:38:10 2015 +1000
description:
Bug 1186890 - Ensure parent always know when the child exit fullscreen. r=Dolske

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/mozilla-central/rev/dee3e26cc1c0