Merge "Add component to the input tests presubmit module" into main am: 8ffdc4b563 am: c57676394a
Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/2653928
Change-Id: I1bce9837581cc04a405c030651613806567dcec4
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/libs/sensor/SensorManager.cpp b/libs/sensor/SensorManager.cpp
index 40061cd..9f814f1 100644
--- a/libs/sensor/SensorManager.cpp
+++ b/libs/sensor/SensorManager.cpp
@@ -176,11 +176,8 @@
mSensors = mSensorServer->getSensorList(mOpPackageName);
size_t count = mSensors.size();
- if (count == 0) {
- ALOGE("Failed to get Sensor list");
- mSensorServer.clear();
- return UNKNOWN_ERROR;
- }
+ // If count is 0, mSensorList will be non-null. This is old
+ // existing behavior and callers expect this.
mSensorList =
static_cast<Sensor const**>(malloc(count * sizeof(Sensor*)));
LOG_ALWAYS_FATAL_IF(mSensorList == nullptr, "mSensorList NULL");
diff --git a/services/gpuservice/Android.bp b/services/gpuservice/Android.bp
index fba64c7..052efb6b 100644
--- a/services/gpuservice/Android.bp
+++ b/services/gpuservice/Android.bp
@@ -71,6 +71,7 @@
cc_library_shared {
name: "libgpuservice",
defaults: ["libgpuservice_production_defaults"],
+ export_include_dirs: ["include"],
srcs: [
":libgpuservice_sources",
],
diff --git a/services/gpuservice/GpuService.cpp b/services/gpuservice/GpuService.cpp
index 7b9782f..5643940 100644
--- a/services/gpuservice/GpuService.cpp
+++ b/services/gpuservice/GpuService.cpp
@@ -16,7 +16,7 @@
#define ATRACE_TAG ATRACE_TAG_GRAPHICS
-#include "GpuService.h"
+#include "gpuservice/GpuService.h"
#include <android-base/stringprintf.h>
#include <binder/IPCThreadState.h>
@@ -34,6 +34,7 @@
#include <vkjson.h>
#include <thread>
+#include <memory>
namespace android {
@@ -55,18 +56,21 @@
mGpuStats(std::make_unique<GpuStats>()),
mGpuMemTracer(std::make_unique<GpuMemTracer>()) {
- std::thread gpuMemAsyncInitThread([this]() {
+ mGpuMemAsyncInitThread = std::make_unique<std::thread>([this] (){
mGpuMem->initialize();
mGpuMemTracer->initialize(mGpuMem);
});
- gpuMemAsyncInitThread.detach();
- std::thread gpuWorkAsyncInitThread([this]() {
+ mGpuWorkAsyncInitThread = std::make_unique<std::thread>([this]() {
mGpuWork->initialize();
});
- gpuWorkAsyncInitThread.detach();
};
+GpuService::~GpuService() {
+ mGpuWorkAsyncInitThread->join();
+ mGpuMemAsyncInitThread->join();
+}
+
void GpuService::setGpuStats(const std::string& driverPackageName,
const std::string& driverVersionName, uint64_t driverVersionCode,
int64_t driverBuildTime, const std::string& appPackageName,
diff --git a/services/gpuservice/GpuService.h b/services/gpuservice/include/gpuservice/GpuService.h
similarity index 94%
rename from services/gpuservice/GpuService.h
rename to services/gpuservice/include/gpuservice/GpuService.h
index d7313d1..3e0ae66 100644
--- a/services/gpuservice/GpuService.h
+++ b/services/gpuservice/include/gpuservice/GpuService.h
@@ -24,6 +24,7 @@
#include <serviceutils/PriorityDumper.h>
#include <mutex>
+#include <thread>
#include <vector>
namespace android {
@@ -41,6 +42,7 @@
static const char* const SERVICE_NAME ANDROID_API;
GpuService() ANDROID_API;
+ ~GpuService();
protected:
status_t shellCommand(int in, int out, int err, std::vector<String16>& args) override;
@@ -86,6 +88,8 @@
std::unique_ptr<GpuMemTracer> mGpuMemTracer;
std::mutex mLock;
std::string mDeveloperDriverPath;
+ std::unique_ptr<std::thread> mGpuMemAsyncInitThread;
+ std::unique_ptr<std::thread> mGpuWorkAsyncInitThread;
};
} // namespace android
diff --git a/services/gpuservice/main_gpuservice.cpp b/services/gpuservice/main_gpuservice.cpp
index 64aafca..2002372 100644
--- a/services/gpuservice/main_gpuservice.cpp
+++ b/services/gpuservice/main_gpuservice.cpp
@@ -18,7 +18,7 @@
#include <binder/IServiceManager.h>
#include <binder/ProcessState.h>
#include <sys/resource.h>
-#include "GpuService.h"
+#include "gpuservice/GpuService.h"
using namespace android;
diff --git a/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp b/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp
index c2574a3..241b864 100644
--- a/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp
+++ b/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp
@@ -16,7 +16,7 @@
#include <fuzzbinder/libbinder_driver.h>
-#include "GpuService.h"
+#include "gpuservice/GpuService.h"
using ::android::fuzzService;
using ::android::GpuService;
diff --git a/services/gpuservice/tests/unittests/Android.bp b/services/gpuservice/tests/unittests/Android.bp
index 51642f9..c870b17 100644
--- a/services/gpuservice/tests/unittests/Android.bp
+++ b/services/gpuservice/tests/unittests/Android.bp
@@ -28,6 +28,7 @@
"GpuMemTest.cpp",
"GpuMemTracerTest.cpp",
"GpuStatsTest.cpp",
+ "GpuServiceTest.cpp",
],
header_libs: ["bpf_headers"],
shared_libs: [
@@ -45,6 +46,7 @@
"libstatslog",
"libstatspull",
"libutils",
+ "libgpuservice",
],
static_libs: [
"libgmock",
diff --git a/services/gpuservice/tests/unittests/GpuServiceTest.cpp b/services/gpuservice/tests/unittests/GpuServiceTest.cpp
new file mode 100644
index 0000000..62b3e53
--- /dev/null
+++ b/services/gpuservice/tests/unittests/GpuServiceTest.cpp
@@ -0,0 +1,52 @@
+#undef LOG_TAG
+#define LOG_TAG "gpuservice_unittest"
+
+#include "gpuservice/GpuService.h"
+
+#include <gtest/gtest.h>
+#include <log/log_main.h>
+
+#include <chrono>
+#include <thread>
+
+namespace android {
+namespace {
+
+class GpuServiceTest : public testing::Test {
+public:
+ GpuServiceTest() {
+ const ::testing::TestInfo* const test_info =
+ ::testing::UnitTest::GetInstance()->current_test_info();
+ ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name());
+ }
+
+ ~GpuServiceTest() {
+ const ::testing::TestInfo* const test_info =
+ ::testing::UnitTest::GetInstance()->current_test_info();
+ ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name());
+ }
+
+};
+
+
+/*
+* The behaviour before this test + fixes was UB caused by threads accessing deallocated memory.
+*
+* This test creates the service (which initializes the culprit threads),
+* deallocates it immediately and sleeps.
+*
+* GpuService's destructor gets called and joins the threads.
+* If we haven't crashed by the time the sleep time has elapsed, we're good
+* Let the test pass.
+*/
+TEST_F(GpuServiceTest, onInitializeShouldNotCauseUseAfterFree) {
+ sp<GpuService> service = new GpuService();
+ service.clear();
+ std::this_thread::sleep_for(std::chrono::seconds(3));
+
+ // If we haven't crashed yet due to threads accessing freed up memory, let the test pass
+ EXPECT_TRUE(true);
+}
+
+} // namespace
+} // namespace android
diff --git a/services/surfaceflinger/DisplayHardware/AidlComposerHal.cpp b/services/surfaceflinger/DisplayHardware/AidlComposerHal.cpp
index 3651231..82d8c08 100644
--- a/services/surfaceflinger/DisplayHardware/AidlComposerHal.cpp
+++ b/services/surfaceflinger/DisplayHardware/AidlComposerHal.cpp
@@ -769,6 +769,7 @@
auto status = mAidlComposerClient->executeCommands(commands, &results);
if (!status.isOk()) {
ALOGE("executeCommands failed %s", status.getDescription().c_str());
+ mWriter.reset();
return static_cast<Error>(status.getServiceSpecificError());
}
@@ -780,6 +781,7 @@
const auto index = static_cast<size_t>(cmdErr.commandIndex);
if (index < 0 || index >= commands.size()) {
ALOGE("invalid command index %zu", index);
+ mWriter.reset();
return Error::BAD_PARAMETER;
}
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index a31cdf0..905fe40 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -2398,7 +2398,16 @@
info.inputConfig |= WindowInfo::InputConfig::NOT_TOUCHABLE;
}
- info.setInputConfig(WindowInfo::InputConfig::NOT_VISIBLE, !isVisibleForInput());
+ // For compatibility reasons we let layers which can receive input
+ // receive input before they have actually submitted a buffer. Because
+ // of this we use canReceiveInput instead of isVisible to check the
+ // policy-visibility, ignoring the buffer state. However for layers with
+ // hasInputInfo()==false we can use the real visibility state.
+ // We are just using these layers for occlusion detection in
+ // InputDispatcher, and obviously if they aren't visible they can't occlude
+ // anything.
+ const bool visible = hasInputInfo() ? canReceiveInput() : isVisible();
+ info.setInputConfig(WindowInfo::InputConfig::NOT_VISIBLE, !visible);
info.alpha = getAlpha();
fillTouchOcclusionMode(info);
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 5ffcabf..f0c8ad7 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -472,21 +472,6 @@
virtual bool canReceiveInput() const;
/*
- * Whether or not the layer should be considered visible for input calculations.
- */
- virtual bool isVisibleForInput() const {
- // For compatibility reasons we let layers which can receive input
- // receive input before they have actually submitted a buffer. Because
- // of this we use canReceiveInput instead of isVisible to check the
- // policy-visibility, ignoring the buffer state. However for layers with
- // hasInputInfo()==false we can use the real visibility state.
- // We are just using these layers for occlusion detection in
- // InputDispatcher, and obviously if they aren't visible they can't occlude
- // anything.
- return hasInputInfo() ? canReceiveInput() : isVisible();
- }
-
- /*
* isProtected - true if the layer may contain protected contents in the
* GRALLOC_USAGE_PROTECTED sense.
*/
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 8c46515..26f8010 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3269,34 +3269,16 @@
if (!updateWindowInfo && mInputWindowCommands.empty()) {
return;
}
-
- std::unordered_set<Layer*> visibleLayers;
- mDrawingState.traverse([&visibleLayers](Layer* layer) {
- if (layer->isVisibleForInput()) {
- visibleLayers.insert(layer);
- }
- });
- bool visibleLayersChanged = false;
- if (visibleLayers != mVisibleLayers) {
- visibleLayersChanged = true;
- mVisibleLayers = std::move(visibleLayers);
- }
-
BackgroundExecutor::getInstance().sendCallbacks({[updateWindowInfo,
windowInfos = std::move(windowInfos),
displayInfos = std::move(displayInfos),
inputWindowCommands =
std::move(mInputWindowCommands),
- inputFlinger = mInputFlinger, this,
- visibleLayersChanged]() {
+ inputFlinger = mInputFlinger, this]() {
ATRACE_NAME("BackgroundExecutor::updateInputFlinger");
if (updateWindowInfo) {
- mWindowInfosListenerInvoker
- ->windowInfosChanged(std::move(windowInfos), std::move(displayInfos),
- /* shouldSync= */ inputWindowCommands.syncInputWindows,
- /* forceImmediateCall= */
- visibleLayersChanged ||
- !inputWindowCommands.focusRequests.empty());
+ mWindowInfosListenerInvoker->windowInfosChanged(windowInfos, displayInfos,
+ inputWindowCommands.syncInputWindows);
} else if (inputWindowCommands.syncInputWindows) {
// If the caller requested to sync input windows, but there are no
// changes to input windows, notify immediately.
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index d9add5c..62ee1b9 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -1454,11 +1454,6 @@
nsecs_t mAnimationTransactionTimeout = s2ns(5);
friend class SurfaceComposerAIDL;
-
- // Layers visible during the last commit. This set should only be used for testing set equality
- // and membership. The pointers should not be dereferenced as it's possible the set contains
- // pointers to freed layers.
- std::unordered_set<Layer*> mVisibleLayers;
};
class SurfaceComposerAIDL : public gui::BnSurfaceComposer {
diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.cpp b/services/surfaceflinger/WindowInfosListenerInvoker.cpp
index 023402f..30b9d8f 100644
--- a/services/surfaceflinger/WindowInfosListenerInvoker.cpp
+++ b/services/surfaceflinger/WindowInfosListenerInvoker.cpp
@@ -28,26 +28,19 @@
struct WindowInfosListenerInvoker::WindowInfosReportedListener
: gui::BnWindowInfosReportedListener {
- explicit WindowInfosReportedListener(WindowInfosListenerInvoker& invoker, size_t callbackCount,
- bool shouldSync)
- : mInvoker(invoker), mCallbacksPending(callbackCount), mShouldSync(shouldSync) {}
+ explicit WindowInfosReportedListener(WindowInfosListenerInvoker& invoker) : mInvoker(invoker) {}
binder::Status onWindowInfosReported() override {
- mCallbacksPending--;
- if (mCallbacksPending == 0) {
- mInvoker.windowInfosReported(mShouldSync);
- }
+ mInvoker.windowInfosReported();
return binder::Status::ok();
}
-private:
WindowInfosListenerInvoker& mInvoker;
- std::atomic<size_t> mCallbacksPending;
- bool mShouldSync;
};
WindowInfosListenerInvoker::WindowInfosListenerInvoker(SurfaceFlinger& flinger)
- : mFlinger(flinger) {}
+ : mFlinger(flinger),
+ mWindowInfosReportedListener(sp<WindowInfosReportedListener>::make(*this)) {}
void WindowInfosListenerInvoker::addWindowInfosListener(sp<IWindowInfosListener> listener) {
sp<IBinder> asBinder = IInterface::asBinder(listener);
@@ -71,76 +64,30 @@
mWindowInfosListeners.erase(who);
}
-void WindowInfosListenerInvoker::windowInfosChanged(std::vector<WindowInfo> windowInfos,
- std::vector<DisplayInfo> displayInfos,
- bool shouldSync, bool forceImmediateCall) {
- auto callListeners = [this, windowInfos = std::move(windowInfos),
- displayInfos = std::move(displayInfos)](bool shouldSync) mutable {
- ftl::SmallVector<const sp<IWindowInfosListener>, kStaticCapacity> windowInfosListeners;
- {
- std::scoped_lock lock(mListenersMutex);
- for (const auto& [_, listener] : mWindowInfosListeners) {
- windowInfosListeners.push_back(listener);
- }
- }
-
- auto reportedListener =
- sp<WindowInfosReportedListener>::make(*this, windowInfosListeners.size(),
- shouldSync);
-
- for (const auto& listener : windowInfosListeners) {
- auto status =
- listener->onWindowInfosChanged(windowInfos, displayInfos, reportedListener);
- if (!status.isOk()) {
- reportedListener->onWindowInfosReported();
- }
- }
- };
-
+void WindowInfosListenerInvoker::windowInfosChanged(const std::vector<WindowInfo>& windowInfos,
+ const std::vector<DisplayInfo>& displayInfos,
+ bool shouldSync) {
+ ftl::SmallVector<const sp<IWindowInfosListener>, kStaticCapacity> windowInfosListeners;
{
- std::scoped_lock lock(mMessagesMutex);
- // If there are unacked messages and this isn't a forced call, then return immediately.
- // If a forced window infos change doesn't happen first, the update will be sent after
- // the WindowInfosReportedListeners are called. If a forced window infos change happens or
- // if there are subsequent delayed messages before this update is sent, then this message
- // will be dropped and the listeners will only be called with the latest info. This is done
- // to reduce the amount of binder memory used.
- if (mActiveMessageCount > 0 && !forceImmediateCall) {
- mWindowInfosChangedDelayed = std::move(callListeners);
- mShouldSyncDelayed |= shouldSync;
- return;
+ std::scoped_lock lock(mListenersMutex);
+ for (const auto& [_, listener] : mWindowInfosListeners) {
+ windowInfosListeners.push_back(listener);
}
-
- mWindowInfosChangedDelayed = nullptr;
- shouldSync |= mShouldSyncDelayed;
- mShouldSyncDelayed = false;
- mActiveMessageCount++;
}
- callListeners(shouldSync);
+
+ mCallbacksPending = windowInfosListeners.size();
+
+ for (const auto& listener : windowInfosListeners) {
+ listener->onWindowInfosChanged(windowInfos, displayInfos,
+ shouldSync ? mWindowInfosReportedListener : nullptr);
+ }
}
-void WindowInfosListenerInvoker::windowInfosReported(bool shouldSync) {
- if (shouldSync) {
+void WindowInfosListenerInvoker::windowInfosReported() {
+ mCallbacksPending--;
+ if (mCallbacksPending == 0) {
mFlinger.windowInfosReported();
}
-
- std::function<void(bool)> callListeners;
- bool shouldSyncDelayed;
- {
- std::scoped_lock lock{mMessagesMutex};
- mActiveMessageCount--;
- if (!mWindowInfosChangedDelayed || mActiveMessageCount > 0) {
- return;
- }
-
- mActiveMessageCount++;
- callListeners = std::move(mWindowInfosChangedDelayed);
- mWindowInfosChangedDelayed = nullptr;
- shouldSyncDelayed = mShouldSyncDelayed;
- mShouldSyncDelayed = false;
- }
-
- callListeners(shouldSyncDelayed);
}
} // namespace android
diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.h b/services/surfaceflinger/WindowInfosListenerInvoker.h
index 701f11e..d8d8d0f 100644
--- a/services/surfaceflinger/WindowInfosListenerInvoker.h
+++ b/services/surfaceflinger/WindowInfosListenerInvoker.h
@@ -34,15 +34,15 @@
void addWindowInfosListener(sp<gui::IWindowInfosListener>);
void removeWindowInfosListener(const sp<gui::IWindowInfosListener>& windowInfosListener);
- void windowInfosChanged(std::vector<gui::WindowInfo>, std::vector<gui::DisplayInfo>,
- bool shouldSync, bool forceImmediateCall);
+ void windowInfosChanged(const std::vector<gui::WindowInfo>&,
+ const std::vector<gui::DisplayInfo>&, bool shouldSync);
protected:
void binderDied(const wp<IBinder>& who) override;
private:
struct WindowInfosReportedListener;
- void windowInfosReported(bool shouldSync);
+ void windowInfosReported();
SurfaceFlinger& mFlinger;
std::mutex mListenersMutex;
@@ -51,10 +51,8 @@
ftl::SmallMap<wp<IBinder>, const sp<gui::IWindowInfosListener>, kStaticCapacity>
mWindowInfosListeners GUARDED_BY(mListenersMutex);
- std::mutex mMessagesMutex;
- uint32_t mActiveMessageCount GUARDED_BY(mMessagesMutex) = 0;
- std::function<void(bool)> mWindowInfosChangedDelayed GUARDED_BY(mMessagesMutex);
- bool mShouldSyncDelayed;
+ sp<gui::IWindowInfosReportedListener> mWindowInfosReportedListener;
+ std::atomic<size_t> mCallbacksPending{0};
};
} // namespace android