Attempt to fix [2152536] ANR in browser
The ANR is caused by SurfaceFlinger waiting for buffers of a removed surface to become availlable.
When it is removed from the current list, a Surface is marked as NO_INIT, which causes SF to return
immediately in the above case. For some reason, the surface here wasn't marked as NO_INIT.
This change makes the code more robust by always (irregadless or errors) setting the NO_INIT status
in all code paths where a surface is removed from the list.
Additionaly added more information in the logs, should this happen again.
diff --git a/include/private/ui/SharedBufferStack.h b/include/private/ui/SharedBufferStack.h
index c02b2e7..59cf31c 100644
--- a/include/private/ui/SharedBufferStack.h
+++ b/include/private/ui/SharedBufferStack.h
@@ -142,6 +142,7 @@
SharedBufferBase(SharedClient* sharedClient, int surface, int num);
~SharedBufferBase();
uint32_t getIdentity();
+ status_t getStatus() const;
size_t getFrontBuffer() const;
String8 dump(char const* prefix) const;
@@ -177,7 +178,7 @@
{
const SharedBufferStack& stack( *mSharedStack );
SharedClient& client( *mSharedClient );
- const nsecs_t TIMEOUT = s2ns(1);
+ const nsecs_t TIMEOUT = s2ns(1);
Mutex::Autolock _l(client.lock);
while ((condition()==false) && (stack.status == NO_ERROR)) {
status_t err = client.cv.waitRelative(client.lock, TIMEOUT);
@@ -187,14 +188,15 @@
if (err == TIMED_OUT) {
if (condition()) {
LOGE("waitForCondition(%s) timed out (identity=%d), "
- "but condition is true! We recovered but it "
- "shouldn't happen." ,
- T::name(), mSharedStack->identity);
+ "but condition is true! We recovered but it "
+ "shouldn't happen." , T::name(),
+ mSharedStack->identity);
break;
} else {
- LOGW("waitForCondition(%s) timed out (identity=%d). "
- "CPU may be pegged. trying again.",
- T::name(), mSharedStack->identity);
+ LOGW("waitForCondition(%s) timed out "
+ "(identity=%d, status=%d). "
+ "CPU may be pegged. trying again.", T::name(),
+ mSharedStack->identity, mSharedStack->status);
}
} else {
LOGE("waitForCondition(%s) error (%s) ",
diff --git a/libs/surfaceflinger/Buffer.cpp b/libs/surfaceflinger/Buffer.cpp
index 65650aa..6190cd8 100644
--- a/libs/surfaceflinger/Buffer.cpp
+++ b/libs/surfaceflinger/Buffer.cpp
@@ -20,17 +20,12 @@
#include <utils/Errors.h>
#include <utils/Log.h>
-#include <binder/MemoryBase.h>
-#include <binder/IMemory.h>
#include <ui/PixelFormat.h>
-#include <ui/Surface.h>
#include <pixelflinger/pixelflinger.h>
#include "Buffer.h"
#include "BufferAllocator.h"
-#include "SurfaceFlinger.h"
-
namespace android {
diff --git a/libs/surfaceflinger/Buffer.h b/libs/surfaceflinger/Buffer.h
index 79f4eeb..203da3b 100644
--- a/libs/surfaceflinger/Buffer.h
+++ b/libs/surfaceflinger/Buffer.h
@@ -20,20 +20,11 @@
#include <stdint.h>
#include <sys/types.h>
-#include <hardware/gralloc.h>
-
-#include <utils/Atomic.h>
-
#include <ui/PixelFormat.h>
#include <ui/Rect.h>
-#include <ui/Surface.h>
-
#include <pixelflinger/pixelflinger.h>
-
-#include <private/ui/SharedBufferStack.h>
#include <private/ui/SurfaceBuffer.h>
-class copybit_image_t;
struct android_native_buffer_t;
namespace android {
@@ -42,8 +33,6 @@
// Buffer
// ===========================================================================
-class NativeBuffer;
-
class Buffer : public SurfaceBuffer
{
public:
diff --git a/libs/surfaceflinger/BufferAllocator.cpp b/libs/surfaceflinger/BufferAllocator.cpp
index caf9bec..3e37bc3 100644
--- a/libs/surfaceflinger/BufferAllocator.cpp
+++ b/libs/surfaceflinger/BufferAllocator.cpp
@@ -15,8 +15,6 @@
** limitations under the License.
*/
-#include <sys/mman.h>
-#include <cutils/ashmem.h>
#include <cutils/log.h>
#include <utils/Singleton.h>
diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp
index 07222ec..13201db 100644
--- a/libs/surfaceflinger/Layer.cpp
+++ b/libs/surfaceflinger/Layer.cpp
@@ -65,14 +65,6 @@
// the actual buffers will be destroyed here
}
-// called with SurfaceFlinger::mStateLock as soon as the layer is entered
-// in the purgatory list
-void Layer::onRemoved()
-{
- // wake up the condition
- lcblk->setStatus(NO_INIT);
-}
-
void Layer::destroy()
{
for (size_t i=0 ; i<NUM_BUFFERS ; i++) {
diff --git a/libs/surfaceflinger/Layer.h b/libs/surfaceflinger/Layer.h
index 6c7b27b..f111840 100644
--- a/libs/surfaceflinger/Layer.h
+++ b/libs/surfaceflinger/Layer.h
@@ -85,8 +85,6 @@
return mBuffers[mFrontBufferIndex];
}
- virtual void onRemoved();
-
void reloadTexture(const Region& dirty);
sp<SurfaceBuffer> requestBuffer(int index, int usage);
diff --git a/libs/surfaceflinger/LayerBase.cpp b/libs/surfaceflinger/LayerBase.cpp
index d83c8429..83814cc 100644
--- a/libs/surfaceflinger/LayerBase.cpp
+++ b/libs/surfaceflinger/LayerBase.cpp
@@ -690,6 +690,14 @@
const_cast<LayerBaseClient *>(this));
}
+// called with SurfaceFlinger::mStateLock as soon as the layer is entered
+// in the purgatory list
+void LayerBaseClient::onRemoved()
+{
+ // wake up the condition
+ lcblk->setStatus(NO_INIT);
+}
+
// ---------------------------------------------------------------------------
LayerBaseClient::Surface::Surface(
@@ -700,7 +708,6 @@
{
}
-
LayerBaseClient::Surface::~Surface()
{
/*
diff --git a/libs/surfaceflinger/LayerBase.h b/libs/surfaceflinger/LayerBase.h
index 16ee542..0dfa4fe 100644
--- a/libs/surfaceflinger/LayerBase.h
+++ b/libs/surfaceflinger/LayerBase.h
@@ -205,10 +205,13 @@
*/
virtual bool isSecure() const { return false; }
- /** signal this layer that it's not needed any longer. called from the
- * main thread */
+ /** Called from the main thread, when the surface is removed from the
+ * draw list */
virtual status_t ditch() { return NO_ERROR; }
+ /** called with the state lock when the surface is removed from the
+ * current list */
+ virtual void onRemoved() { };
enum { // flags for doTransaction()
@@ -318,7 +321,7 @@
sp<Surface> getSurface();
virtual sp<Surface> createSurface() const;
- virtual void onRemoved() { }
+ virtual void onRemoved();
class Surface : public BnSurface
{
diff --git a/libs/surfaceflinger/SurfaceFlinger.cpp b/libs/surfaceflinger/SurfaceFlinger.cpp
index eb0983a..f2b918f 100644
--- a/libs/surfaceflinger/SurfaceFlinger.cpp
+++ b/libs/surfaceflinger/SurfaceFlinger.cpp
@@ -1073,6 +1073,8 @@
// remove the layer from the main list (through a transaction).
ssize_t err = removeLayer_l(layerBase);
+ layerBase->onRemoved();
+
// it's possible that we don't find a layer, because it might
// have been destroyed already -- this is not technically an error
// from the user because there is a race between BClient::destroySurface(),
@@ -1321,7 +1323,6 @@
if (layer != 0) {
err = purgatorizeLayer_l(layer);
if (err == NO_ERROR) {
- layer->onRemoved();
setTransactionFlags(eTransactionNeeded);
}
}
diff --git a/libs/ui/SharedBufferStack.cpp b/libs/ui/SharedBufferStack.cpp
index 9ad4349..47c596c 100644
--- a/libs/ui/SharedBufferStack.cpp
+++ b/libs/ui/SharedBufferStack.cpp
@@ -114,6 +114,12 @@
return stack.identity;
}
+status_t SharedBufferBase::getStatus() const
+{
+ SharedBufferStack& stack( *mSharedStack );
+ return stack.status;
+}
+
size_t SharedBufferBase::getFrontBuffer() const
{
SharedBufferStack& stack( *mSharedStack );
@@ -135,7 +141,6 @@
return result;
}
-
// ============================================================================
// conditions and updates
// ============================================================================
@@ -375,8 +380,10 @@
void SharedBufferServer::setStatus(status_t status)
{
- StatusUpdate update(this, status);
- updateCondition( update );
+ if (status < NO_ERROR) {
+ StatusUpdate update(this, status);
+ updateCondition( update );
+ }
}
status_t SharedBufferServer::reallocate()
diff --git a/libs/ui/Surface.cpp b/libs/ui/Surface.cpp
index 64522fb..285edb4 100644
--- a/libs/ui/Surface.cpp
+++ b/libs/ui/Surface.cpp
@@ -733,9 +733,11 @@
index, usage);
if (buffer != 0) { // this should never happen by construction
LOGE_IF(buffer->handle == NULL,
- "requestBuffer(%d, %08x) returned a buffer with a null handle",
- index, usage);
- if (buffer->handle != NULL) {
+ "Surface (identity=%d) requestBuffer(%d, %08x) returned"
+ "a buffer with a null handle", mIdentity, index, usage);
+ err = mSharedBufferClient->getStatus();
+ LOGE_IF(err, "Surface (identity=%d) state = %d", mIdentity, err);
+ if (!err && buffer->handle != NULL) {
err = getBufferMapper().registerBuffer(buffer->handle);
LOGW_IF(err, "registerBuffer(...) failed %d (%s)",
err, strerror(-err));