1243014 - [e10s] DOM fullscreen transition sometimes finishes before the video on Youtube changes its size, and that ruins the point of transition

Status: NEW

(Firefox :: General, defect, P3)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160121030208
STR:
1. Open https://www.youtube.com/watch?v=sGbxmsDFVnE,
   Press "K" or click pause button when something bright will be visible on the video
2. Click fullscreen button within the video
3.A) Press Escape when the screen becomes completely black, but the transition isn't finished yet
3.B) Press Escape When transition is finished
4. Repeat Steps 2-3 if you haven't noticed the bug

Result (watch screencast in comment 1):       
 Transition from DOM fullscreen to normal window finishes before the video changes its size.
 In scenario (A) this is ~90% reproducible for me, in scenario (B) it's ~10% reproducible

Expectations: 
 Video shouldn't blink: the last part of DOM fullscreen transition (from completely black screen
 to visible page content) should finish after the video changes its size and position on the page

Comment 1

[arni2033]

Screencast:
> https://dl.dropboxusercontent.com/s/85ymhfh2g157uw2/screencast%201%20-%20%5Be10s%5D%20Fullscreen%20transition.webm?dl=0

Screenshots:
> https://dl.dropboxusercontent.com/s/hlbv1nhqbbpczx0/2015.12.19%2002-16-21.png?dl=0
> https://dl.dropboxusercontent.com/s/2hcmgulvrl2ayut/2015.12.19%2002-11-19.png?dl=0
> https://dl.dropboxusercontent.com/s/lilm05cs24av4jo/2015.12.19%2002-06-54.png?dl=0

Comment 2

[arni2033]

I'm not sure what "tracking-e10s: -" means exactly, but I'm quite confident in the results
of my testing. This is e10s-only issue. Also, I forgot to mention Step 0.
> 0. Open 5 background tabs with http://www.rp-online.de/, wait until each tab is loaded
That's required to emulate real-life browsing and make the bug easy to reproduce.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

I'd think this is because of bug 1240978 which shortens the default timeout for the black screen from 1s to 0.5s, by which I try to improve the user experience when layout doesn't response in time.

Could you try testing different value of "full-screen-api.transition.timeout" to see what is the optimal timeout for you?

This is more reproducible on e10s since on e10s we have a longer path, within which there are inter-process communication involves. I did observe an issue that the IPC sometimes takes a significant time (which could be ~100ms in some cases). I'll investigate that a bit and see whether it is improvable.

Comment 4

[arni2033]

First of all, my original screenshots were taken at 2015.12.19 02-06-54 (see titles), so this wasn't related to 1240978. Also, the bug was reported for ID 20160121030208, i.e. before bug 1240978

But the issue is presented on the latest Nightly (ID 20160126030244).

e10s:     I set "full-screen-api.transition.timeout" to 5000, then to 50000, and I still see the bug
non-e10s: Unaffected. I see no bug. The screen stays black as long as it's needed for video to restore its position on the page. I just had that black screen for 3s with the pref set to 500.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

If this still there with a big timeout value, it means paint notification is received before things get ready...

Well, I guess that could happen if you enter then exit fullscreen in a very short time, in which case, the paint notification for entering fullscreen could arrive after exit is triggered.

Comment 6

[arni2033]

> if you enter then exit fullscreen in a very short time
This is not what I do. I click fullscreen button, then wait (about 1 second) for screen to become completely black, then press Escape. This is NOT "click fullscreen button, then immediately Escape".

Also, as I said in comment 0, in 10% of cases I see it when I press Escape only after transition is finished. In addition, I sometimes see the "opposite" effect: after Step 2 I do nothing and just observe. Sometimes transition from [100% black] to [no black] already starts, but the video is still displayed at its old place, as well as the page itself. Then the video jumps to "fullscreen mode"
> screenshot:   https://dl.dropboxusercontent.com/s/vuj9ivdplu87koa/2016.01.27%2004-39-50.png?dl=0

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to arni2033 from comment #6)
> > if you enter then exit fullscreen in a very short time
> This is not what I do. I click fullscreen button, then wait (about 1 second)
> for screen to become completely black, then press Escape. This is NOT "click
> fullscreen button, then immediately Escape".

Yeah, I meant to say what you were describing here. Pressing Escape before completely black screen doesn't actually have any effect. In that case it is possible that the paint message for entering fullscreen also wakes up transition for exiting fullscreen.

> Also, as I said in comment 0, in 10% of cases I see it when I press Escape
> only after transition is finished. In addition, I sometimes see the
> "opposite" effect: after Step 2 I do nothing and just observe. Sometimes
> transition from [100% black] to [no black] already starts, but the video is
> still displayed at its old place, as well as the page itself. Then the video
> jumps to "fullscreen mode"
> > screenshot:   https://dl.dropboxusercontent.com/s/vuj9ivdplu87koa/2016.01.27%2004-39-50.png?dl=0

That's the main issue, where the underlying change takes time longer than the timeout. And a shorter timeout could make this issue more likely to be observed. And that happening only with e10s indicates there is some performance issue with IPC.

If you set full-screen-api.transition.timeout to zero, you would reliably see this.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Reread the description of this bug, I guess this should have been fixed by bug 1264091.

arni2033, could you try the current nightly and see if this still happens?

Comment 9

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160426044609
This bug is still reproducible; please mark same bugs as dependent for this one. Anyway, why people are so obsessed about Yahoo!? I can reproduce it with a video opened in tab  (OOT: I don't know
why this bug was abandoned and bug 1264091 fixed - probably I need to add bogus "QA" to my nickname)

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

(In reply to arni2033 from comment #9)
> >>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160426044609
> This bug is still reproducible; please mark same bugs as dependent for this
> one. Anyway, why people are so obsessed about Yahoo!? I can reproduce it
> with a video opened in tab  (OOT: I don't know
> why this bug was abandoned and bug 1264091 fixed - probably I need to add
> bogus "QA" to my nickname)

This bug is not abandoned. Bug 1264091 is fixed just because there happened to be a dev-platform post [1] introduced a new technique which would help improving e10s fullscreen just several days before I saw that bug, so I decide to use that bug to land this improvement. And it seems people don't think that bug itself has been fully fixed, thus bug 1266799.

[1] https://lists.mozilla.org/pipermail/dev-platform/2016-April/014385.html

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

The issue described in comment 0 via (A) isn't really something worth fixing. It seems to happen only when exiting fullscreen starts before entering fullscreen ends, which no one does in general.

If it still happens via (B), that might be we are hitting the timeout somehow. If extending transition.timeout doesn't stop (B)'s happening, then it would be some other bug need to be investigated.


For completeness, I'd like to leave a note about what's happening for scenario (A):

In normal cases, the following happens in sequence:
1. fullscreen transition start
2. start changing the state > notify the content process
3. end changing the state < notified by the content process
4. fullscreen transition end

In scenario (A), two transitions run concurrently, which mess up things:
1. fullscreen transition start (for entering)
2. fullscreen transition start (for exiting)
3. start changing the state (for entering) > notify the content process
4. start changing the state (for exiting) > notify the content process
5. end changing the state (for entering and exiting) < notified by the content process
  * because we cannot distinguish between notification for entering and exiting, this makes the chrome process think both have been finished
6. fullscreen transition end (for entering and exiting)
  * at this point, the content process may not have started processing the exiting message at all, thus the incorrect document state

It seems to me fixing scenario (A) is non-trivial, and given it only happens in unusual cases, I think it isn't worth fixing.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #11)
> If it still happens via (B), that might be we are hitting the timeout
> somehow. If extending transition.timeout doesn't stop (B)'s happening, then
> it would be some other bug need to be investigated.

Arni, any chance you could test str B here for us and see if it is fixed, or if an extended timeout helps?

Comment 13

[arni2033]

(In reply to Jim Mathies [:jimm] from comment #12)
> Arni, any chance you could test str B here for us and see if it is fixed, or
> if an extended timeout helps?
I can do that in ~2 days once you explicitly specify what pref in about:config I should change and how

BTW (since this comment should be marked as "spam" anyway):
I personally don't like new fullscreen prefs, as even if a pref's purpose is clear, it's still unreliable. I got that impression from reading a russian forum and from my own testing. So I can also
test some of the prefs which intended to be reliable and present results in any form (email,bug,etc).

Comment 14

[Jim Mathies [:jimm]]

(In reply to arni2033 from comment #13)
> (In reply to Jim Mathies [:jimm] from comment #12)
> > Arni, any chance you could test str B here for us and see if it is fixed, or
> > if an extended timeout helps?
> I can do that in ~2 days once you explicitly specify what pref in
> about:config I should change and how
> 
> BTW (since this comment should be marked as "spam" anyway):
> I personally don't like new fullscreen prefs, as even if a pref's purpose is
> clear, it's still unreliable. I got that impression from reading a russian
> forum and from my own testing. So I can also
> test some of the prefs which intended to be reliable and present results in
> any form (email,bug,etc).

The pref is 'full-screen-api.transition.timeout' which should be set to 500. You can try setting that to say 1000, or 1500, and see if scenario B in your original comment here improves. Thanks!

Comment 15

[arni2033]

1) The results of the testing depend on how busy are content precess and/or parent process. I usually
   tested this with 1-2 tabs loading in background, and that doesn't provide reliable CPU consumption
  (e.g. strictly equals to 30%). This also explains why everybody loved Yahoo! example - that's
   because that page itself is very heavy and consumes ~20% CPU for 10+ seconds on my PC.
2) Results:
   The pref is set to:      500    1500    50000
   Percentage of bug:        80%     10%       0%
I think these results are useless (see (1)), and you need to create some tests to measure how IPC time
depends on CPU consumption by each process (those tests are quite useful anyway) and decide on the real-life values (or grab some telemetry data to see how much time IPC takes in real life) and then,
probably, change the value of the pref (which actually works now). That's how I see the plan.
3) Note that with pref set to 1500+ scenario (A) becames way more possible, because if browser can't
   switch to fullscreen mode in a 3 seconds, I as a user will try to immediately quit fullscreen and
   watch the rest of the video in a normal mode.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

The IPC time doesn't really depend on CPU consumption heavily. The main issue here is that the event loop of the main thread could be blocked by some other part and thus the IPC message may not be handled in time...