+sadrul@, +sky@: I'd like to get LocalSurfaceId allocation out of Aura windows because in this case, it isn't an aura window resize that is initiating the change. Not sure where to put the allocator and how to plumb the allocation through Aura, though. This needs more thought.

Perhaps only places that are associated with LayerTreeHosts should have logic for allocating the id and Window should only offer setters for it? When the size of a WindowTreeHost changes WindowTreeHost would be responsible for applying the new size and calling SetLocalSurfaceId on the window. Does that address the issue?

A semi-related observation: Where we update LocalSurfaceId for DSF changes is unclear or redundant.
- A DSF change detected by RWHVAura::OnDisplayMetricsChanged (among others) will call
  RWHVBase::UpdateScreenInfo, which calls
  RWHVAura::OnSynchronizedDisplayPropertiesChanged, which calls
  RWHVAura::WasResized, which then allocates a new LocalSurfaceId via
  aura::Window::AllocateLocalSurfaceId
- A DSF change will also be picked up by
  aura::Window::OnDeviceScaleFactorChanged, which calls
  aura::WindowPortLocal::OnDeviceScaleFactorChanged, which also allocates a local surface id

Which of these is the intended mechanism, and why?

Unfortunately, they're used for different use cases and the fact that they interact is unfortunate.

1. WindowPortLocal was added in order to support exo (ARC++). When building Mus and Mus+Ash, our initial intention was that FrameSinkId and LocalSurfaceId were implementation details of aura::Windows and clients didn't need to care about synchronization (it happened automagically inside aura).


2. Later it was suggested that we might as well synchronize color space and other ScreenInfo properties (such as screen orientation) as well, and Viz should support non-Mus use cases. The latter case in particular required us to expose LocalSurfaceIds outside of aura.

That's how we got to where we are, unfortunately.

In retrospect, I would recommend (assuming sadrul@, and sky@ support this) moving LocalSurfaceId and ParentLocalSurfaceIdAllocator outside of aura.

The problem with this is it's kind of major surgery for Mus+Ash so I'm not sure we'd want to pursue this immediately.

An alternative is to leverage cblume's ScopedSurfaceIdAllocator in order to make sure we only allocate a LocalSurfaceId..maybe..

Also, THANK YOU, Chris for investigating all this! HUGELY appreciated!

re comment #4: in current state, I think the second option is preferable. Since aura is currently responsible for allocating the LocalSurfaceId in some cases, it'd be simpler if it is *always* responsible for doing this. The fact that we do AllocateLocalSurfaceId() in RWHVAura::WasResized() [1] I think is unfortunate. The better place to do this would be when an aura::Window's bounds changes (either in Window::OnLayerBoundsChanged(), see a wip attempt at [2], or in RWHVAura::OnBoundsChanged() [3]).

re comment #5: I think it's also unfortunate that we split the LocalSurfaceId allocation into WindowPortLocal and WindowPortMus. I think aura::Window itself can be responsible (see the earlier-mentioned wip CL [2]) ... with some extra work for mus+ash when I believe the window-server is sometimes responsible for allocating the LSI?

I like the idea of keeping LSI allocation inside of aura::Window, because then there's one place where this happens (internally in aura, instead of spread in content and in exo). Although I think it can still be through the WindowDelegate if needed (in which means RWHVAura will be involved). The Window::AllocateLocalSurfaceId() would go away, and would be replaced by Window::InvalidateLocalSurfaceId(). Calls to Window::GetLocalSurfaceId() will decide whether to allocate a new one (if size/dsf etc. changed, or it was explicitly invalidated), or to re-use the previous one.

[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_aura.cc?l=2038
[2] https://chromium-review.googlesource.com/c/chromium/src/+/938388/3/ui/aura/window.cc#1150
[3] https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_aura.cc?l=1536

To go a bit further on #6:

What about just adding an aura::WindowObserver::OnLocalSurfaceIdChanged method, which aura::WindowTreeHost and content::RenderWidgetHostViewAura can listen to? We should also them remove aura::Window::GetLocalSurfaceId, because anything querying it without listening for OnLocalSurfaceIdChanged is guaranteed to be missing changes that it needs to know about.

I have a patch I'm going to send out (making sure it passes all tests first) for this issue, which fixes a few of the races.

Some DSF cleanup is needed in Aura in general.
* Every function that specifies a pixel value needs to specify a device scale factor. This is a pervasive problem that is hitting the particular stacks in this bug (OnHostResizedInPixels).
* We need to stop using the DSF from display::Display promiscuously -- ui::GetScaleFactorForNativeView is the worst offender here (I plan to delete that function). In general if we have a question about DSF, we need to be asking "for what purpose is this information to be used, and what is the source of the consistent view of this information". The current approach of "just grab a DSF from a hopefully-nearby display and hope for the best" causes all sorts of inconsistency.

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

commit 09f8e63796bcc2beec72f1934de1d1940645c32b
Author: Christopher Cameron <ccameron@chromium.org>
Date: Mon Mar 12 08:56:47 2018

Remove aura::WindowTreeHost::SetOutputSurfacePaddingInPixels

This isn't used.

TBR=sky

Bug: 818085
Change-Id: I2998682053bffc7a1b8b016d4eb8280dfcd20993
Reviewed-on: https://chromium-review.googlesource.com/958003
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542443}
[modify] https://crrev.com/09f8e63796bcc2beec72f1934de1d1940645c32b/ash/host/ash_window_tree_host.cc
[modify] https://crrev.com/09f8e63796bcc2beec72f1934de1d1940645c32b/ui/aura/window_tree_host.cc
[modify] https://crrev.com/09f8e63796bcc2beec72f1934de1d1940645c32b/ui/aura/window_tree_host.h
[modify] https://crrev.com/09f8e63796bcc2beec72f1934de1d1940645c32b/ui/aura/window_tree_host_unittest.cc

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

commit fa89068bcb7e4b624326f48c351bdea6bdcc1e5c
Author: Christopher Cameron <ccameron@chromium.org>
Date: Mon Mar 12 20:29:31 2018

Fix WindowTreeHost invariant violation

Do not set the ui::Compositor's LocalSurfaceId until after it allocated,
which is done by resizing its aura::Window.

Just changing the order of the calls breaks the behavior of
UpdateRootWindowSizeInPixels. This is a function that specifies pixels
and commits the sin of not specifying a device scale factor. It needs
a DSF, so it grabs one from the ui::Compositor (which now is out of
date).

To fix this, snapshot the device scale factor from the display::Display
in OnHostResizedInPixels, where it should have been specified to begin
with.

This opens up the question of why the value of OnHostResizedInPixels is
not snapshotted itself, so add comments indicating this potential source
of inconsistency. While exploring that rabbit hole, some surface
invariant violations in DesktopWindowTreeHost were also discovered, so
add comments to remember them.

Bug: 818085
Change-Id: I88d5b59e031ef6c57b5a7ef778540ae4a8b95bc8
Reviewed-on: https://chromium-review.googlesource.com/958329
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542584}
[modify] https://crrev.com/fa89068bcb7e4b624326f48c351bdea6bdcc1e5c/ui/aura/mus/window_tree_client_unittest.cc
[modify] https://crrev.com/fa89068bcb7e4b624326f48c351bdea6bdcc1e5c/ui/aura/window_tree_host.cc
[modify] https://crrev.com/fa89068bcb7e4b624326f48c351bdea6bdcc1e5c/ui/aura/window_tree_host.h
[modify] https://crrev.com/fa89068bcb7e4b624326f48c351bdea6bdcc1e5c/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc

To go back to #4, a few more remarks on this:

- aura::Window::OnDeviceScaleFactorChanged is an override of ui::LayerDelegate
  - it is responsible for allocating a new SurfaceId

- this means it will only be called by ui::Compositor::SetScaleAndSize
  - this function is supposed to take the aura::Window's SurfaceId as an argument
  - and the value of the aura::Window's SurfaceId changes in this function call
  - so a broken state is guaranteed to exist, and how it resolves itself is unclear

We have two different stacks where bounds and DSF travel.

Bounds goes through an explicit ui::Layer::SetBounds call
  aura::WindowPortLocal::OnDidChangeBounds()
  aura::Window::OnLayerBoundsChanged()
  ui::Layer::SetBoundsFromAnimation()
  ui::LayerAnimator::SetBounds()
  ui::Layer::SetBounds()
  aura::Window::SetBoundsInternal()
  aura::Window::SetBounds()
  ash::TransformerHelper::UpdateWindowSize()
  ash::AshWindowTreeHostPlatform::UpdateRootWindowSizeInPixels()
  aura::WindowTreeHost::OnHostResizedInPixels()

DSF comes through the ui::Compositor::SetScaleAndSize call
  aura::WindowPortLocal::OnDeviceScaleFactorChanged()
  aura::Window::OnDeviceScaleFactorChanged()
  ui::Layer::OnDeviceScaleFactorChanged()
  ui::Compositor::SetScaleAndSize()
  aura::WindowTreeHost::OnHostResizedInPixels()

So it seems odd that ui::Compositor will
- call ui::Layer::OnDeviceScaleFactorChanged on root_layer_
- *not* call ui::Layer::SetBounds on root_layer_ (leaving that responsibility to the caller)
- cc::Layer::SetBounds on root_web_layer_

The fix seems to be to make the caller responsible for calling ui::Layer::OnDeviceScaleFactorChanged, just like it is responsible for for calling ui::Layer::SetBounds.

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

commit 5b7677f54a8e72415477b9ba157fafa32ed1ae3e
Author: Christopher Cameron <ccameron@chromium.org>
Date: Tue Mar 13 23:32:32 2018

aura: Work around race condition in ui::Compositor::SetScaleAndSize

The aura::Window LocalSurfaceId is updated after device scale factor
change via
- aura::Window::OnDeviceScaleFactorChanged, which is called by
- ui::Layer::OnDeviceScaleFactorChanged, which is called by
- ui::Compositor::SetScaleAndSize

But ui::Compositor::SetScaleAndSize takes the aura::Window's
LocalSurfaceId as a parameter.

This means that ui::Compositor::SetScaleAndSize's LocalSurfaceId
argument will be incorrect any time that the DeviceScaleFactor
changes.

Note that the way that aura::Window updates its LocalSurfaceId for
bounds' changes is different. It does not implicitly update the
bounds of its root layer, rather, it requires that the root layer's
bounds be updated before calling ui::Compositor::SetScaleAndSize.

Add a function aura::Window::SetDeviceScaleFactor, to match
aura::Window::SetBounds, which will update the layer's properties
and cause the generation of a new LocalSurfaceId. Make this call in
WindowTreeHost::UpdateRootWindowSizeInPixels (next to the corresponding
call to aura::Window::SetBounds).

Bug: 818085
Change-Id: I929a6e641aa3aa1253e99647ce3bfe05ca7c482a
Reviewed-on: https://chromium-review.googlesource.com/961450
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542948}
[modify] https://crrev.com/5b7677f54a8e72415477b9ba157fafa32ed1ae3e/ui/aura/window.cc
[modify] https://crrev.com/5b7677f54a8e72415477b9ba157fafa32ed1ae3e/ui/aura/window.h
[modify] https://crrev.com/5b7677f54a8e72415477b9ba157fafa32ed1ae3e/ui/aura/window_tree_host.cc

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

commit 7c4a8576d7f2288bc8a6dfca059c947e7cc4c036
Author: Christopher Cameron <ccameron@chromium.org>
Date: Fri Mar 16 08:03:02 2018

aura: Fix race condition in AshWindowTreeHost classes

These classes override the UpdateRootWindowSizeInPixels method, and
consequently fall vicitim to the race condition described in
https://crrev.com/542948

Propagate the device scale factor change through the overridden
UpdateRootWindowSizeInPixels methods.

Add comments indicating inconsitent behavior in
UpdateRootWindowSizeInPixels, as its interaction with other methods
for updating bounds is unclear.

Bug: 818085
Change-Id: I0fc43954a532f4a9eb269919685f5108b762ed2a
Reviewed-on: https://chromium-review.googlesource.com/963487
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543645}
[modify] https://crrev.com/7c4a8576d7f2288bc8a6dfca059c947e7cc4c036/ash/host/ash_window_tree_host_mus.cc
[modify] https://crrev.com/7c4a8576d7f2288bc8a6dfca059c947e7cc4c036/ash/host/ash_window_tree_host_platform.cc
[modify] https://crrev.com/7c4a8576d7f2288bc8a6dfca059c947e7cc4c036/ash/host/transformer_helper.cc
[modify] https://crrev.com/7c4a8576d7f2288bc8a6dfca059c947e7cc4c036/ash/host/transformer_helper.h
[modify] https://crrev.com/7c4a8576d7f2288bc8a6dfca059c947e7cc4c036/ui/aura/window_tree_host.h

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

commit 827ddeb7c882d0cb7fdcb9615ce64f58261812e8
Author: Christopher Cameron <ccameron@chromium.org>
Date: Fri Mar 16 09:19:23 2018

cc: CHECK surface invariants in LayerTreeHost on all platforms

This is an option now that aura::WindowTreeHost have been fixed

Bug: 818085
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I074ade34758a5005862543549e4bbe69838301b9
Reviewed-on: https://chromium-review.googlesource.com/959550
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543656}
[modify] https://crrev.com/827ddeb7c882d0cb7fdcb9615ce64f58261812e8/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/827ddeb7c882d0cb7fdcb9615ce64f58261812e8/testing/buildbot/filters/mash.ash_unittests.filter

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

commit 519c97b069ccbb70c8e1e1b18da4490080a1b4f1
Author: ccameron <ccameron@chromium.org>
Date: Mon Mar 19 08:49:25 2018

Revert "cc: CHECK surface invariants in LayerTreeHost on all platforms"

This reverts commit 827ddeb7c882d0cb7fdcb9615ce64f58261812e8.

Reason for revert: crbug.com/823119

Original change's description:
> cc: CHECK surface invariants in LayerTreeHost on all platforms
>
> This is an option now that aura::WindowTreeHost have been fixed
>
> Bug: 818085
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> Change-Id: I074ade34758a5005862543549e4bbe69838301b9
> Reviewed-on: https://chromium-review.googlesource.com/959550
> Commit-Queue: ccameron <ccameron@chromium.org>
> Reviewed-by: Fady Samuel <fsamuel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#543656}

TBR=ccameron@chromium.org,fsamuel@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 818085, 823119
Change-Id: Ib0e66b23c1fb3420a2a6d38e17255e7b01b9e803
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/968161
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543993}
[modify] https://crrev.com/519c97b069ccbb70c8e1e1b18da4490080a1b4f1/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/519c97b069ccbb70c8e1e1b18da4490080a1b4f1/testing/buildbot/filters/mash.ash_unittests.filter

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

commit e177f11f13b53c54b99d5d860510cb7ecb41012a
Author: Christopher Cameron <ccameron@chromium.org>
Date: Mon Mar 19 21:30:23 2018

Revert "cc: CHECK surface invariants in LayerTreeHost on all platforms"

This reverts commit 827ddeb7c882d0cb7fdcb9615ce64f58261812e8.

Reason for revert: crbug.com/823119

Original change's description:
> cc: CHECK surface invariants in LayerTreeHost on all platforms
>
> This is an option now that aura::WindowTreeHost have been fixed
>
> Bug: 818085
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> Change-Id: I074ade34758a5005862543549e4bbe69838301b9
> Reviewed-on: https://chromium-review.googlesource.com/959550
> Commit-Queue: ccameron <ccameron@chromium.org>
> Reviewed-by: Fady Samuel <fsamuel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#543656}

TBR=ccameron@chromium.org, fsamuel@chromium.org


(cherry picked from commit 519c97b069ccbb70c8e1e1b18da4490080a1b4f1)

Bug: 818085, 823119
Change-Id: Ib0e66b23c1fb3420a2a6d38e17255e7b01b9e803
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/968161
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#543993}
Reviewed-on: https://chromium-review.googlesource.com/969612
Cr-Commit-Position: refs/branch-heads/3375@{#1}
Cr-Branched-From: 9a9b208d93102cd427f37a45e2bdc28c07f23cbc-refs/heads/master@{#543961}
[modify] https://crrev.com/e177f11f13b53c54b99d5d860510cb7ecb41012a/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/e177f11f13b53c54b99d5d860510cb7ecb41012a/testing/buildbot/filters/mash.ash_unittests.filter