1282334 - Exiting fullscreen (and sometime entering fullscreen) takes a long time when the page is static

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

I have a testing page which is completely empty, and does nothing other than request fullscreen on given event. It almost always takes a long time for it to exit fullscreen.

Given the page is empty, the layout or js on the page should not be the factor. We need to figure out what happens there. (Could Firefox UI trigger some heavy reflow during fullscreen?)

(I tried Gecko Profiler, but unfortunately it doesn't work at the moment. I will try later.)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

ni? myself to visit this again later.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Now I have a profile report: https://cleopatra.io/#report=a510abb926f017d301647918067298f1e9d2131a

In this report, lots of "Fullscreen toggle start" end up with "Fullscreen toggle timeout", which looks weird, because there is no CPU usage at all. It probably indicates that we fail to send "DOMFullscreen:Painted" message in some cases, which may indicate that either MozAfterPaint event is not reliable or transitionId is not reliable.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

There are two issues:
1. for e10s, it seems sometimes paint is not triggered at all, and thus the page is kept unchanged after the fullscreen change, and consequently MozAfterPaint is never changed.
2. for non-e10s, it seems bug 1264091 actually regressed non-e10s behavior that static page will never have :Painted message sent because the transation id may have been changed when DOMFullscreen:CleanUp is called. Consequently, toggle timeout would always be triggered.

The first issue may still need further investigation.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> There are two issues:
> 1. for e10s, it seems sometimes paint is not triggered at all, and thus the
> page is kept unchanged after the fullscreen change, and consequently
> MozAfterPaint is never changed.

is never *triggered.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

The first issue is a regression by bug 1267568... It indicates that refresh driver is sometimes not triggered regularly for static pages.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1282334 part 1 - Not check transaction id for non-e10s.

Before this patch, there are two issues for non-e10s window:

* The initial entering fullscreen would hit timeout because we do not
  record lastTransitionId for entering for non-e10s, and a number is
  never larger than undefined, so MozAfterPaint never triggers the
  message it should trigger.

* Every exiting fullscreen may hit timeout because when we record the
  transition id in DOMFullscreen:CleanUp, the last paint for fullscreen
  change may have completed, and consequently we won't receive any new
  MozAfterPaint with a larger transition if the page is static.

Review commit: https://reviewboard.mozilla.org/r/61170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61170/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1282334 part 2 - Ensure scheduling flush for fullscreen change.

The refresh driver may not be triggered regularly if the page is static.
In that case, we need to ensure a flush is scheduled for fullscreen
change, otherwise the page may get stuck.

Review commit: https://reviewboard.mozilla.org/r/61172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61172/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1282334 followup - Remove pointless DOMFullscreenHandler._fullscreenDoc.

Review commit: https://reviewboard.mozilla.org/r/61174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61174/

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8766166 [details]
Bug 1282334 part 1 - Not check transaction id for non-e10s.

I'd prefer if you checked !this._lastTransactionId || ... in the MozAfterPaint handler.

The other change could also use some documentation.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8766167 [details]
Bug 1282334 part 2 - Ensure scheduling flush for fullscreen change.

https://reviewboard.mozilla.org/r/61172/#review58116

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766166 [details]
Bug 1282334 part 1 - Not check transaction id for non-e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61170/diff/1-2/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766167 [details]
Bug 1282334 part 2 - Ensure scheduling flush for fullscreen change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61172/diff/1-2/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766168 [details]
Bug 1282334 followup - Remove pointless DOMFullscreenHandler._fullscreenDoc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61174/diff/1-2/

Comment 14

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #9)
> The other change could also use some documentation.

This still stands.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766166 [details]
Bug 1282334 part 1 - Not check transaction id for non-e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61170/diff/2-3/

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766167 [details]
Bug 1282334 part 2 - Ensure scheduling flush for fullscreen change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61172/diff/2-3/

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766168 [details]
Bug 1282334 followup - Remove pointless DOMFullscreenHandler._fullscreenDoc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61174/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/300bbe55198b
part 1 - Not check transaction id for non-e10s. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d69294a888e
part 2 - Ensure scheduling flush for fullscreen change. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cddfe07a253
followup - Remove pointless DOMFullscreenHandler._fullscreenDoc. r=dao

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766166 [details]
Bug 1282334 part 1 - Not check transaction id for non-e10s.

Approval Request Comment
[Feature/regressing bug #]: bug 1264091
[User impact if declined]: user may see long delay for fullscreen change for static pages (with no animation etc.) on non-e10s
[Describe test coverage new/current, TreeHerder]: existing test should be enough
[Risks and why]: low risk, this just ensures that the mechanism we added for e10s wouldn't be triggered unexpectedly for non-e10s
[String/UUID change made/needed]: n/a

Note: this request is *only* for this patch.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8766167 [details]
Bug 1282334 part 2 - Ensure scheduling flush for fullscreen change.

Approval Request Comment
[Feature/regressing bug #]: bug 1267568
[User impact if declined]: user may see long delay for fullscreen change for static pages (with no animations etc.) on e10s
[Describe test coverage new/current, TreeHerder]: existing tests should be enough
[Risks and why]: low risk for breakage, might be a little risky for fullscreen change performance, but I think telemetry would catch that regression if it really affects.
[String/UUID change made/needed]: n/a

Note: this request is *only* for this patch.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/300bbe55198b
https://hg.mozilla.org/mozilla-central/rev/0d69294a888e
https://hg.mozilla.org/mozilla-central/rev/4cddfe07a253

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8766166 [details]
Bug 1282334 part 1 - Not check transaction id for non-e10s.

OK, let's do it as we are going to ship e10s
I am not approving the second patch on purpose not to confuse the sheriff.
should be in 48 beta 5

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cea38bcafb6a

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9274f2d6136f

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8766167 [details]
Bug 1282334 part 2 - Ensure scheduling flush for fullscreen change.

OK to uplift part 2 to aurora so fullscreen changes will work in this case.

Comment 26

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Hello! I tried to reproduce this issue on Windows 10 x64 using 50.0a1 (2016-06-26) build, but I didn't managed it. Can you please give me more details about reproducing this or a sample of page that reproduces it? I used static pages samples from https://nellen.co.za/static-websites and https://jekyllrb.com/docs/sites/, but no luck.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

You can try this file. Clicking anywhere in the page would enter fullscreen.

Comment 28

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #27)
> Created attachment 8771259 [details]
> testcase
> 
> You can try this file. Clicking anywhere in the page would enter fullscreen.

Thank you for your clarification! 
I managed to reproduce this bug on 50.0a1 (2016-06-26). Also, I verified it on 
- latest Nightly 50.0a1 (2016-07-14)
- latest Aurora 49.0a2 (2016-07-14)
- 48.0b7 build1 (20160711002726) 
using 
- Windows 10 x64
- Windows 7 x64
- Ubuntu 14.04 x86 
- Mac OS X 10.11.1 
Latest Nightly and Beta builds are verified fixed across above mentioned OSs. The issues is still reproducible on latest Aurora 49.0a2 (2016-07-14) using Windows 10 x64 and Windows 7 x64, only with e10s on (click to enter full screen mode -> hit ESC/F11 key to exit full screen mode -> the full screen toggle is delayed for a few seconds, while the whole screen turns black). For Mac and Ubuntu, latest Aurora build is verified fixed.
I will set the corresponding flags.