From 402eec8de75161bf200d21e11c15a33fa0e0be74 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 2 Aug 2021 20:46:24 -0700 Subject: [PATCH] Protect against crashes when over-releasing a TouchEvent Summary: It seems that, possibly due to optimizations and refactoring elsewhere in the event system, some TouchEvents are being over-disposed. This doesn't really pose a problem besides performance; and could even indicate that an Event was in a pool but never properly initialized. In these cases it seems perfectly reasonable to silently continue, and to log a soft exception. This WILL still crash in debug mode, so we can gather more information if we find a good repro; otherwise we will continue to get production data from this soft exception if it's an issue and we can investigate further. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D30061178 fbshipit-source-id: 05d1f60afc382ce0a202ac8f3de34770cf9a760d --- .../react/uimanager/events/TouchEvent.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/TouchEvent.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/TouchEvent.java index e0509633bb7..f6503965093 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/TouchEvent.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/TouchEvent.java @@ -131,9 +131,25 @@ public class TouchEvent extends Event { @Override public void onDispose() { - Assertions.assertNotNull(mMotionEvent).recycle(); + MotionEvent motionEvent = mMotionEvent; mMotionEvent = null; - EVENTS_POOL.release(this); + if (motionEvent != null) { + motionEvent.recycle(); + } + + // Either `this` is in the event pool, or motionEvent + // is null. It is in theory not possible for a TouchEvent to + // be in the EVENTS_POOL but for motionEvent to be null. However, + // out of an abundance of caution and to avoid memory leaks or + // other crashes at all costs, we attempt to release here and log + // a soft exception here if release throws an IllegalStateException + // due to `this` being over-released. This may indicate that there is + // a logic error in our events system or pooling mechanism. + try { + EVENTS_POOL.release(this); + } catch (IllegalStateException e) { + ReactSoftException.logSoftException(TAG, e); + } } @Override