Closed Bug 1350875 Opened 8 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Core & HTML, defect, P5)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: intermittent-bug-filer, Assigned: xidorn)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell disabled])

Attachments

(9 files, 1 obsolete file)

Bug 1350875 - Disable file_fullscreen-api.html and file_fullscreen-scrollbar.html for linux m-e10s.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1350875 - Disable file_fullscreen-api.html and file_fullscreen-scrollbar.html for linux m-e10s.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1350875 part 1 - Have android notify fullscreen will change.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1350875 part 2 - Have cocoa always notify fullscreen will change.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1350875 part 3 - Have GTK notify fullscreen will change.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1350875 part 4 - Have windows notify fullscreen will change.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1350875 part 6 - Wait for window resize for one frame after window goes fullscreen. r?smaug
47 bytes, text/x-phabricator-request
Details | Review
Bug 1350875 part 7 - Enable fullscreen scroll tests. r?smaug
47 bytes, text/x-phabricator-request
Details | Review
Bug 1350875 part 5 - Have junit fullscreen test wait for fullscreen change to finish. r?snorp
47 bytes, text/x-phabricator-request
Details | Review
> <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: → 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.
See Also: → 1332040
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.
See Also: → 1295815
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.
(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)
See Also: → 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)
Attached patch 1350875.patch (obsolete) — — Splinter Review
This should fix this issue but not bug 1332040 which might have different causes.
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.
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 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+
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
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)
Component: DOM → DOM: Core & HTML
See Also: → 1579623
Assignee: nobody → xidorn+moz
Status: NEW → ASSIGNED

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02b9970fef644dbc1fdca6f797871e60b569d3fd

But this patch may affect all platforms, so I'd probably wait until the end of soft code freeze.

Pushed by mozilla@upsuper.org: https://hg.mozilla.org/integration/autoland/rev/e65ead196f4b part 1 - Have android notify fullscreen will change. r=snorp https://hg.mozilla.org/integration/autoland/rev/71ba43d7b2df part 2 - Have cocoa always notify fullscreen will change. r=spohl https://hg.mozilla.org/integration/autoland/rev/149058087816 part 3 - Have GTK notify fullscreen will change. r=karlt https://hg.mozilla.org/integration/autoland/rev/4eda0d89d8a2 part 4 - Have windows notify fullscreen will change. r=jmathies https://hg.mozilla.org/integration/autoland/rev/5084623c0f83 part 5 - Wait for window resize for one frame after window goes fullscreen. r=smaug https://hg.mozilla.org/integration/autoland/rev/c65f2c0b6332 part 6 - Enable fullscreen scroll tests. r=smaug
Attachment #9136531 - Attachment description: Bug 1350875 part 5 - Wait for window resize for one frame after window goes fullscreen. r?smaug → Bug 1350875 part 6 - Wait for window resize for one frame after window goes fullscreen. r?smaug
Attachment #9136532 - Attachment description: Bug 1350875 part 6 - Enable fullscreen scroll tests. r?smaug → Bug 1350875 part 7 - Enable fullscreen scroll tests. r?smaug
Pushed by mozilla@upsuper.org: https://hg.mozilla.org/integration/autoland/rev/a24fa4189a91 part 1 - Have android notify fullscreen will change. r=snorp,geckoview-reviewers,agi https://hg.mozilla.org/integration/autoland/rev/829b72f954a3 part 2 - Have cocoa always notify fullscreen will change. r=spohl https://hg.mozilla.org/integration/autoland/rev/51e5fc15e945 part 3 - Have GTK notify fullscreen will change. r=karlt https://hg.mozilla.org/integration/autoland/rev/a4aa9a5f050c part 4 - Have windows notify fullscreen will change. r=jmathies https://hg.mozilla.org/integration/autoland/rev/5482b2715e60 part 5 - Have junit fullscreen test wait for fullscreen change to finish. r=geckoview-reviewers,agi https://hg.mozilla.org/integration/autoland/rev/9b7c96ad9198 part 6 - Wait for window resize for one frame after window goes fullscreen. r=smaug https://hg.mozilla.org/integration/autoland/rev/2ba20424fa16 part 7 - Enable fullscreen scroll tests. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Regressions: 1489190
Regressions: 1677244
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: