Teardown external view embedder prior to unmerging threads #30893
Conversation
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I'm not at all familiar with this code, and threading details can be subtle; surely there's a better reviewer for this?
shell/platform/android/external_view_embedder/external_view_embedder.cc
Outdated
Show resolved
Hide resolved
| // The platform view embedder may keep these surfaces even after the lease have expired in | ||
| // case they are used in a future frame that contains platform views. | ||
| // | ||
| // This is particularly an issue when a "cached" engine is used. In this case, this method |
There was a problem hiding this comment.
nits:
When I read this, I don't know what it means by "cached" engine. Maybe we can add a reference link to an issue or something to explain it?
There was a problem hiding this comment.
There's mention down below that FlutterEngineCache is a good place to look at.
blasten
force-pushed
the
teardown_before_unmerge
branch
from
e292770 to
20250bc
Compare
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Adding synchronous waits in the embedder (like the one added in runOnLooper) makes threading hard to reason about. There are a few places this happens in the engine and even in that localized context, it is tricky. Please don't lean into this anti-pattern by adding a convenience method for this (runOnLooper). I believe this can be handled in the engine itself. Perhaps the pool needs to be notified (via barriers) maybe that the thread configuration is about to be swapped?
|
I updated the PR to reuse the APIs available in the engine. PTAL |
blasten
added
the
waiting for tree to go green
…lutter#30893)" This reverts commit d1d8119.
…reads (flutter#30893)" (flutter#31108)" This reverts commit 55420ec.
5 participants
Fixes flutter/flutter#96661
I have a pending PR that refactors the external_view_embedder, and makes testing this situation easier.