SF: Clean up HWC2::Layer ownership

Changes the HWC2::Display::createLayer() call to return a shared_ptr
instead of a bare pointer. Also if the HWC2::Display is disconnected,
instead of directly destroying the HWC2::Layers associated with the
display, a new call is made to clear the display information from the
layer. For safety, checks are added to take an early out if the display
information isn't set for any call where it is used.

The CompositionEngine code was already creating a shared_ptr for the
createLayer call, and expecting the pointer to be valid until it
released it via destroyLayer.

However a hotplug disconnect could leave the CompositionEngine code
holding an invalid pointer until the next display refresh, and it could
lead to bad memory accesses if the pointer was dereferenced.
CompositionEngine itself did not do so -- it released its ownership of
the layer (and the associated display) without accessing them. This
seems to have only affected "dumpsys SurfaceFlinger", if it happened to
be executed after the disconnect, and before another refresh, and only
because it tried to print out the HWC layer id.

Bug: 181061001
Test: libsurfaceflinger_unittest
Test: libcompositionengine_test
Test: dumpsys SurfaceFlinger # after hotplug event

Change-Id: I508d6aa8ef7a6af848dd54198408d5f311175070
Merged-In: I508d6aa8ef7a6af848dd54198408d5f311175070

11 files changed, 341 insertions, 103 deletions

diff --git a/services/surfaceflinger/CompositionEngine/src/Display.cpp b/services/surfaceflinger/CompositionEngine/src/Display.cpp
index 8c8331c757..953eb76b63 100644
--- a/services/surfaceflinger/CompositionEngine/src/Display.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Display.cpp
@@ -192,16 +192,7 @@ std::unique_ptr<compositionengine::OutputLayer> Display::createOutputLayer(
     if (const auto halDisplayId = HalDisplayId::tryCast(mId);
         outputLayer && !mIsDisconnected && halDisplayId) {
         auto& hwc = getCompositionEngine().getHwComposer();
-        // Note: For the moment we ensure it is safe to take a reference to the
-        // HWComposer implementation by destroying all the OutputLayers (and
-        // hence the HWC2::Layers they own) before setting a new HWComposer. See
-        // for example SurfaceFlinger::updateVrFlinger().
-        // TODO(b/121291683): Make this safer.
-        auto hwcLayer =
-                std::shared_ptr<HWC2::Layer>(hwc.createLayer(*halDisplayId),
-                                             [&hwc, id = *halDisplayId](HWC2::Layer* layer) {
-                                                 hwc.destroyLayer(id, layer);
-                                             });
+        auto hwcLayer = hwc.createLayer(*halDisplayId);
         ALOGE_IF(!hwcLayer, "Failed to create a HWC layer for a HWC supported display %s",
                  getName().c_str());
         outputLayer->setHwcLayer(std::move(hwcLayer));
diff --git a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
index e63d09a00f..e12cb57feb 100644
--- a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
@@ -533,16 +533,15 @@ using DisplayCreateOutputLayerTest = FullDisplayImplTestCommon;
 
 TEST_F(DisplayCreateOutputLayerTest, setsHwcLayer) {
     sp<mock::LayerFE> layerFE = new StrictMock<mock::LayerFE>();
-    StrictMock<HWC2::mock::Layer> hwcLayer;
+    auto hwcLayer = std::make_shared<StrictMock<HWC2::mock::Layer>>();
 
     EXPECT_CALL(mHwComposer, createLayer(HalDisplayId(DEFAULT_DISPLAY_ID)))
-            .WillOnce(Return(&hwcLayer));
+            .WillOnce(Return(hwcLayer));
 
     auto outputLayer = mDisplay->createOutputLayer(layerFE);
 
-    EXPECT_EQ(&hwcLayer, outputLayer->getHwcLayer());
+    EXPECT_EQ(hwcLayer.get(), outputLayer->getHwcLayer());
 
-    EXPECT_CALL(mHwComposer, destroyLayer(HalDisplayId(DEFAULT_DISPLAY_ID), &hwcLayer));
     outputLayer.reset();
 }
 
diff --git a/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h b/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
index 74d4b92343..cd2d09e3db 100644
--- a/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
+++ b/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
@@ -48,8 +48,7 @@ public:
     MOCK_METHOD3(allocateVirtualDisplay,
                  std::optional<DisplayId>(uint32_t, uint32_t, ui::PixelFormat*));
     MOCK_METHOD2(allocatePhysicalDisplay, void(hal::HWDisplayId, PhysicalDisplayId));
-    MOCK_METHOD1(createLayer, HWC2::Layer*(HalDisplayId));
-    MOCK_METHOD2(destroyLayer, void(HalDisplayId, HWC2::Layer*));
+    MOCK_METHOD1(createLayer, std::shared_ptr<HWC2::Layer>(HalDisplayId));
     MOCK_METHOD4(getDeviceCompositionChanges,
                  status_t(HalDisplayId, bool, std::chrono::steady_clock::time_point,
                           std::optional<android::HWComposer::DeviceRequestedChanges>*));
diff --git a/services/surfaceflinger/DisplayHardware/HWC2.cpp b/services/surfaceflinger/DisplayHardware/HWC2.cpp
index d04b5f7316..27146ab79c 100644
--- a/services/surfaceflinger/DisplayHardware/HWC2.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWC2.cpp
@@ -78,7 +78,19 @@ Display::Display(android::Hwc2::Composer& composer,
 }
 
 Display::~Display() {
-    mLayers.clear();
+    // Note: The calls to onOwningDisplayDestroyed() are allowed (and expected)
+    // to call Display::onLayerDestroyed(). As that call removes entries from
+    // mLayers, we do not want to have a for loop directly over it here. Since
+    // the end goal is an empty mLayers anyway, we just go ahead and swap an
+    // initially empty local container with mLayers, and then enumerate
+    // the contents of the local container.
+    Layers destroyingLayers;
+    std::swap(mLayers, destroyingLayers);
+    for (const auto& [_, weakLayer] : destroyingLayers) {
+        if (std::shared_ptr layer = weakLayer.lock()) {
+            layer->onOwningDisplayDestroyed();
+        }
+    }
 
     Error error = Error::NONE;
     const char* msg;
@@ -110,29 +122,21 @@ Error Display::acceptChanges()
     return static_cast<Error>(intError);
 }
 
-Error Display::createLayer(HWC2::Layer** outLayer) {
-    if (!outLayer) {
-        return Error::BAD_PARAMETER;
-    }
+base::expected<std::shared_ptr<HWC2::Layer>, hal::Error> Display::createLayer() {
     HWLayerId layerId = 0;
     auto intError = mComposer.createLayer(mId, &layerId);
     auto error = static_cast<Error>(intError);
     if (error != Error::NONE) {
-        return error;
+        return base::unexpected(error);
     }
 
-    auto layer = std::make_unique<impl::Layer>(mComposer, mCapabilities, mId, layerId);
-    *outLayer = layer.get();
-    mLayers.emplace(layerId, std::move(layer));
-    return Error::NONE;
+    auto layer = std::make_shared<impl::Layer>(mComposer, mCapabilities, *this, layerId);
+    mLayers.emplace(layerId, layer);
+    return layer;
 }
 
-Error Display::destroyLayer(HWC2::Layer* layer) {
-    if (!layer) {
-        return Error::BAD_PARAMETER;
-    }
-    mLayers.erase(layer->getId());
-    return Error::NONE;
+void Display::onLayerDestroyed(hal::HWLayerId layerId) {
+    mLayers.erase(layerId);
 }
 
 bool Display::isVsyncPeriodSwitchSupported() const {
@@ -161,7 +165,7 @@ Error Display::getChangedCompositionTypes(std::unordered_map<HWC2::Layer*, Compo
             auto type = types[element];
             ALOGV("getChangedCompositionTypes: adding %" PRIu64 " %s",
                     layer->getId(), to_string(type).c_str());
-            outTypes->emplace(layer, type);
+            outTypes->emplace(layer.get(), type);
         } else {
             ALOGE("getChangedCompositionTypes: invalid layer %" PRIu64 " found"
                     " on display %" PRIu64, layerIds[element], mId);
@@ -254,7 +258,7 @@ Error Display::getRequests(HWC2::DisplayRequest* outDisplayRequests,
         if (layer) {
             auto layerRequest =
                     static_cast<LayerRequest>(layerRequests[element]);
-            outLayerRequests->emplace(layer, layerRequest);
+            outLayerRequests->emplace(layer.get(), layerRequest);
         } else {
             ALOGE("getRequests: invalid layer %" PRIu64 " found on display %"
                     PRIu64, layerIds[element], mId);
@@ -340,7 +344,7 @@ Error Display::getReleaseFences(std::unordered_map<HWC2::Layer*, sp<Fence>>* out
         auto layer = getLayerById(layerIds[element]);
         if (layer) {
             sp<Fence> fence(new Fence(fenceFds[element]));
-            releaseFences.emplace(layer, fence);
+            releaseFences.emplace(layer.get(), fence);
         } else {
             ALOGE("getReleaseFences: invalid layer %" PRIu64
                     " found on display %" PRIu64, layerIds[element], mId);
@@ -550,12 +554,9 @@ void Display::setConnected(bool connected) {
 
 // Other Display methods
 
-HWC2::Layer* Display::getLayerById(HWLayerId id) const {
-    if (mLayers.count(id) == 0) {
-        return nullptr;
-    }
-
-    return mLayers.at(id).get();
+std::shared_ptr<HWC2::Layer> Display::getLayerById(HWLayerId id) const {
+    auto it = mLayers.find(id);
+    return it != mLayers.end() ? it->second.lock() : nullptr;
 }
 } // namespace impl
 
@@ -566,47 +567,78 @@ Layer::~Layer() = default;
 namespace impl {
 
 Layer::Layer(android::Hwc2::Composer& composer, const std::unordered_set<Capability>& capabilities,
-             HWDisplayId displayId, HWLayerId layerId)
+             HWC2::Display& display, HWLayerId layerId)
       : mComposer(composer),
         mCapabilities(capabilities),
-        mDisplayId(displayId),
+        mDisplay(&display),
         mId(layerId),
         mColorMatrix(android::mat4()) {
-    ALOGV("Created layer %" PRIu64 " on display %" PRIu64, layerId, displayId);
+    ALOGV("Created layer %" PRIu64 " on display %" PRIu64, layerId, display.getId());
 }
 
 Layer::~Layer()
 {
-    auto intError = mComposer.destroyLayer(mDisplayId, mId);
+    onOwningDisplayDestroyed();
+}
+
+void Layer::onOwningDisplayDestroyed() {
+    // Note: onOwningDisplayDestroyed() may be called to perform cleanup by
+    // either the Layer dtor or by the Display dtor and must be safe to call
+    // from either path. In particular, the call to Display::onLayerDestroyed()
+    // is expected to be safe to do,
+
+    if (CC_UNLIKELY(!mDisplay)) {
+        return;
+    }
+
+    mDisplay->onLayerDestroyed(mId);
+
+    // Note: If the HWC display was actually disconnected, these calls are will
+    // return an error. We always make them as there may be other reasons for
+    // the HWC2::Display to be destroyed.
+    auto intError = mComposer.destroyLayer(mDisplay->getId(), mId);
     auto error = static_cast<Error>(intError);
     ALOGE_IF(error != Error::NONE,
              "destroyLayer(%" PRIu64 ", %" PRIu64 ")"
              " failed: %s (%d)",
-             mDisplayId, mId, to_string(error).c_str(), intError);
+             mDisplay->getId(), mId, to_string(error).c_str(), intError);
+
+    mDisplay = nullptr;
 }
 
 Error Layer::setCursorPosition(int32_t x, int32_t y)
 {
-    auto intError = mComposer.setCursorPosition(mDisplayId, mId, x, y);
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
+    auto intError = mComposer.setCursorPosition(mDisplay->getId(), mId, x, y);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setBuffer(uint32_t slot, const sp<GraphicBuffer>& buffer,
         const sp<Fence>& acquireFence)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     if (buffer == nullptr && mBufferSlot == slot) {
         return Error::NONE;
     }
     mBufferSlot = slot;
 
     int32_t fenceFd = acquireFence->dup();
-    auto intError = mComposer.setLayerBuffer(mDisplayId, mId, slot, buffer,
-                                             fenceFd);
+    auto intError = mComposer.setLayerBuffer(mDisplay->getId(), mId, slot, buffer, fenceFd);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setSurfaceDamage(const Region& damage)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     if (damage.isRect() && mDamageRegion.isRect() &&
         (damage.getBounds() == mDamageRegion.getBounds())) {
         return Error::NONE;
@@ -617,8 +649,8 @@ Error Layer::setSurfaceDamage(const Region& damage)
     // rects for HWC
     Hwc2::Error intError = Hwc2::Error::NONE;
     if (damage.isRect() && damage.getBounds() == Rect::INVALID_RECT) {
-        intError = mComposer.setLayerSurfaceDamage(mDisplayId,
-                mId, std::vector<Hwc2::IComposerClient::Rect>());
+        intError = mComposer.setLayerSurfaceDamage(mDisplay->getId(), mId,
+                                                   std::vector<Hwc2::IComposerClient::Rect>());
     } else {
         size_t rectCount = 0;
         auto rectArray = damage.getArray(&rectCount);
@@ -629,7 +661,7 @@ Error Layer::setSurfaceDamage(const Region& damage)
                     rectArray[rect].right, rectArray[rect].bottom});
         }
 
-        intError = mComposer.setLayerSurfaceDamage(mDisplayId, mId, hwcRects);
+        intError = mComposer.setLayerSurfaceDamage(mDisplay->getId(), mId, hwcRects);
     }
 
     return static_cast<Error>(intError);
@@ -637,34 +669,54 @@ Error Layer::setSurfaceDamage(const Region& damage)
 
 Error Layer::setBlendMode(BlendMode mode)
 {
-    auto intError = mComposer.setLayerBlendMode(mDisplayId, mId, mode);
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
+    auto intError = mComposer.setLayerBlendMode(mDisplay->getId(), mId, mode);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setColor(Color color) {
-    auto intError = mComposer.setLayerColor(mDisplayId, mId, color);
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
+    auto intError = mComposer.setLayerColor(mDisplay->getId(), mId, color);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setCompositionType(Composition type)
 {
-    auto intError = mComposer.setLayerCompositionType(mDisplayId, mId, type);
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
+    auto intError = mComposer.setLayerCompositionType(mDisplay->getId(), mId, type);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setDataspace(Dataspace dataspace)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     if (dataspace == mDataSpace) {
         return Error::NONE;
     }
     mDataSpace = dataspace;
-    auto intError = mComposer.setLayerDataspace(mDisplayId, mId, mDataSpace);
+    auto intError = mComposer.setLayerDataspace(mDisplay->getId(), mId, mDataSpace);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setPerFrameMetadata(const int32_t supportedPerFrameMetadata,
         const android::HdrMetadata& metadata)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     if (metadata == mHdrMetadata) {
         return Error::NONE;
     }
@@ -705,7 +757,7 @@ Error Layer::setPerFrameMetadata(const int32_t supportedPerFrameMetadata,
     }
 
     Error error = static_cast<Error>(
-            mComposer.setLayerPerFrameMetadata(mDisplayId, mId, perFrameMetadatas));
+            mComposer.setLayerPerFrameMetadata(mDisplay->getId(), mId, perFrameMetadatas));
 
     if (validTypes & HdrMetadata::HDR10PLUS) {
         if (CC_UNLIKELY(mHdrMetadata.hdr10plus.size() == 0)) {
@@ -715,8 +767,9 @@ Error Layer::setPerFrameMetadata(const int32_t supportedPerFrameMetadata,
         std::vector<Hwc2::PerFrameMetadataBlob> perFrameMetadataBlobs;
         perFrameMetadataBlobs.push_back(
                 {Hwc2::PerFrameMetadataKey::HDR10_PLUS_SEI, mHdrMetadata.hdr10plus});
-        Error setMetadataBlobsError = static_cast<Error>(
-                mComposer.setLayerPerFrameMetadataBlobs(mDisplayId, mId, perFrameMetadataBlobs));
+        Error setMetadataBlobsError =
+                static_cast<Error>(mComposer.setLayerPerFrameMetadataBlobs(mDisplay->getId(), mId,
+                                                                           perFrameMetadataBlobs));
         if (error == Error::NONE) {
             return setMetadataBlobsError;
         }
@@ -726,46 +779,70 @@ Error Layer::setPerFrameMetadata(const int32_t supportedPerFrameMetadata,
 
 Error Layer::setDisplayFrame(const Rect& frame)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     Hwc2::IComposerClient::Rect hwcRect{frame.left, frame.top,
         frame.right, frame.bottom};
-    auto intError = mComposer.setLayerDisplayFrame(mDisplayId, mId, hwcRect);
+    auto intError = mComposer.setLayerDisplayFrame(mDisplay->getId(), mId, hwcRect);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setPlaneAlpha(float alpha)
 {
-    auto intError = mComposer.setLayerPlaneAlpha(mDisplayId, mId, alpha);
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
+    auto intError = mComposer.setLayerPlaneAlpha(mDisplay->getId(), mId, alpha);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setSidebandStream(const native_handle_t* stream)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     if (mCapabilities.count(Capability::SIDEBAND_STREAM) == 0) {
         ALOGE("Attempted to call setSidebandStream without checking that the "
                 "device supports sideband streams");
         return Error::UNSUPPORTED;
     }
-    auto intError = mComposer.setLayerSidebandStream(mDisplayId, mId, stream);
+    auto intError = mComposer.setLayerSidebandStream(mDisplay->getId(), mId, stream);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setSourceCrop(const FloatRect& crop)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     Hwc2::IComposerClient::FRect hwcRect{
         crop.left, crop.top, crop.right, crop.bottom};
-    auto intError = mComposer.setLayerSourceCrop(mDisplayId, mId, hwcRect);
+    auto intError = mComposer.setLayerSourceCrop(mDisplay->getId(), mId, hwcRect);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setTransform(Transform transform)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     auto intTransform = static_cast<Hwc2::Transform>(transform);
-    auto intError = mComposer.setLayerTransform(mDisplayId, mId, intTransform);
+    auto intError = mComposer.setLayerTransform(mDisplay->getId(), mId, intTransform);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setVisibleRegion(const Region& region)
 {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     if (region.isRect() && mVisibleRegion.isRect() &&
         (region.getBounds() == mVisibleRegion.getBounds())) {
         return Error::NONE;
@@ -781,22 +858,30 @@ Error Layer::setVisibleRegion(const Region& region)
                 rectArray[rect].right, rectArray[rect].bottom});
     }
 
-    auto intError = mComposer.setLayerVisibleRegion(mDisplayId, mId, hwcRects);
+    auto intError = mComposer.setLayerVisibleRegion(mDisplay->getId(), mId, hwcRects);
     return static_cast<Error>(intError);
 }
 
 Error Layer::setZOrder(uint32_t z)
 {
-    auto intError = mComposer.setLayerZOrder(mDisplayId, mId, z);
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
+    auto intError = mComposer.setLayerZOrder(mDisplay->getId(), mId, z);
     return static_cast<Error>(intError);
 }
 
 // Composer HAL 2.3
 Error Layer::setColorTransform(const android::mat4& matrix) {
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
     if (matrix == mColorMatrix) {
         return Error::NONE;
     }
-    auto intError = mComposer.setLayerColorTransform(mDisplayId, mId, matrix.asArray());
+    auto intError = mComposer.setLayerColorTransform(mDisplay->getId(), mId, matrix.asArray());
     Error error = static_cast<Error>(intError);
     if (error != Error::NONE) {
         return error;
@@ -808,7 +893,12 @@ Error Layer::setColorTransform(const android::mat4& matrix) {
 // Composer HAL 2.4
 Error Layer::setLayerGenericMetadata(const std::string& name, bool mandatory,
                                      const std::vector<uint8_t>& value) {
-    auto intError = mComposer.setLayerGenericMetadata(mDisplayId, mId, name, mandatory, value);
+    if (CC_UNLIKELY(!mDisplay)) {
+        return Error::BAD_DISPLAY;
+    }
+
+    auto intError =
+            mComposer.setLayerGenericMetadata(mDisplay->getId(), mId, name, mandatory, value);
     return static_cast<Error>(intError);
 }
 
diff --git a/services/surfaceflinger/DisplayHardware/HWC2.h b/services/surfaceflinger/DisplayHardware/HWC2.h
index e7bf286d08..fae95e79c3 100644
--- a/services/surfaceflinger/DisplayHardware/HWC2.h
+++ b/services/surfaceflinger/DisplayHardware/HWC2.h
@@ -16,6 +16,7 @@
 
 #pragma once
 
+#include <android-base/expected.h>
 #include <gui/HdrMetadata.h>
 #include <math/mat4.h>
 #include <ui/HdrCapabilities.h>
@@ -84,10 +85,11 @@ public:
     virtual void setConnected(bool connected) = 0; // For use by Device only
     virtual const std::unordered_set<hal::DisplayCapability>& getCapabilities() const = 0;
     virtual bool isVsyncPeriodSwitchSupported() const = 0;
+    virtual void onLayerDestroyed(hal::HWLayerId layerId) = 0;
 
     [[clang::warn_unused_result]] virtual hal::Error acceptChanges() = 0;
-    [[clang::warn_unused_result]] virtual hal::Error createLayer(Layer** outLayer) = 0;
-    [[clang::warn_unused_result]] virtual hal::Error destroyLayer(Layer* layer) = 0;
+    [[clang::warn_unused_result]] virtual base::expected<std::shared_ptr<HWC2::Layer>, hal::Error>
+    createLayer() = 0;
     [[clang::warn_unused_result]] virtual hal::Error getChangedCompositionTypes(
             std::unordered_map<Layer*, hal::Composition>* outTypes) = 0;
     [[clang::warn_unused_result]] virtual hal::Error getColorModes(
@@ -152,6 +154,8 @@ public:
 
 namespace impl {
 
+class Layer;
+
 class Display : public HWC2::Display {
 public:
     Display(android::Hwc2::Composer&, const std::unordered_set<hal::Capability>&, hal::HWDisplayId,
@@ -160,10 +164,9 @@ public:
 
     // Required by HWC2
     hal::Error acceptChanges() override;
-    hal::Error createLayer(Layer** outLayer) override;
-    hal::Error destroyLayer(Layer*) override;
+    base::expected<std::shared_ptr<HWC2::Layer>, hal::Error> createLayer() override;
     hal::Error getChangedCompositionTypes(
-            std::unordered_map<Layer*, hal::Composition>* outTypes) override;
+            std::unordered_map<HWC2::Layer*, hal::Composition>* outTypes) override;
     hal::Error getColorModes(std::vector<hal::ColorMode>* outModes) const override;
     // Returns a bitmask which contains HdrMetadata::Type::*.
     int32_t getSupportedPerFrameMetadata() const override;
@@ -174,7 +177,7 @@ public:
     hal::Error getName(std::string* outName) const override;
     hal::Error getRequests(
             hal::DisplayRequest* outDisplayRequests,
-            std::unordered_map<Layer*, hal::LayerRequest>* outLayerRequests) override;
+            std::unordered_map<HWC2::Layer*, hal::LayerRequest>* outLayerRequests) override;
     hal::Error getConnectionType(ui::DisplayConnectionType*) const override;
     hal::Error supportsDoze(bool* outSupport) const override;
     hal::Error getHdrCapabilities(android::HdrCapabilities* outCapabilities) const override;
@@ -185,8 +188,8 @@ public:
                                                 uint64_t maxFrames) const override;
     hal::Error getDisplayedContentSample(uint64_t maxFrames, uint64_t timestamp,
                                          android::DisplayedFrameStats* outStats) const override;
-    hal::Error getReleaseFences(
-            std::unordered_map<Layer*, android::sp<android::Fence>>* outFences) const override;
+    hal::Error getReleaseFences(std::unordered_map<HWC2::Layer*, android::sp<android::Fence>>*
+                                        outFences) const override;
     hal::Error present(android::sp<android::Fence>* outPresentFence) override;
     hal::Error setClientTarget(uint32_t slot, const android::sp<android::GraphicBuffer>& target,
                                const android::sp<android::Fence>& acquireFence,
@@ -218,13 +221,14 @@ public:
     const std::unordered_set<hal::DisplayCapability>& getCapabilities() const override {
         return mDisplayCapabilities;
     };
-    virtual bool isVsyncPeriodSwitchSupported() const override;
+    bool isVsyncPeriodSwitchSupported() const override;
+    void onLayerDestroyed(hal::HWLayerId layerId) override;
 
 private:
 
     // This may fail (and return a null pointer) if no layer with this ID exists
     // on this display
-    Layer* getLayerById(hal::HWLayerId) const;
+    std::shared_ptr<HWC2::Layer> getLayerById(hal::HWLayerId id) const;
 
     friend android::TestableSurfaceFlinger;
 
@@ -240,7 +244,8 @@ private:
     hal::DisplayType mType;
     bool mIsConnected = false;
 
-    std::unordered_map<hal::HWLayerId, std::unique_ptr<Layer>> mLayers;
+    using Layers = std::unordered_map<hal::HWLayerId, std::weak_ptr<HWC2::impl::Layer>>;
+    Layers mLayers;
 
     std::once_flag mDisplayCapabilityQueryFlag;
     std::unordered_set<hal::DisplayCapability> mDisplayCapabilities;
@@ -295,10 +300,12 @@ namespace impl {
 class Layer : public HWC2::Layer {
 public:
     Layer(android::Hwc2::Composer& composer,
-          const std::unordered_set<hal::Capability>& capabilities, hal::HWDisplayId displayId,
+          const std::unordered_set<hal::Capability>& capabilities, HWC2::Display& display,
           hal::HWLayerId layerId);
     ~Layer() override;
 
+    void onOwningDisplayDestroyed();
+
     hal::HWLayerId getId() const override { return mId; }
 
     hal::Error setCursorPosition(int32_t x, int32_t y) override;
@@ -334,7 +341,7 @@ private:
     android::Hwc2::Composer& mComposer;
     const std::unordered_set<hal::Capability>& mCapabilities;
 
-    hal::HWDisplayId mDisplayId;
+    HWC2::Display* mDisplay;
     hal::HWLayerId mId;
 
     // Cached HWC2 data, to ensure the same commands aren't sent to the HWC
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
index dc4839eba4..36876dc7f1 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
@@ -305,20 +305,15 @@ int32_t HWComposer::getAttribute(hal::HWDisplayId hwcDisplayId, hal::HWConfigId
     return value;
 }
 
-HWC2::Layer* HWComposer::createLayer(HalDisplayId displayId) {
+std::shared_ptr<HWC2::Layer> HWComposer::createLayer(HalDisplayId displayId) {
     RETURN_IF_INVALID_DISPLAY(displayId, nullptr);
 
-    HWC2::Layer* layer;
-    auto error = mDisplayData[displayId].hwcDisplay->createLayer(&layer);
-    RETURN_IF_HWC_ERROR(error, displayId, nullptr);
-    return layer;
-}
-
-void HWComposer::destroyLayer(HalDisplayId displayId, HWC2::Layer* layer) {
-    RETURN_IF_INVALID_DISPLAY(displayId);
-
-    auto error = mDisplayData[displayId].hwcDisplay->destroyLayer(layer);
-    RETURN_IF_HWC_ERROR(error, displayId);
+    auto expected = mDisplayData[displayId].hwcDisplay->createLayer();
+    if (!expected.has_value()) {
+        auto error = std::move(expected).error();
+        RETURN_IF_HWC_ERROR(error, displayId, nullptr);
+    }
+    return std::move(expected).value();
 }
 
 bool HWComposer::isConnected(PhysicalDisplayId displayId) const {
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h
index 5bad52973a..d0c0c1105a 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.h
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.h
@@ -116,9 +116,7 @@ public:
     virtual void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId) = 0;
 
     // Attempts to create a new layer on this display
-    virtual HWC2::Layer* createLayer(HalDisplayId) = 0;
-    // Destroy a previously created layer
-    virtual void destroyLayer(HalDisplayId, HWC2::Layer*) = 0;
+    virtual std::shared_ptr<HWC2::Layer> createLayer(HalDisplayId) = 0;
 
     // Gets any required composition change requests from the HWC device.
     //
@@ -264,9 +262,7 @@ public:
     void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId) override;
 
     // Attempts to create a new layer on this display
-    HWC2::Layer* createLayer(HalDisplayId) override;
-    // Destroy a previously created layer
-    void destroyLayer(HalDisplayId, HWC2::Layer*) override;
+    std::shared_ptr<HWC2::Layer> createLayer(HalDisplayId) override;
 
     status_t getDeviceCompositionChanges(
             HalDisplayId, bool frameUsesClientComposition,
diff --git a/services/surfaceflinger/tests/unittests/Android.bp b/services/surfaceflinger/tests/unittests/Android.bp
index 88fb811b52..b33434e34d 100644
--- a/services/surfaceflinger/tests/unittests/Android.bp
+++ b/services/surfaceflinger/tests/unittests/Android.bp
@@ -94,6 +94,7 @@ cc_test {
         "VSyncReactorTest.cpp",
         "VsyncConfigurationTest.cpp",
         "mock/DisplayHardware/MockComposer.cpp",
+        "mock/DisplayHardware/MockHWC2.cpp",
         "mock/DisplayHardware/MockPowerAdvisor.cpp",
         "mock/MockEventThread.cpp",
         "mock/MockFrameTimeline.cpp",
diff --git a/services/surfaceflinger/tests/unittests/HWComposerTest.cpp b/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
index 6b8217087a..cbf8cc21bd 100644
--- a/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
@@ -37,6 +37,7 @@
 #include "DisplayHardware/HWComposer.h"
 #include "DisplayHardware/Hal.h"
 #include "mock/DisplayHardware/MockComposer.h"
+#include "mock/DisplayHardware/MockHWC2.h"
 
 // TODO(b/129481165): remove the #pragma below and fix conversion issues
 #pragma clang diagnostic pop // ignored "-Wconversion"
@@ -120,13 +121,19 @@ struct HWComposerLayerTest : public testing::Test {
     static constexpr hal::HWLayerId kLayerId = static_cast<hal::HWLayerId>(1002);
 
     HWComposerLayerTest(const std::unordered_set<hal::Capability>& capabilities)
-          : mCapabilies(capabilities) {}
+          : mCapabilies(capabilities) {
+        EXPECT_CALL(mDisplay, getId()).WillRepeatedly(Return(kDisplayId));
+    }
 
-    ~HWComposerLayerTest() override { EXPECT_CALL(*mHal, destroyLayer(kDisplayId, kLayerId)); }
+    ~HWComposerLayerTest() override {
+        EXPECT_CALL(mDisplay, onLayerDestroyed(kLayerId));
+        EXPECT_CALL(*mHal, destroyLayer(kDisplayId, kLayerId));
+    }
 
     std::unique_ptr<Hwc2::mock::Composer> mHal{new StrictMock<Hwc2::mock::Composer>()};
     const std::unordered_set<hal::Capability> mCapabilies;
-    HWC2::impl::Layer mLayer{*mHal, mCapabilies, kDisplayId, kLayerId};
+    StrictMock<HWC2::mock::Display> mDisplay;
+    HWC2::impl::Layer mLayer{*mHal, mCapabilies, mDisplay, kLayerId};
 };
 
 struct HWComposerLayerGenericMetadataTest : public HWComposerLayerTest {
@@ -176,4 +183,4 @@ TEST_F(HWComposerLayerGenericMetadataTest, forwardsSupportedMetadata) {
 }
 
 } // namespace
-} // namespace android
\ No newline at end of file
+} // namespace android
diff --git a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWC2.cpp b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWC2.cpp
new file mode 100644
index 0000000000..2647bf4f9d
--- /dev/null
+++ b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWC2.cpp
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#undef LOG_TAG
+#define LOG_TAG "MockHWC2"
+
+#include "mock/DisplayHardware/MockHWC2.h"
+
+namespace android::HWC2::mock {
+
+// Explicit default instantiation is recommended.
+Display::Display() = default;
+Display::~Display() = default;
+
+Layer::Layer() = default;
+Layer::~Layer() = default;
+
+} // namespace android::HWC2::mock
diff --git a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWC2.h b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWC2.h
new file mode 100644
index 0000000000..c3919d9310
--- /dev/null
+++ b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWC2.h
@@ -0,0 +1,122 @@
+/*
+ * Copyright 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <gmock/gmock.h>
+
+#include "DisplayHardware/HWC2.h"
+
+namespace android::HWC2::mock {
+
+class Display : public HWC2::Display {
+public:
+    Display();
+    ~Display() override;
+
+    MOCK_METHOD(hal::HWDisplayId, getId, (), (const, override));
+    MOCK_METHOD(bool, isConnected, (), (const, override));
+    MOCK_METHOD(void, setConnected, (bool), (override));
+    MOCK_METHOD(const std::unordered_set<hal::DisplayCapability> &, getCapabilities, (),
+                (const, override));
+    MOCK_METHOD(bool, isVsyncPeriodSwitchSupported, (), (const, override));
+    MOCK_METHOD(void, onLayerDestroyed, (hal::HWLayerId), (override));
+
+    MOCK_METHOD(hal::Error, acceptChanges, (), (override));
+    MOCK_METHOD((base::expected<std::shared_ptr<HWC2::Layer>, hal::Error>), createLayer, (),
+                (override));
+    MOCK_METHOD(hal::Error, getChangedCompositionTypes,
+                ((std::unordered_map<Layer *, hal::Composition> *)), (override));
+    MOCK_METHOD(hal::Error, getColorModes, (std::vector<hal::ColorMode> *), (const, override));
+    MOCK_METHOD(int32_t, getSupportedPerFrameMetadata, (), (const, override));
+    MOCK_METHOD(hal::Error, getRenderIntents, (hal::ColorMode, std::vector<hal::RenderIntent> *),
+                (const, override));
+    MOCK_METHOD(hal::Error, getDataspaceSaturationMatrix, (hal::Dataspace, android::mat4 *),
+                (override));
+    MOCK_METHOD(hal::Error, getName, (std::string *), (const, override));
+    MOCK_METHOD(hal::Error, getRequests,
+                (hal::DisplayRequest *, (std::unordered_map<Layer *, hal::LayerRequest> *)),
+                (override));
+    MOCK_METHOD(hal::Error, getConnectionType, (ui::DisplayConnectionType *), (const, override));
+    MOCK_METHOD(hal::Error, supportsDoze, (bool *), (const, override));
+    MOCK_METHOD(hal::Error, getHdrCapabilities, (android::HdrCapabilities *), (const, override));
+    MOCK_METHOD(hal::Error, getDisplayedContentSamplingAttributes,
+                (hal::PixelFormat *, hal::Dataspace *, uint8_t *), (const, override));
+    MOCK_METHOD(hal::Error, setDisplayContentSamplingEnabled, (bool, uint8_t, uint64_t),
+                (const, override));
+    MOCK_METHOD(hal::Error, getDisplayedContentSample,
+                (uint64_t, uint64_t, android::DisplayedFrameStats *), (const, override));
+    MOCK_METHOD(hal::Error, getReleaseFences,
+                ((std::unordered_map<Layer *, android::sp<android::Fence>> *)), (const, override));
+    MOCK_METHOD(hal::Error, present, (android::sp<android::Fence> *), (override));
+    MOCK_METHOD(hal::Error, setClientTarget,
+                (uint32_t, const android::sp<android::GraphicBuffer> &,
+                 const android::sp<android::Fence> &, hal::Dataspace),
+                (override));
+    MOCK_METHOD(hal::Error, setColorMode, (hal::ColorMode, hal::RenderIntent), (override));
+    MOCK_METHOD(hal::Error, setColorTransform, (const android::mat4 &, hal::ColorTransform),
+                (override));
+    MOCK_METHOD(hal::Error, setOutputBuffer,
+                (const android::sp<android::GraphicBuffer> &, const android::sp<android::Fence> &),
+                (override));
+    MOCK_METHOD(hal::Error, setPowerMode, (hal::PowerMode), (override));
+    MOCK_METHOD(hal::Error, setVsyncEnabled, (hal::Vsync), (override));
+    MOCK_METHOD(hal::Error, validate, (uint32_t *, uint32_t *), (override));
+    MOCK_METHOD(hal::Error, presentOrValidate,
+                (uint32_t *, uint32_t *, android::sp<android::Fence> *, uint32_t *), (override));
+    MOCK_METHOD(std::future<hal::Error>, setDisplayBrightness, (float), (override));
+    MOCK_METHOD(hal::Error, setActiveConfigWithConstraints,
+                (hal::HWConfigId, const hal::VsyncPeriodChangeConstraints &,
+                 hal::VsyncPeriodChangeTimeline *),
+                (override));
+    MOCK_METHOD(hal::Error, setAutoLowLatencyMode, (bool), (override));
+    MOCK_METHOD(hal::Error, getSupportedContentTypes, (std::vector<hal::ContentType> *),
+                (const, override));
+    MOCK_METHOD(hal::Error, setContentType, (hal::ContentType), (override));
+    MOCK_METHOD(hal::Error, getClientTargetProperty, (hal::ClientTargetProperty *), (override));
+};
+
+class Layer : public HWC2::Layer {
+public:
+    Layer();
+    ~Layer() override;
+
+    MOCK_METHOD(hal::HWLayerId, getId, (), (const, override));
+    MOCK_METHOD(hal::Error, setCursorPosition, (int32_t, int32_t), (override));
+    MOCK_METHOD(hal::Error, setBuffer,
+                (uint32_t, const android::sp<android::GraphicBuffer> &,
+                 const android::sp<android::Fence> &),
+                (override));
+    MOCK_METHOD(hal::Error, setSurfaceDamage, (const android::Region &), (override));
+    MOCK_METHOD(hal::Error, setBlendMode, (hal::BlendMode), (override));
+    MOCK_METHOD(hal::Error, setColor, (hal::Color), (override));
+    MOCK_METHOD(hal::Error, setCompositionType, (hal::Composition), (override));
+    MOCK_METHOD(hal::Error, setDataspace, (android::ui::Dataspace), (override));
+    MOCK_METHOD(hal::Error, setPerFrameMetadata, (const int32_t, const android::HdrMetadata &),
+                (override));
+    MOCK_METHOD(hal::Error, setDisplayFrame, (const android::Rect &), (override));
+    MOCK_METHOD(hal::Error, setPlaneAlpha, (float), (override));
+    MOCK_METHOD(hal::Error, setSidebandStream, (const native_handle_t *), (override));
+    MOCK_METHOD(hal::Error, setSourceCrop, (const android::FloatRect &), (override));
+    MOCK_METHOD(hal::Error, setTransform, (hal::Transform), (override));
+    MOCK_METHOD(hal::Error, setVisibleRegion, (const android::Region &), (override));
+    MOCK_METHOD(hal::Error, setZOrder, (uint32_t), (override));
+    MOCK_METHOD(hal::Error, setColorTransform, (const android::mat4 &), (override));
+    MOCK_METHOD(hal::Error, setLayerGenericMetadata,
+                (const std::string &, bool, const std::vector<uint8_t> &), (override));
+};
+
+} // namespace android::HWC2::mock