The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8dbecfca5db911d9f54fcd54dcec49aa8205eab3

commit 8dbecfca5db911d9f54fcd54dcec49aa8205eab3
Author: Yi Gu <yigu@chromium.org>
Date: Mon Oct 22 20:12:21 2018

Disable unittests that failed ASan LSan tests

ExtensionApiTest.DisplayModeWindowIsInFullscreen and
ChromeNavigationBrowserTest.TransientEntryPreservedOnMultipleNavigationsDuringInterstitial
started to fail after patch crrev.com/c/1292513
Given that the patch was landed 4 hours ago, will disable the tests and
assign the bug to the patch owner.

TBR=sky@chromium.org
NOTRY=true

Bug: 897879
Change-Id: I559a1820293fe057eebb628b9d5aa962f1999020
Reviewed-on: https://chromium-review.googlesource.com/c/1294599
Commit-Queue: Yi Gu <yigu@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601702}
[modify] https://crrev.com/8dbecfca5db911d9f54fcd54dcec49aa8205eab3/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/8dbecfca5db911d9f54fcd54dcec49aa8205eab3/chrome/browser/extensions/extension_fullscreen_apitest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/84d6ae374c2591fd605e3fba51e15ca5421d749e

commit 84d6ae374c2591fd605e3fba51e15ca5421d749e
Author: Scott Violet <sky@chromium.org>
Date: Tue Oct 23 00:27:24 2018

chromeos: Disables DisplayModeWindowIsInFullscreen

It's flaky, but only on ChromeOS, and only with single-process-mash.

BUG=897879
TEST=test only change
TBR=yigu@chromium.org

Change-Id: I26d5e39edc67c16d1334a2279d7f93796746143a
Reviewed-on: https://chromium-review.googlesource.com/c/1295188
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601798}
[modify] https://crrev.com/84d6ae374c2591fd605e3fba51e15ca5421d749e/chrome/browser/extensions/extension_fullscreen_apitest.cc
[modify] https://crrev.com/84d6ae374c2591fd605e3fba51e15ca5421d749e/testing/buildbot/filters/chromeos.single_process_mash.interactive_ui_tests.filter

 Issue 898169  has been merged into this issue.

This test seemed most flaky on the asan bot.

This test appears to be using the JS api Window.matchMedia(). It toggles the window state (fullscreen/restore) and then checks using matchMedia(). See chrome/test/data/extensions/api_test/fullscreen/mq_display_mode/window.js. I'm guessing there is a race condition with how the media state is queried when script is changing the state. I'm not sure what browser side code feeds the media state.


ikilpatrick, would you happen to know what code is content is responsible for setting the media state?

Without being able to answer #6, it would seem the problem is that the state change and the resulting size change are not kept synchronous. If this test is passing on all other platforms plus classic Ash it seems that Mash must be the outlier in that regard. We've run into this within Chrome but can always change Chrome code to work around it, e.g. [0], but if there's js out there that relies on these two properties updating together, we might need to revise how Mash/ws sends those updates. Although I'm not sure why it would be flaky if this is the problem.

mlamouri@ or ikilpatrick@, is it a requirement that the display mode is updated at the same time or before the resize event, or is that just an artifact of the test?[1]

[0] https://chromium-review.googlesource.com/c/chromium/src/+/1153397
[1] https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/fullscreen/mq_display_mode/window.js

I only poked at this briefly to chase down the failure. I'm not actively looking at it, so I'm passing to Evan.

Ian and/or Mounir, any thoughts on the Q in #7?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b373e9317d0985abbbdca59877ae8ad80919abef

commit b373e9317d0985abbbdca59877ae8ad80919abef
Author: Scott Violet <sky@chromium.org>
Date: Wed Dec 19 06:58:03 2018

chromeos: removes single_process_mash.interactive_ui_tests.filter file

And instead adds early outs. This way the tree will be green when we switch
SingleProcessMash on by default.

BUG= 916183 ,897879,916379
TEST=test only changes

Change-Id: I0e32837a216e02611ceb2b1454c31cf2c4defb4e
Reviewed-on: https://chromium-review.googlesource.com/c/1383124
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617746}
[modify] https://crrev.com/b373e9317d0985abbbdca59877ae8ad80919abef/chrome/browser/browser_keyevents_browsertest.cc
[modify] https://crrev.com/b373e9317d0985abbbdca59877ae8ad80919abef/chrome/browser/extensions/extension_fullscreen_apitest.cc
[modify] https://crrev.com/b373e9317d0985abbbdca59877ae8ad80919abef/testing/buildbot/chromium.chromiumos.json
[modify] https://crrev.com/b373e9317d0985abbbdca59877ae8ad80919abef/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/b373e9317d0985abbbdca59877ae8ad80919abef/testing/buildbot/filters/BUILD.gn
[delete] https://crrev.com/94cbc94bffae9985b93e9b8381907726faf5efb7/testing/buildbot/filters/chromeos.single_process_mash.interactive_ui_tests.filter
[modify] https://crrev.com/b373e9317d0985abbbdca59877ae8ad80919abef/testing/buildbot/test_suites.pyl

Sorry for the silence. I missed this ping.

I do not remember any hard requirement but in general, I would advise caution as websites may rely on such timing and it may not be a good idea to have mash break it as this could lead to very hard to reproduce bugs in the future.

Thanks Mounir.

Scott, I think to fix this the cautious way would require us to coalesce changes to window properties, but we've been reluctant to do that in the past. WDYT?

Evan, could you be more specific as to what you mean by 'coalescing changes to window properties'?

It seems like we want some level of atomicity with window bounds and state, but before mash we never really had that, so I'm not sure what the problem is. For example, in classic, if you maximize a widget we set the property first and then change the bounds. So, if you were listening to the property change you wouldn't necessarily see that the bounds had changed. In practice you likely did, because ash's WindowState is created very early on and would change the bounds.

Maybe the right fix is changes to windowstate should trigger sending a bounds change *with* the state.

> ... some level of atomicity with window bounds and state ...
> ... sending a bounds change *with* the state.

yes, that's what I meant by coalescing.

> before mash we never really had that

At the js level we did, right? JS onresize is asynchronous relative to the C++ bounds change while the C++ state change is synchronous.

In thinking about this a bit more my suggestion isn't quite enough. I suspect the sequence is something like:

1. client changes WindowState to maximized.
2. Page is notified that WindowState changed.
3. server services WindowState change, changing bounds.
4. client receives bounds change.

(2) is the problem. If (2) immediately asks for the bounds it won't get the right answer. The client only knows the right bounds at (4). My suggestion above only addresses bounds changes initiated from the server, which isn't (2).

Seems like we have two options:
1. Don't have the client directly change the window state. I have no doubt this would be confusing as it means calls to Widget::IsMaximized() immediately after a call to Widget::Mazimize() would return false.
2. In addition to what I outlined above have the client track a structure that is last known state and bounds. Code that cares about the true state (such as whatever code media is using) would observe this.

Neither solution sounds particularly satisfying. I might be inclined to see how painful (1) is.

OK, I may have to do some more tracing, because I think what you've said is backwards. The test proceeds after a bounds change and the state may not be updated by then, so some way or another it appears to receive the bounds change before the state change.

Tracing would definitely be helpful. I'm surprised that I have it backwards, because I think this is the call sequence:

Widget::Maximize() -> DesktopNativeWidgetAura::Maximize() -> DesktopWindowTreeHostMus::Maximize() -> Window::SetProperty --->

now in the ash side:

WindowTree::SetWindowProperty() -> aura::Window::SetProperty() -> some magic in window state -> aura::Window::SetBounds() --->

back in chrome side

WindowTreeClient::OnWindowBoundsChanged() -> aura::Window::SetBounds() ...

So, if this test is listening for a bounds change, then the state should definitely be maximized once the bounds change.