Store layer state changes by layer handle in Transaction objects
Layer state changes are stored in an unordered map with the surface control as the key
and state changes as the value. This causes a few problems when merging the transactions. If
transactions contained state changes from a cloned surface control then it will not be merged.
This will cause ordering issues when the transaction is applied since the changes are stored in
and unordered map.
When parcelling transactions, a new surface control is created from the existing one and this
surfaces the problem more frequently.
Instead we store the layer changes by the layer handle which is consistent across processes.
Test: atest SurfaceFlinger_test
Test: go/wm-smoke
Change-Id: I2e041d70ae24db2c1f26ada003532ad97f667167
diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index 6066421..a09ceae 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -169,12 +169,10 @@
}
status_t ComposerState::write(Parcel& output) const {
- output.writeStrongBinder(IInterface::asBinder(client));
return state.write(output);
}
status_t ComposerState::read(const Parcel& input) {
- client = interface_cast<ISurfaceComposerClient>(input.readStrongBinder());
return state.read(input);
}
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index c59fddf..de36511 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -202,7 +202,8 @@
* callbackIds to generate one super map that contains all the sp<IBinder> to sp<SurfaceControl>
* that could possibly exist for the callbacks.
*/
- std::unordered_map<sp<IBinder>, sp<SurfaceControl>, IBinderHash> surfaceControls;
+ std::unordered_map<sp<IBinder>, sp<SurfaceControl>, SurfaceComposerClient::IBinderHash>
+ surfaceControls;
for (const auto& transactionStats : listenerStats.transactionStats) {
for (auto callbackId : transactionStats.callbackIds) {
auto& [callbackFunction, callbackSurfaceControls] = mCallbacks[callbackId];
@@ -365,16 +366,16 @@
if (count > parcel->dataSize()) {
return BAD_VALUE;
}
- std::unordered_map<sp<SurfaceControl>, ComposerState, SCHash> composerStates;
+ std::unordered_map<sp<IBinder>, ComposerState, IBinderHash> composerStates;
composerStates.reserve(count);
for (size_t i = 0; i < count; i++) {
- sp<SurfaceControl> surfaceControl = SurfaceControl::readFromParcel(parcel);
+ sp<IBinder> surfaceControlHandle = parcel->readStrongBinder();
ComposerState composerState;
if (composerState.read(*parcel) == BAD_VALUE) {
return BAD_VALUE;
}
- composerStates[surfaceControl] = composerState;
+ composerStates[surfaceControlHandle] = composerState;
}
InputWindowCommands inputWindowCommands;
@@ -408,8 +409,8 @@
}
parcel->writeUint32(static_cast<uint32_t>(mComposerStates.size()));
- for (auto const& [surfaceControl, composerState] : mComposerStates) {
- surfaceControl->writeToParcel(parcel);
+ for (auto const& [surfaceHandle, composerState] : mComposerStates) {
+ parcel->writeStrongBinder(surfaceHandle);
composerState.write(*parcel);
}
@@ -418,11 +419,11 @@
}
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Transaction&& other) {
- for (auto const& kv : other.mComposerStates) {
- if (mComposerStates.count(kv.first) == 0) {
- mComposerStates[kv.first] = kv.second;
+ for (auto const& [surfaceHandle, composerState] : other.mComposerStates) {
+ if (mComposerStates.count(surfaceHandle) == 0) {
+ mComposerStates[surfaceHandle] = composerState;
} else {
- mComposerStates[kv.first].state.merge(kv.second.state);
+ mComposerStates[surfaceHandle].state.merge(composerState.state);
}
}
@@ -465,14 +466,12 @@
mDesiredPresentTime = -1;
}
-void SurfaceComposerClient::doDropReferenceTransaction(const sp<IBinder>& handle,
- const sp<ISurfaceComposerClient>& client) {
+void SurfaceComposerClient::doDropReferenceTransaction(const sp<IBinder>& handle) {
sp<ISurfaceComposer> sf(ComposerService::getComposerService());
Vector<ComposerState> composerStates;
Vector<DisplayState> displayStates;
ComposerState s;
- s.client = client;
s.state.surface = handle;
s.state.what |= layer_state_t::eReparent;
s.state.parentHandleForChild = nullptr;
@@ -499,8 +498,8 @@
}
size_t count = 0;
- for (auto& [sc, cs] : mComposerStates) {
- layer_state_t* s = getLayerState(sc);
+ for (auto& [handle, cs] : mComposerStates) {
+ layer_state_t* s = getLayerState(handle);
if (!(s->what & layer_state_t::eBufferChanged)) {
continue;
}
@@ -639,16 +638,15 @@
mEarlyWakeup = true;
}
-layer_state_t* SurfaceComposerClient::Transaction::getLayerState(const sp<SurfaceControl>& sc) {
- if (mComposerStates.count(sc) == 0) {
+layer_state_t* SurfaceComposerClient::Transaction::getLayerState(const sp<IBinder>& handle) {
+ if (mComposerStates.count(handle) == 0) {
// we don't have it, add an initialized layer_state to our list
ComposerState s;
- s.client = sc->getClient()->mClient;
- s.state.surface = sc->getHandle();
- mComposerStates[sc] = s;
+ s.state.surface = handle;
+ mComposerStates[handle] = s;
}
- return &(mComposerStates[sc].state);
+ return &(mComposerStates[handle].state);
}
void SurfaceComposerClient::Transaction::registerSurfaceControlForCallback(
diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp
index b9defdd..071314f 100644
--- a/libs/gui/SurfaceControl.cpp
+++ b/libs/gui/SurfaceControl.cpp
@@ -65,8 +65,8 @@
{
// Avoid reparenting the server-side surface to null if we are not the owner of it,
// meaning that we retrieved it from another process.
- if (mClient != nullptr && mHandle != nullptr && mOwned) {
- SurfaceComposerClient::doDropReferenceTransaction(mHandle, mClient->getClient());
+ if (mHandle != nullptr && mOwned) {
+ SurfaceComposerClient::doDropReferenceTransaction(mHandle);
}
release();
}
diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h
index f438eb3..cbd1c85 100644
--- a/libs/gui/include/gui/LayerState.h
+++ b/libs/gui/include/gui/LayerState.h
@@ -206,7 +206,6 @@
};
struct ComposerState {
- sp<ISurfaceComposerClient> client;
layer_state_t state;
status_t write(Parcel& output) const;
status_t read(const Parcel& input);
@@ -274,8 +273,6 @@
};
static inline int compare_type(const ComposerState& lhs, const ComposerState& rhs) {
- if (lhs.client < rhs.client) return -1;
- if (lhs.client > rhs.client) return 1;
if (lhs.state.surface < rhs.state.surface) return -1;
if (lhs.state.surface > rhs.state.surface) return 1;
return 0;
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 4dda97f..22ab62d 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -163,8 +163,7 @@
* Called from SurfaceControl d'tor to 'destroy' the surface (or rather, reparent it
* to null), but without needing an sp<SurfaceControl> to avoid infinite ressurection.
*/
- static void doDropReferenceTransaction(const sp<IBinder>& handle,
- const sp<ISurfaceComposerClient>& client);
+ static void doDropReferenceTransaction(const sp<IBinder>& handle);
/**
* Uncaches a buffer in ISurfaceComposer. It must be uncached via a transaction so that it is
@@ -270,6 +269,12 @@
}
};
+ struct IBinderHash {
+ std::size_t operator()(const sp<IBinder>& iBinder) const {
+ return std::hash<IBinder*>{}(iBinder.get());
+ }
+ };
+
struct TCLHash {
std::size_t operator()(const sp<ITransactionCompletedListener>& tcl) const {
return std::hash<IBinder*>{}((tcl) ? IInterface::asBinder(tcl).get() : nullptr);
@@ -286,7 +291,7 @@
};
class Transaction : Parcelable {
- std::unordered_map<sp<SurfaceControl>, ComposerState, SCHash> mComposerStates;
+ std::unordered_map<sp<IBinder>, ComposerState, IBinderHash> mComposerStates;
SortedVector<DisplayState > mDisplayStates;
std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>
mListenerCallbacks;
@@ -314,7 +319,10 @@
InputWindowCommands mInputWindowCommands;
int mStatus = NO_ERROR;
- layer_state_t* getLayerState(const sp<SurfaceControl>& sc);
+ layer_state_t* getLayerState(const sp<IBinder>& surfaceHandle);
+ layer_state_t* getLayerState(const sp<SurfaceControl>& sc) {
+ return getLayerState(sc->getHandle());
+ }
DisplayState& getDisplayState(const sp<IBinder>& token);
void cacheBuffers();
@@ -560,15 +568,10 @@
CallbackId mCallbackIdCounter GUARDED_BY(mMutex) = 1;
- struct IBinderHash {
- std::size_t operator()(const sp<IBinder>& iBinder) const {
- return std::hash<IBinder*>{}(iBinder.get());
- }
- };
-
struct CallbackTranslation {
TransactionCompletedCallback callbackFunction;
- std::unordered_map<sp<IBinder>, sp<SurfaceControl>, IBinderHash> surfaceControls;
+ std::unordered_map<sp<IBinder>, sp<SurfaceControl>, SurfaceComposerClient::IBinderHash>
+ surfaceControls;
};
std::unordered_map<CallbackId, CallbackTranslation> mCallbacks GUARDED_BY(mMutex);
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index f1e3971..9c612b4 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3655,27 +3655,6 @@
return !mTransactionQueues.empty();
}
-bool SurfaceFlinger::containsAnyInvalidClientState(const Vector<ComposerState>& states) {
- for (const ComposerState& state : states) {
- // Here we need to check that the interface we're given is indeed
- // one of our own. A malicious client could give us a nullptr
- // IInterface, or one of its own or even one of our own but a
- // different type. All these situations would cause us to crash.
- if (state.client == nullptr) {
- return true;
- }
-
- sp<IBinder> binder = IInterface::asBinder(state.client);
- if (binder == nullptr) {
- return true;
- }
-
- if (binder->queryLocalInterface(ISurfaceComposerClient::descriptor) == nullptr) {
- return true;
- }
- }
- return false;
-}
bool SurfaceFlinger::transactionIsReadyToBeApplied(int64_t desiredPresentTime,
const Vector<ComposerState>& states) {
@@ -3714,10 +3693,6 @@
Mutex::Autolock _l(mStateLock);
- if (containsAnyInvalidClientState(states)) {
- return;
- }
-
// If its TransactionQueue already has a pending TransactionState or if it is pending
auto itr = mTransactionQueues.find(applyToken);
// if this is an animation frame, wait until prior animation frame has
@@ -3929,9 +3904,8 @@
const std::vector<ListenerCallbacks>& listenerCallbacks, int64_t postTime,
bool privileged) {
const layer_state_t& s = composerState.state;
- sp<Client> client(static_cast<Client*>(composerState.client.get()));
- sp<Layer> layer(client->getLayerUser(s.surface));
+ sp<Layer> layer(fromHandle(s.surface));
if (layer == nullptr) {
for (auto& listenerCallback : listenerCallbacks) {
mTransactionCompletedThread.registerUnpresentedCallbackHandle(
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index fa801af..30b6b69 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -582,7 +582,6 @@
void latchAndReleaseBuffer(const sp<Layer>& layer);
void commitTransaction() REQUIRES(mStateLock);
void commitOffscreenLayers();
- bool containsAnyInvalidClientState(const Vector<ComposerState>& states);
bool transactionIsReadyToBeApplied(int64_t desiredPresentTime,
const Vector<ComposerState>& states);
uint32_t setClientStateLocked(const ComposerState& composerState, int64_t desiredPresentTime,