Intermittent dom/html/test/test_fullscreen-api.html | [scrollbar] Should not have vertical scrollbar when [object HTMLDivElement] is in fullscreen - got 500, expected 1600

NEW
Unassigned

Status

()

Core
DOM
P5
normal
8 months ago
4 months ago

People

(Reporter: Treeherder Bug Filer, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {intermittent-failure, leave-open})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [stockwell disabled])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 months ago
treeherder
Filed by: cbook [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=86630652&repo=autoland

https://queue.taskcluster.net/v1/task/U0Vxj8PVQ4qEtt8jI_KMNw/runs/0/artifacts/public/logs/live_backing.log

Comment 1

8 months ago
22 failures in 141 pushes (0.156 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 16
* mozilla-inbound: 6

Platform breakdown:
* linux64: 21
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875&startday=2017-03-27&endday=2017-03-27&tree=all
> <kanru> xidorn: I can reproduce locally. Sometimes it's the gVerticalScrollbarWidth not correct but sometimes it's the rect returned by getMeasureRect() not correct
> <kanru> xidorn: it feels like some kind of timing issue
> <xidorn> kanru: it seems to me both of them rely on getBoundingClientRect returns the right value
> <xidorn> kanru: so it feels like layout isn't triggered as expected when we call that method

I can reproduce quite easily. The fail is either

> [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen - got 1586, expected 1588

or

> [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen - got 486, expected 1586

In both cases the screen width is reported correctly so I don't think bug 1194751 is the root cause. bug 1194751 does change the timing so it might expose some existing issue.

The screen is 3200x1800 with ContentsScale=1, DefaultCssScale=2
Flags: needinfo?(xidorn+moz)
So it's on HTMLHtmlElement... which means when the window size gets changed, the document viewport size may not get changed at the same time.

Currently, the addFullscreenChangeContinuation in file_fullscreen-utils.js [1] checks window.inner{Width,Height} to know when we enters fullscreen on Linux. If window.inner{Width,Height} changes, but the viewport size layout system uses doesn't change, this can happen.

Any idea if that could happen from your change?


[1] https://dxr.mozilla.org/mozilla-central/source/dom/html/test/file_fullscreen-utils.js
Flags: needinfo?(xidorn+moz)
Blocks: 1215369
See Also: → bug 1215369
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> So it's on HTMLHtmlElement... which means when the window size gets changed,
> the document viewport size may not get changed at the same time.

This doesn't explain the why gVerticalScrollbarWidth can be different from run to run. It was calculated before we enter fullscreen.

Updated

8 months ago
See Also: → bug 1332040

Comment 5

8 months ago
27 failures in 165 pushes (0.164 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 15
* mozilla-inbound: 10
* mozilla-central: 2

Platform breakdown:
* linux64: 24
* linux32: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875&startday=2017-03-28&endday=2017-03-28&tree=all

Comment 6

8 months ago
20 failures in 188 pushes (0.106 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 11
* mozilla-inbound: 9

Platform breakdown:
* linux64: 18
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875&startday=2017-03-29&endday=2017-03-29&tree=all
This is very frequent- sounds like we are already looking into this- I will follow up early next week if this isn't fixed.
Whiteboard: [stockwell needswork]
(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #4)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> > So it's on HTMLHtmlElement... which means when the window size gets changed,
> > the document viewport size may not get changed at the same time.
> 
> This doesn't explain the why gVerticalScrollbarWidth can be different from
> run to run. It was calculated before we enter fullscreen.

I can reproduce the gVerticalScrollbarWidth issue without my patches so we can ignore that and focus on why the div size didn't change at the same time.

Updated

8 months ago
See Also: → bug 1295815

Comment 9

8 months ago
18 failures in 123 pushes (0.146 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 10
* mozilla-inbound: 5
* mozilla-central: 2
* try: 1

Platform breakdown:
* linux64: 14
* linux32: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875&startday=2017-03-30&endday=2017-03-30&tree=all

Comment 10

8 months ago
23 failures in 146 pushes (0.158 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 14
* mozilla-inbound: 6
* try: 1
* mozilla-central: 1
* graphics: 1

Platform breakdown:
* linux64: 19
* linux32: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875&startday=2017-03-31&endday=2017-03-31&tree=all

Comment 11

8 months ago
144 failures in 845 pushes (0.17 failures/push) were associated with this bug in the last 7 days. 

This is the #7 most frequent failure this week. 

** This failure happened more than 75 times this week! Resolving this bug is a very high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 1 week, the affected test(s) may be disabled. **  

Repository breakdown:
* autoland: 83
* mozilla-inbound: 45
* mozilla-central: 13
* try: 2
* graphics: 1

Platform breakdown:
* linux64: 115
* linux32: 29

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875&startday=2017-03-27&endday=2017-04-02&tree=all
So sometimes before we query the bounds of gMeasureDiv, the child process receives a TabChild::RecvUpdateDimensions event with old dimensions from the parent process. The event is triggered "MozUpdateWindowPos" event listener in TabParent. The "MozUpdateWindowPos" event is sent by nsWebShellWindow::WindowMoved. On GTK that is triggered by a native configure event.

It looks like this event is interfering the fullscreen implementation. If I remove the event handler then the test failure is no longer reproducible.

Kats, do you know if this event is still needed?
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(bugmail)
To be honest, I'm not sure. I *think* it's still needed, because we need it to update the child-side window offset when the top-level window moves. But it's been a long time since I poked around in this code.

I looked at the test failure a little (using some recent logs linked to from orangefactor). In all the logs I saw I didn't notice any problem with gVerticalScrollbarWidth being miscalculated, it seems it's always 13. It sounds like you were seeing that in your local repros but I didn't see that in any of the logs I checked from TreeHerder failures.

It looks like the fullscreen is halfway done at the point the test fails. This is clear from the screenshot saved by the harness - the outer window has gone fullscreen but the content has not yet been resized to match. The screenshot matches the failure output (e.g. actual width 487, expected width 1187). To me it seems like this is just an artifact of the fullscreenchange event firing before the content process has finished resizing and so the test continuation runs before we want it to.

If you think removing MozWindowUpdatePos will solve this feel free to try it but I'm fairly certain it's still needed and you'll probably get other tests failing without it.
Flags: needinfo?(bugmail)
Looks like all failures happen on e10s. e10s fullscreen has some special code [1] which does some tricks around view manager to avoid unnecessary reflow. IIRC this doesn't work quite well on Linux, because GTK+ is quite arbitrary on the order of signal it sends.

Probably you can have you change the test file to dump those various info (window.inner{Width,Height}, div size) at different stages to see what is happening.

[1] https://dxr.mozilla.org/mozilla-central/rev/9aacfa8081b35bb8ae1a59ce3fd9d7aba57cfc7b/dom/base/nsDOMWindowUtils.cpp#3256-3324
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #14)
> Looks like all failures happen on e10s. e10s fullscreen has some special
> code [1] which does some tricks around view manager to avoid unnecessary
> reflow. IIRC this doesn't work quite well on Linux, because GTK+ is quite
> arbitrary on the order of signal it sends.
> 
> Probably you can have you change the test file to dump those various info
> (window.inner{Width,Height}, div size) at different stages to see what is
> happening.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 9aacfa8081b35bb8ae1a59ce3fd9d7aba57cfc7b/dom/base/nsDOMWindowUtils.cpp#3256-
> 3324

Yeah, I'm pretty sure it's the MozWindowUpdatePos event. Bug 1295815 has failures on non-Linux platform too. I'll remove the event and try some retriggers.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a227937f709f4ee383a94c7a04cf0593101eb50d
(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #16)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a227937f709f4ee383a94c7a04cf0593101eb50d

Removing the event handler in TabParent fixes this test but breaks toolkit/components/windowcreator/test/test_window_open_position_constraint.html

I think we need to find a way to deal with the delayed or unexpected resize in fullscreen tests
The log when test works

301 INFO must wait for focus
302 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar 
303 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar 
304 INFO [scrollbar] Entering fullscreen on root
GECKO(26054) | ================================= SendUpdateDimensions (1000, 1000)
GECKO(26054) | ================================= RecvUpdateDimensions (1000, 1000)
GECKO(26054) | ================================= ReflowFinished->UpdatePositionAndSize
GECKO(26054) | ================================= SendUpdateDimensions (3200, 1800)
GECKO(26054) | ================================= RecvUpdateDimensions (3200, 1800)
305 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should only check the current fullscreen element 
306 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen 
307 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar when [object HTMLHtmlElement] is in fullscreen 
308 INFO [scrollbar] Triggering a force frame reconstruction
309 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen 
310 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar when [object HTMLHtmlElement] is in fullscreen 

The log when test fails

343 INFO must wait for focus
344 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar 
345 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar 
346 INFO [scrollbar] Entering fullscreen on root
GECKO(26054) | ================================= SendUpdateDimensions (1000, 1000)
GECKO(26054) | ================================= RecvUpdateDimensions (1000, 1000)
347 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should only check the current fullscreen element 
GECKO(26054) | ================================= ReflowFinished->UpdatePositionAndSize
348 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen - got 486, expected 1586
349 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar when [object HTMLHtmlElement] is in fullscreen - got 486, expected 886
350 INFO [scrollbar] Triggering a force frame reconstruction
GECKO(26054) | ================================= SendUpdateDimensions (3200, 1800)
351 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen - got 486, expected 1586
352 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar when [object HTMLHtmlElement] is in fullscreen - got 486, expected 886
353 INFO [scrollbar] Entering fullscreen on div
GECKO(26054) | ================================= RecvUpdateDimensions (3200, 1800)
354 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should only check the current fullscreen element 

So the reflow in the parent side must be finished too before we can check the result. The invokeCallback hack in addFullscreenChangeContinuation is not enough.
I don't have good ideas to fix this other than checking #measure dimensions in a setTimeout loop. What do you think?
Flags: needinfo?(xidorn+moz)
Blocks: 1332040

Updated

8 months ago
See Also: → bug 1352923
I don't have idea without some investigation, which I probably don't have much time to do for now.

As far as it is Linux-only, we can probably disable the problematic subtest via adding it to shouldSkipTest() in test_fullscreen-api.html with the reference to this bug. We probably also want to disable subtest file_fullscreen-api.html as well for bug 1332040.

I can probably revisit these issues when I have time...

I'm more worried about bug 1295815 because that's not Linux-specific. I guess we can probably workaround it somehow without disabling.
Flags: needinfo?(xidorn+moz)
Created attachment 8855459 [details] [diff] [review]
1350875.patch

This should fix this issue but not bug 1332040 which might have different causes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f96541515fd0e903021c89937b99990b5753cc1
Comment on attachment 8855459 [details] [diff] [review]
1350875.patch

Could you review this or suggest a reviewer?
Attachment #8855459 - Flags: review?(xidorn+moz)
TBH I'm not really happy with this... We have too many special checks here just for Linux because of its weird WM mechanism... Actually I believe the only reason addFullscreenChangeContinuation still needs to exist is because of Linux... I would rather just disable the subtests on Linux rather than adding more complexity to the tests to hide the mess.

Also, I still think we should sync viewport change with layout, otherwise there could be unnecessary additional reflow, which slows down the fullscreen change. And this kind of observable intermediate state could also potentially hurt webcompat.
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #24)
> TBH I'm not really happy with this... We have too many special checks here
> just for Linux because of its weird WM mechanism... Actually I believe the
> only reason addFullscreenChangeContinuation still needs to exist is because
> of Linux... I would rather just disable the subtests on Linux rather than
> adding more complexity to the tests to hide the mess.

I'm afraid that if we disable the subtests for linux then we won't notice it when it breaks.

> Also, I still think we should sync viewport change with layout, otherwise
> there could be unnecessary additional reflow, which slows down the
> fullscreen change. And this kind of observable intermediate state could also
> potentially hurt webcompat.

This is the right thing to do but it will need some time.

Given that we don't have time to deal with this right now, let's disable the subtests and file a bug to fix this properly.
Comment hidden (mozreview-request)
Attachment #8855459 - Attachment is obsolete: true
Attachment #8855459 - Flags: review?(xidorn+moz)
Depends on: 1354676
Comment on attachment 8855915 [details]
Bug 1350875 - Disable file_fullscreen-api.html and file_fullscreen-scrollbar.html for linux m-e10s.

https://reviewboard.mozilla.org/r/127800/#review130614

ok.

::: dom/html/test/test_fullscreen-api.html:74
(Diff revision 1)
> +
>  function shouldSkipTest(test) {
> -  if (test == "file_fullscreen-plugins.html") {
> -    if (!SpecialPowers.isMainProcess() &&
> +  if (!SpecialPowers.isMainProcess() &&
> -        navigator.platform.indexOf('Linux') >= 0) {
> +      navigator.platform.indexOf('Linux') >= 0) {
> -      // Bug 1330553
> +    for (shouldSkip of gLinuxE10sSkipList) {

```javascript
for (let item of gLinuxE10sSkipList) {
```
Attachment #8855915 - Flags: review?(xidorn+moz) → review+
Keywords: leave-open
Whiteboard: [stockwell needswork] → [stockwell disabled]

Comment 28

8 months ago
8 failures in 867 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 6
* mozilla-inbound: 2

Platform breakdown:
* linux64: 8

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875&startday=2017-04-03&endday=2017-04-09&tree=all
Comment hidden (mozreview-request)
Comment on attachment 8856506 [details]
Bug 1350875 - Disable file_fullscreen-api.html and file_fullscreen-scrollbar.html for linux m-e10s.

https://reviewboard.mozilla.org/r/128456/#review130892
Attachment #8856506 - Flags: review?(xidorn+moz) → review+

Comment 31

8 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b32571657c5b
Disable file_fullscreen-api.html and file_fullscreen-scrollbar.html for linux m-e10s. r=xidorn
Comment hidden (mozreview-request)

Comment 33

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b32571657c5b

Updated

4 months ago
Priority: -- → P5
:xidorn, this has been disabled for a few months, is it worthwhile to leave this bug open?
Flags: needinfo?(xidorn+moz)
I think we may still want to enable them at some point... so I think we should still keep this open. Although I don't have plan for this at the moment...
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.