For mus, and really for other non-android platforms, should the LTHI ever be allocating a new LocalSurfaceId? The LocalSurfaceId should normally be provided by what is embedding the compositor (so aura, or content). On android, we have the use-case where scrolling the web-page requires moving the top-bar controls, which requires a new LSI allocation, and that needs to happen from LTHI. But does any of the other cases require an LSI allocation from LTHI?

On other platforms, a new child id should only be allocated on demand via RequestNewLocalSurfaceId, which happens in some cases such as renderer-initiated resize.

For auto-resize/renderer-initiated resize etc., blink has to be the one that initiates the size-change, right? If that is the case, then shouldn't content be able to allocate the associated LSI for the change?

So we have a few cases for LTHI updating the child sequence number:

  - Top controls changing  
  - Android controls, selection, background transparency
  - Renderer initialized resize
  - Init/reconnecting a LayerTreeFrameSink
 
We are seeing an issue with the reconnect, as this is also being handled by Mus. So we end up with duplicate advances of the child sequence number. However Mus ends up not realizing this, and the Window Server ends up out of sync with the actual LSId state.

As for resize, currently we increment the child sequence, and notify the embedder. Which then merges the ids and notifies the renderer.

We don't really end up with duplicate advances of the child sequence. Rather it's that LTHI advances the child sequence, which is then not communicated externally so that the id can't be passed to mus. If the id was injected (LTHI doesn't advance ids) this would not be a problem.

Right. So in this case, the problematic re-allocation is in LTHI::InitializeFrameSink() [1]. From the comment in the code, it sounds like we attempt to re-allocate so that frame-eviction does cause problems here. Is that correct? What can go wrong with frame-eviction if we re-use the LSI? I tested locally, and if we do not re-allocate here, things keep working after a gpu-restart in s-mash, but I don't know how that might break with frame-eviction etc. samans@/jonross@ do you have thoughts?

[1] https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=3356

I hit this DCHECK if I comment out the GenerateId call in LTHI:

[129160:129160:0116/113320.567182:FATAL:compositor_frame_sink_support.cc(304)] Check failed: result == SubmitResult::ACCEPTED (2 vs. 0)
#0 0x7fc8a64de89f base::debug::StackTrace::StackTrace()
#1 0x7fc8a640b8ba logging::LogMessage::~LogMessage()
#2 0x7fc89684551a viz::CompositorFrameSinkSupport::SubmitCompositorFrame()
#3 0x7fc8a3953042 content::DelegatedFrameHost::SubmitCompositorFrame()
#4 0x7fc8a374ad70 content::RenderWidgetHostViewAura::SubmitCompositorFrame()
#5 0x7fc8a373e663 content::RenderWidgetHostImpl::SubmitCompositorFrame()
#6 0x7fc8a31afc9b viz::mojom::CompositorFrameSinkStubDispatch::Accept()
#7 0x7fc8a67a2e76 mojo::InterfaceEndpointClient::HandleValidatedMessage()
#8 0x7fc8a67a27e6 mojo::FilterChain::Accept()
#9 0x7fc8a67a4205 mojo::InterfaceEndpointClient::HandleIncomingMessage()
#10 0x7fc8a67aa668 mojo::internal::MultiplexRouter::ProcessIncomingMessage()
#11 0x7fc8a67a9b29 mojo::internal::MultiplexRouter::Accept()
#12 0x7fc8a67a27e6 mojo::FilterChain::Accept()
#13 0x7fc8a679d536 mojo::Connector::ReadSingleMessage()
#14 0x7fc8a679e041 mojo::Connector::ReadAllAvailableMessages()
#15 0x7fc8a679dee9 mojo::Connector::OnHandleReadyInternal()
#16 0x7fc8a679e857 mojo::SimpleWatcher::DiscardReadyState()
#17 0x7fc8a6762b92 mojo::SimpleWatcher::OnHandleReady()
#18 0x7fc8a67630a1 _ZN4base8internal7InvokerINS0_9BindStateIMN4mojo13SimpleWatcherEFvijRKNS3_18HandleSignalsStateEEJNS_7WeakPtrIS4_EEijS5_EEEFvvEE7RunImplIRKS9_RKN\
St4__Cr5tupleIJSB_ijS5_EEEJLm0ELm1ELm2ELm3EEEEvOT_OT0_NSI_16integer_sequenceImJXspT1_EEEE
#19 0x7fc8a63efb31 base::debug::TaskAnnotator::RunTask()
#20 0x7fc8a641a9ef base::MessageLoopImpl::RunTask()
#21 0x7fc8a641b073 base::MessageLoopImpl::DoWork()
#22 0x7fc8a6501b69 base::MessagePumpLibevent::Run()
#23 0x7fc8a641a598 base::MessageLoopImpl::Run()
#24 0x7fc8a644b9b5 base::RunLoop::Run()
#25 0x55db8970e00b ChromeBrowserMainParts::MainMessageLoopRun()
#26 0x7fc8a3351e44 content::BrowserMainLoop::RunMainMessageLoopParts()
#27 0x7fc8a33544f6 content::BrowserMainRunnerImpl::Run()
#28 0x7fc8a334ea6e content::BrowserMain()
#29 0x7fc8a3e2ffcc content::ContentMainRunnerImpl::RunServiceManager()
#30 0x7fc8a3e2fbd1 content::ContentMainRunnerImpl::Run()
#31 0x7fc89644c3b3 service_manager::Main()
#32 0x7fc8a3e2e034 content::ContentMain()
#33 0x55db8889f803 ChromeMain
#34 0x7fc8969942b1 __libc_start_main
#35 0x55db8889f67a _start

Oh, interesting. Yea, I get that too most of the time. Sometimes it recovers without that crash though.

samans@: do you know why we return null when there is an existing surface from CreateSurface, instead of the existing surface?

There was some complexity with having surfaces come back to life once they were put in the garbage collector's queue. It wasn't too bad to be honest, but we thought just allocating a new id would be nicer.

We can avoid this allocation when VizDisplayCompositor is enabled. Does smash imply  viz?

Well, now that I think about it, probably it doesn't, otherwise there was no way we would enable it by default on cros.

So I can think of a few solutions:
- Simulate what happens on context loss when viz is enabled, i.e. destroy all surfaces. Then don't allocate a new id in the client.
- Bring back the code that used to allow us to resurrect surfaces. Then don't allocate a new id. 
- Once external child allocators are supported, let the owner of the allocator allocate ids in this case.