Move PointerController from sp to shared_ptr

Bug: 160010896
Test: atest PointerController_test, manual usage
Change-Id: I4e665d00c56b44c9c1a4ea8cb27ffd10ade3315b
Merged-In: I4e665d00c56b44c9c1a4ea8cb27ffd10ade3315b

libs/input/PointerController.cpp

@@ -24,37 +24,10 @@
 
 #include <log/log.h>
 
-#include <SkBitmap.h>
+#include <memory>
-#include <SkCanvas.h>
-#include <SkColor.h>
-#include <SkPaint.h>
-#include <SkBlendMode.h>
 
 namespace android {
 
-// --- WeakLooperCallback ---
-
-class WeakLooperCallback: public LooperCallback {
-protected:
-    virtual ~WeakLooperCallback() { }
-
-public:
-    explicit WeakLooperCallback(const wp<LooperCallback>& callback) :
-        mCallback(callback) {
-    }
-
-    virtual int handleEvent(int fd, int events, void* data) {
-        sp<LooperCallback> callback = mCallback.promote();
-        if (callback != NULL) {
-            return callback->handleEvent(fd, events, data);
-        }
-        return 0; // the client is gone, remove the callback
-    }
-
-private:
-    wp<LooperCallback> mCallback;
-};
-
 // --- PointerController ---
 
 // Time to wait before starting the fade when the pointer is inactive.
@@ -70,21 +43,42 @@ static const nsecs_t POINTER_FADE_DURATION = 500 * 1000000LL; // 500 ms
 // The number of events to be read at once for DisplayEventReceiver.
 static const int EVENT_BUFFER_SIZE = 100;
 
-// --- PointerController ---
+std::shared_ptr<PointerController> PointerController::create(
+        const sp<PointerControllerPolicyInterface>& policy, const sp<Looper>& looper,
+        const sp<SpriteController>& spriteController) {
+    std::shared_ptr<PointerController> controller = std::shared_ptr<PointerController>(
+            new PointerController(policy, looper, spriteController));
 
-PointerController::PointerController(const sp<PointerControllerPolicyInterface>& policy,
+    /*
-        const sp<Looper>& looper, const sp<SpriteController>& spriteController) :
+     * Now we need to hook up the constructed PointerController object to its callbacks.
-        mPolicy(policy), mLooper(looper), mSpriteController(spriteController) {
+     *
-    mHandler = new WeakMessageHandler(this);
+     * This must be executed after the constructor but before any other methods on PointerController
-    mCallback = new WeakLooperCallback(this);
+     * in order to ensure that the fully constructed object is visible on the Looper thread, since
+     * that may be a different thread than where the PointerController is initially constructed.
+     *
+     * Unfortunately, this cannot be done as part of the constructor since we need to hand out
+     * weak_ptr's which themselves cannot be constructed until there's at least one shared_ptr.
+     */
 
-    if (mDisplayEventReceiver.initCheck() == NO_ERROR) {
+    controller->mHandler->pointerController = controller;
-        mLooper->addFd(mDisplayEventReceiver.getFd(), Looper::POLL_CALLBACK,
+    controller->mCallback->pointerController = controller;
-                       Looper::EVENT_INPUT, mCallback, nullptr);
+    if (controller->mDisplayEventReceiver.initCheck() == NO_ERROR) {
+        controller->mLooper->addFd(controller->mDisplayEventReceiver.getFd(), Looper::POLL_CALLBACK,
+                                   Looper::EVENT_INPUT, controller->mCallback, nullptr);
     } else {
         ALOGE("Failed to initialize DisplayEventReceiver.");
     }
+    return controller;
+}
 
+PointerController::PointerController(const sp<PointerControllerPolicyInterface>& policy,
+                                     const sp<Looper>& looper,
+                                     const sp<SpriteController>& spriteController)
+      : mPolicy(policy),
+        mLooper(looper),
+        mSpriteController(spriteController),
+        mHandler(new MessageHandler()),
+        mCallback(new LooperCallback()) {
     AutoMutex _l(mLock);
 
     mLocked.animationPending = false;
@@ -486,24 +480,35 @@ void PointerController::setCustomPointerIcon(const SpriteIcon& icon) {
     updatePointerLocked();
 }
 
-void PointerController::handleMessage(const Message& message) {
+void PointerController::MessageHandler::handleMessage(const Message& message) {
+    std::shared_ptr<PointerController> controller = pointerController.lock();
+
+    if (controller == nullptr) {
+        ALOGE("PointerController instance was released before processing message: what=%d",
+              message.what);
+        return;
+    }
     switch (message.what) {
     case MSG_INACTIVITY_TIMEOUT:
-        doInactivityTimeout();
+        controller->doInactivityTimeout();
         break;
     }
 }
 
-int PointerController::handleEvent(int /* fd */, int events, void* /* data */) {
+int PointerController::LooperCallback::handleEvent(int /* fd */, int events, void* /* data */) {
+    std::shared_ptr<PointerController> controller = pointerController.lock();
+    if (controller == nullptr) {
+        ALOGW("PointerController instance was released with pending callbacks.  events=0x%x",
+              events);
+        return 0; // Remove the callback, the PointerController is gone anyways
+    }
     if (events & (Looper::EVENT_ERROR | Looper::EVENT_HANGUP)) {
-        ALOGE("Display event receiver pipe was closed or an error occurred.  "
+        ALOGE("Display event receiver pipe was closed or an error occurred.  events=0x%x", events);
-              "events=0x%x", events);
         return 0; // remove the callback
     }
 
     if (!(events & Looper::EVENT_INPUT)) {
-        ALOGW("Received spurious callback for unhandled poll event.  "
+        ALOGW("Received spurious callback for unhandled poll event.  events=0x%x", events);
-              "events=0x%x", events);
         return 1; // keep the callback
     }
 
@@ -511,7 +516,7 @@ int PointerController::handleEvent(int /* fd */, int events, void* /* data */) {
     ssize_t n;
     nsecs_t timestamp;
     DisplayEventReceiver::Event buf[EVENT_BUFFER_SIZE];
-    while ((n = mDisplayEventReceiver.getEvents(buf, EVENT_BUFFER_SIZE)) > 0) {
+    while ((n = controller->mDisplayEventReceiver.getEvents(buf, EVENT_BUFFER_SIZE)) > 0) {
         for (size_t i = 0; i < static_cast<size_t>(n); ++i) {
             if (buf[i].header.type == DisplayEventReceiver::DISPLAY_EVENT_VSYNC) {
                 timestamp = buf[i].header.timestamp;
@@ -520,7 +525,7 @@ int PointerController::handleEvent(int /* fd */, int events, void* /* data */) {
         }
     }
     if (gotVsync) {
-        doAnimate(timestamp);
+        controller->doAnimate(timestamp);
     }
     return 1;  // keep the callback
 }
@@ -737,7 +742,7 @@ PointerController::Spot* PointerController::removeFirstFadingSpotLocked(std::vec
             return spot;
         }
     }
-    return NULL;
+    return nullptr;
 }
 
 void PointerController::releaseSpotLocked(Spot* spot) {

libs/input/PointerController.h

@@ -17,19 +17,20 @@
 #ifndef _UI_POINTER_CONTROLLER_H
 #define _UI_POINTER_CONTROLLER_H
 
-#include "SpriteController.h"
+#include <PointerControllerInterface.h>
-
+#include <gui/DisplayEventReceiver.h>
-#include <map>
-#include <vector>
-
-#include <ui/DisplayInfo.h>
 #include <input/DisplayViewport.h>
 #include <input/Input.h>
-#include <PointerControllerInterface.h>
+#include <ui/DisplayInfo.h>
 #include <utils/BitSet.h>
-#include <utils/RefBase.h>
 #include <utils/Looper.h>
-#include <gui/DisplayEventReceiver.h>
+#include <utils/RefBase.h>
+
+#include <map>
+#include <memory>
+#include <vector>
+
+#include "SpriteController.h"
 
 namespace android {
 
@@ -70,25 +71,22 @@ public:
     virtual int32_t getCustomPointerIconId() = 0;
 };
 
-
 /*
  * Tracks pointer movements and draws the pointer sprite to a surface.
  *
  * Handles pointer acceleration and animation.
  */
-class PointerController : public PointerControllerInterface, public MessageHandler,
+class PointerController : public PointerControllerInterface {
-                          public LooperCallback {
-protected:
-    virtual ~PointerController();
-
 public:
+    static std::shared_ptr<PointerController> create(
+            const sp<PointerControllerPolicyInterface>& policy, const sp<Looper>& looper,
+            const sp<SpriteController>& spriteController);
     enum InactivityTimeout {
         INACTIVITY_TIMEOUT_NORMAL = 0,
         INACTIVITY_TIMEOUT_SHORT = 1,
     };
 
-    PointerController(const sp<PointerControllerPolicyInterface>& policy,
+    virtual ~PointerController();
-            const sp<Looper>& looper, const sp<SpriteController>& spriteController);
 
     virtual bool getBounds(float* outMinX, float* outMinY,
             float* outMaxX, float* outMaxY) const;
@@ -113,8 +111,8 @@ public:
     void reloadPointerResources();
 
 private:
-    static const size_t MAX_RECYCLED_SPRITES = 12;
+    static constexpr size_t MAX_RECYCLED_SPRITES = 12;
-    static const size_t MAX_SPOTS = 12;
+    static constexpr size_t MAX_SPOTS = 12;
 
     enum {
         MSG_INACTIVITY_TIMEOUT,
@@ -130,8 +128,13 @@ private:
         float x, y;
 
         inline Spot(uint32_t id, const sp<Sprite>& sprite)
-                : id(id), sprite(sprite), alpha(1.0f), scale(1.0f),
+              : id(id),
-                  x(0.0f), y(0.0f), lastIcon(NULL) { }
+                sprite(sprite),
+                alpha(1.0f),
+                scale(1.0f),
+                x(0.0f),
+                y(0.0f),
+                lastIcon(nullptr) {}
 
         void updateSprite(const SpriteIcon* icon, float x, float y, int32_t displayId);
 
@@ -139,12 +142,24 @@ private:
         const SpriteIcon* lastIcon;
     };
 
+    class MessageHandler : public virtual android::MessageHandler {
+    public:
+        void handleMessage(const Message& message) override;
+        std::weak_ptr<PointerController> pointerController;
+    };
+
+    class LooperCallback : public virtual android::LooperCallback {
+    public:
+        int handleEvent(int fd, int events, void* data) override;
+        std::weak_ptr<PointerController> pointerController;
+    };
+
     mutable Mutex mLock;
 
     sp<PointerControllerPolicyInterface> mPolicy;
     sp<Looper> mLooper;
     sp<SpriteController> mSpriteController;
-    sp<WeakMessageHandler> mHandler;
+    sp<MessageHandler> mHandler;
     sp<LooperCallback> mCallback;
 
     DisplayEventReceiver mDisplayEventReceiver;
@@ -181,14 +196,15 @@ private:
         int32_t buttonState;
 
         std::map<int32_t /* displayId */, std::vector<Spot*>> spotsByDisplay;
-        std::vector<sp<Sprite> > recycledSprites;
+        std::vector<sp<Sprite>> recycledSprites;
     } mLocked GUARDED_BY(mLock);
 
+    PointerController(const sp<PointerControllerPolicyInterface>& policy, const sp<Looper>& looper,
+                      const sp<SpriteController>& spriteController);
+
     bool getBoundsLocked(float* outMinX, float* outMinY, float* outMaxX, float* outMaxY) const;
     void setPositionLocked(float x, float y);
 
-    void handleMessage(const Message& message);
-    int handleEvent(int fd, int events, void* data);
     void doAnimate(nsecs_t timestamp);
     bool doFadingAnimationLocked(nsecs_t timestamp);
     bool doBitmapAnimationLocked(nsecs_t timestamp);

libs/input/tests/PointerController_test.cpp

@@ -136,7 +136,7 @@ protected:
     sp<MockSprite> mPointerSprite;
     sp<MockPointerControllerPolicyInterface> mPolicy;
     sp<MockSpriteController> mSpriteController;
-    sp<PointerController> mPointerController;
+    std::shared_ptr<PointerController> mPointerController;
 
 private:
     void loopThread();
@@ -160,7 +160,7 @@ PointerControllerTest::PointerControllerTest() : mPointerSprite(new NiceMock<Moc
     EXPECT_CALL(*mSpriteController, createSprite())
             .WillOnce(Return(mPointerSprite));
 
-    mPointerController = new PointerController(mPolicy, mLooper, mSpriteController);
+    mPointerController = PointerController::create(mPolicy, mLooper, mSpriteController);
 }
 
 PointerControllerTest::~PointerControllerTest() {

services/core/jni/com_android_server_input_InputManagerService.cpp

@@ -233,7 +233,7 @@ public:
     /* --- InputReaderPolicyInterface implementation --- */
 
     virtual void getReaderConfiguration(InputReaderConfiguration* outConfig);
-    virtual sp<PointerControllerInterface> obtainPointerController(int32_t deviceId);
+    virtual std::shared_ptr<PointerControllerInterface> obtainPointerController(int32_t deviceId);
     virtual void notifyInputDevicesChanged(const std::vector<InputDeviceInfo>& inputDevices);
     virtual sp<KeyCharacterMap> getKeyboardLayoutOverlay(const InputDeviceIdentifier& identifier);
     virtual std::string getDeviceAlias(const InputDeviceIdentifier& identifier);
@@ -306,7 +306,7 @@ private:
         sp<SpriteController> spriteController;
 
         // Pointer controller singleton, created and destroyed as needed.
-        wp<PointerController> pointerController;
+        std::weak_ptr<PointerController> pointerController;
 
         // Input devices to be disabled
         SortedVector<int32_t> disabledInputDevices;
@@ -551,15 +551,16 @@ void NativeInputManager::getReaderConfiguration(InputReaderConfiguration* outCon
     } // release lock
 }
 
-sp<PointerControllerInterface> NativeInputManager::obtainPointerController(int32_t /* deviceId */) {
+std::shared_ptr<PointerControllerInterface> NativeInputManager::obtainPointerController(
+        int32_t /* deviceId */) {
     ATRACE_CALL();
     AutoMutex _l(mLock);
 
-    sp<PointerController> controller = mLocked.pointerController.promote();
+    std::shared_ptr<PointerController> controller = mLocked.pointerController.lock();
     if (controller == nullptr) {
         ensureSpriteControllerLocked();
 
-        controller = new PointerController(this, mLooper, mLocked.spriteController);
+        controller = PointerController::create(this, mLooper, mLocked.spriteController);
         mLocked.pointerController = controller;
         updateInactivityTimeoutLocked();
     }
@@ -840,7 +841,7 @@ void NativeInputManager::setSystemUiVisibility(int32_t visibility) {
 }
 
 void NativeInputManager::updateInactivityTimeoutLocked() REQUIRES(mLock) {
-    sp<PointerController> controller = mLocked.pointerController.promote();
+    std::shared_ptr<PointerController> controller = mLocked.pointerController.lock();
     if (controller == nullptr) {
         return;
     }
@@ -928,7 +929,7 @@ void NativeInputManager::reloadCalibration() {
 
 void NativeInputManager::setPointerIconType(int32_t iconId) {
     AutoMutex _l(mLock);
-    sp<PointerController> controller = mLocked.pointerController.promote();
+    std::shared_ptr<PointerController> controller = mLocked.pointerController.lock();
     if (controller != nullptr) {
         controller->updatePointerIcon(iconId);
     }
@@ -936,7 +937,7 @@ void NativeInputManager::setPointerIconType(int32_t iconId) {
 
 void NativeInputManager::reloadPointerIcons() {
     AutoMutex _l(mLock);
-    sp<PointerController> controller = mLocked.pointerController.promote();
+    std::shared_ptr<PointerController> controller = mLocked.pointerController.lock();
     if (controller != nullptr) {
         controller->reloadPointerResources();
     }
@@ -944,7 +945,7 @@ void NativeInputManager::reloadPointerIcons() {
 
 void NativeInputManager::setCustomPointerIcon(const SpriteIcon& icon) {
     AutoMutex _l(mLock);
-    sp<PointerController> controller = mLocked.pointerController.promote();
+    std::shared_ptr<PointerController> controller = mLocked.pointerController.lock();
     if (controller != nullptr) {
         controller->setCustomPointerIcon(icon);
     }