Closed Bug 1282334 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox48 --- verified
firefox49 --- fixed
firefox50 --- verified

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.)
ni? myself to visit this again later.
Flags: needinfo?(bugzilla)
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.
Flags: needinfo?(bugzilla)
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.
(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.
The first issue is a regression by bug 1267568... It indicates that refresh driver is sometimes not triggered regularly for static pages.
Summary: Exiting fullscreen (and sometime entering fullscreen) takes a long time even if the page is very simple → Exiting fullscreen (and sometime entering fullscreen) takes a long time when the page is static
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/
Attachment #8766166 - Flags: review?(dao+bmo)
Attachment #8766167 - Flags: review?(bugs)
Attachment #8766168 - Flags: review?(dao+bmo)
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/
Attachment #8766168 - Flags: review?(dao+bmo) → review+
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.
Attachment #8766166 - Flags: review?(dao+bmo)
Attachment #8766167 - Flags: review?(bugs) → review+
Comment on attachment 8766167 [details]
Bug 1282334 part 2 - Ensure scheduling flush for fullscreen change.

https://reviewboard.mozilla.org/r/61172/#review58116
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/
Attachment #8766166 - Flags: review?(dao+bmo)
Attachment #8766168 - Flags: review+ → review?(dao+bmo)
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 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/
Attachment #8766168 - Flags: review?(dao+bmo) → review+
Assignee: nobody → xidorn+moz
(In reply to Dão Gottwald [:dao] from comment #9)
> The other change could also use some documentation.

This still stands.
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/
Attachment #8766168 - Flags: review+ → review?(dao+bmo)
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 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/
Attachment #8766166 - Flags: review?(dao+bmo) → review+
Attachment #8766168 - Flags: review?(dao+bmo) → review+
Pushed by xquan@mozilla.com:
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 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.
Attachment #8766166 - Flags: approval-mozilla-beta?
Attachment #8766166 - Flags: approval-mozilla-aurora?
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.
Attachment #8766167 - Flags: approval-mozilla-aurora?
Flags: qe-verify+
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
Attachment #8766166 - Flags: approval-mozilla-beta?
Attachment #8766166 - Flags: approval-mozilla-beta+
Attachment #8766166 - Flags: approval-mozilla-aurora?
Attachment #8766166 - Flags: approval-mozilla-aurora+
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.
Attachment #8766167 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(xidorn+moz)
Attached file testcase
You can try this file. Clicking anywhere in the page would enter fullscreen.
Flags: needinfo?(xidorn+moz)
(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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.