1430380 - layout/style/test/chrome/test_display_mode_reflow.html fails with "offset top changes" after bug 1193394

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tooru Fujisawa [:arai]]

layout/style/test/chrome/test_display_mode_reflow.html fails with "offset top changes" test failure.
https://searchfox.org/mozilla-central/rev/7476b71e0010ab3277b77cc0ae4d998c4b1d2b64/layout/style/test/chrome/test_display_mode_reflow.html#49

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b7ba9086f5524823ea95f717f4bfbe0430313a&filter-searchStr=mochitest-chro&selectedJob=154669384
> TEST-UNEXPECTED-FAIL | layout/style/test/chrome/test_display_mode_reflow.html | offset top changes
>  @chrome://mochitests/content/chrome/layout/style/test/chrome/test_display_mode_reflow.html:44:3
>  async*add_task/</<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:298:21
>  onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:69:15
>  co/<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:58:5
>  co@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:54:10
>  setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:676:12
>  add_task@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:271:7
>  @chrome://mochitests/content/chrome/layout/style/test/chrome/test_display_mode_reflow.html:28:1

this crash is happening only on linux opt builds

Comment 1

[Tooru Fujisawa [:arai]]

this is the test fails, not line 49 one
https://searchfox.org/mozilla-central/rev/7476b71e0010ab3277b77cc0ae4d998c4b1d2b64/layout/style/test/chrome/test_display_mode_reflow.html#63

Comment 2

[Tooru Fujisawa [:arai]]

secondDiv.offsetTop hasn't changed yet just after fullScreenEntered.
the same behavior is confirmed on google chrome.

Comment 3

[Tooru Fujisawa [:arai]]

The question is, why this happens only on linux.
I'll look into how fullscreen event is dispatched and when/how applied style is changed

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: stack-linux.txt

so far, here's C++ stack for the test after the 2nd `await fullScreenEntered;`, on linux which fails.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: stack-macos.txt

and on macOS, which doesn't fail.
I don't see any suspicious difference.
maybe the difference happens before the event is queued?

Comment 6

[Tooru Fujisawa [:arai]]

err, looks like, bug 1415781 also fixes this as well.

Comment 7

[Tooru Fujisawa [:arai]]

still failing
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef75c03168fa73749a49a5a6f0e51652c943a1a&filter-searchStr=chrome&selectedJob=157668255

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Tooru Fujisawa [:arai] from comment #3)
> The question is, why this happens only on linux.
> I'll look into how fullscreen event is dispatched and when/how applied style
> is changed

So... on other platforms, we only dispatch fullscreenchange event when we complete the change to the viewport, but on Linux we cannot guarantee that (see discussion in bug 1254448, TL;DR, GTK doesn't provide any guarantee to the related signals, and they don't consider this to be a fixable issue, so we cannot provide any guarantee either), so in tests we would need to wait arbitrary time until the viewport actually gets changed.

Comment 9

[Tooru Fujisawa [:arai]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #8)
> (In reply to Tooru Fujisawa [:arai] from comment #3)
> > The question is, why this happens only on linux.
> > I'll look into how fullscreen event is dispatched and when/how applied style
> > is changed
> 
> So... on other platforms, we only dispatch fullscreenchange event when we
> complete the change to the viewport, but on Linux we cannot guarantee that
> (see discussion in bug 1254448, TL;DR, GTK doesn't provide any guarantee to
> the related signals, and they don't consider this to be a fixable issue, so
> we cannot provide any guarantee either), so in tests we would need to wait
> arbitrary time until the viewport actually gets changed.

Thanks :)
I'll try modifying the testcase to wait for the condition about offsetTop.

Comment 10

[Tooru Fujisawa [:arai]]

Here's try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ab731bc1a2eb4ebc94efe7d0f33c1d0b4803bee&group_state=expanded

Comment 11

[Tooru Fujisawa [:arai]]

another try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d32711fc045739d062fdb566831718c5c4859f8c&group_state=expanded

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Wait until viewport change completes on GTK after mozfullscreenchange event in layout/style/test/chrome/test_display_mode_reflow.html.

Just to confirm, the issue about the fullscreenchange event timing was pre-existing, and the change about Promise resolution handler invocation timing (Task to MicroTask) discovers it in the test, right?

Modified test_display_mode_reflow.html to wait for viewport change, on GTK environment.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8946216 [details] [diff] [review]
Wait until viewport change completes on GTK after mozfullscreenchange event in layout/style/test/chrome/test_display_mode_reflow.html.

Review of attachment 8946216 [details] [diff] [review]:
-----------------------------------------------------------------

I'm thinking about whether we can wait for some other condition rather than the one we are listening... Maybe resize event? Not sure whether that's going to work, though.

Actually, looking at the test code, I don't really think it is related to viewport change, since it's purely testing whether the style is applied (media condition is changed), and the media condition should really have been changed after the event is dispatched.

So maybe the problem here is that we don't invalidate media rules when we flip the fullscreen state, and thus the invalidation is only done after viewport size changes? That could be the real bug, I guess, but I'm less sure whether it's worth fixing given we are changing viewport anyway. Maybe it's worth if we decide to flush media queries only when there is relevant conditions.

::: layout/style/test/chrome/test_display_mode_reflow.html
@@ +32,5 @@
>  
> +async function checkCondition(condition, msg) {
> +  if (AppConstants.MOZ_WIDGET_GTK) {
> +    // On GTK, fullscreenchange event can be dispatched before the change to
> +    // the viewport completes.  Wait until the viewpoer gets changed.

s/viewpoer/viewport

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Weird that we do trigger MediaFeatureValuesChanged from SizeModeChanged, which should make this test work regardless of platforms...

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/layout/base/nsPresContext.cpp#2210

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Xidorn gives us a big clue. the sizemodchange event comes from nsWebShellWindow::SizeModeChanged [1].  From the function

  // And always fire a user-defined sizemodechange event on the window
    ourWindow->DispatchCustomEvent(NS_LITERAL_STRING("sizemodechange"));
  }

  nsIPresShell* presShell;
  if ((presShell = GetPresShell())) {
    presShell->GetPresContext()->SizeModeChanged(sizeMode);
  }

The SizeModeChanged() function is called after dispatching sizemodechange event.  The test passed locally on Linux if I moved GetPresContext()->SizeModeChange(sizeMode) before dispatching the event.  My question is "why the test does no fail on other platforms?".  I am not sure this is a right fix though (I doubt it). 

[1] https://hg.mozilla.org/mozilla-central/file/9a3b6d64a64b/xpfe/appshell/nsWebShellWindow.cpp#l389

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Sounds like we should move the
> await promiseNextTick();
after each await for event, so that the remaining thing can get triggered properly...

The reason it doesn't happen in other platforms, then, is because on other platforms, the resize is triggered synchronously before sizemode change gets dispatched, and thus we have the chance to trigger media values change due to the synchronous viewport change.

I don't consider GTK's being asynchronous to be a bug, but that is indeed a difference between GTK and other platforms.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

fullscreenchange event should not be affected by this difference, so we only need to workaround sizemodechange.

Having said that, I'm a bit concerned, then, about this, because I'm not sure if we have chrome code depends on the behavior of sizemodechange this way... If so, we probably need to fix them as well rather than simply working around the test.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #18)
> fullscreenchange event should not be affected by this difference, so we only
> need to workaround sizemodechange.
> 
> Having said that, I'm a bit concerned, then, about this, because I'm not
> sure if we have chrome code depends on the behavior of sizemodechange this
> way... If so, we probably need to fix them as well rather than simply
> working around the test.

I did search in our code.  There are several places that use sizemodechange event, but most of them does not involve Promise so they should not be affected by the micro task change.  All the places that involve Promise are in testing/marionette/driver.js.  And it seems to me that they had been suffering from the symptoms that siemodechange event does not work as expected (bug 1364319 for example).  They found workarounds for the symptoms respectively.  Waiting for requestAnimationFrame callback [1], or polling outerWidth change [2].  The only one place that might be affected by the micro task change is fullscreenWindow [3].  I've filed bug 1434785 for that.

[1] https://hg.mozilla.org/mozilla-central/file/17ade9f88b6e/testing/marionette/driver.js#l3636
[2] https://hg.mozilla.org/mozilla-central/file/17ade9f88b6e/testing/marionette/driver.js#l3011
[3] https://hg.mozilla.org/mozilla-central/file/17ade9f88b6e/testing/marionette/driver.js#l3085

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Moving |await promiseNextTick()| seems harmless on bare m-c;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ae38c2a364c8974b8eb2d945828263f0bc1c0c4

And it worked with the micro task change;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd37f78a8d36cdf57d891b8c7485e0dbd73cc5a8

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Stealing.  I'd suggest to do the workaround here in this bug, and land the micro task change (as soon as possible), and then later we will fix this symptoms in some day, given that the marionette part hasn't been shown up in tries with the micro task changes and even if it happened we can do a workaround in bug 1434785 (They have been already using workarounds for a while).

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1430380 - Wait for the next tick after sizemodechange event to apply media feature changes.

On Linux, media feature changes is processed after dispatching sizemodechange
event, so we need to wait for the next tick before checking style changes that
caused by the media feature changes.

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

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8947324 [details]
Bug 1430380 - Wait for the next tick after sizemodechange event to apply media feature changes.

https://reviewboard.mozilla.org/r/217042/#review222882

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/588b11d5976e
Wait for the next tick after sizemodechange event to apply media feature changes. r=xidorn

Comment 25

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/588b11d5976e