From 6165d7a2be441a2602c71ba8a776df4e1dd1b968 Mon Sep 17 00:00:00 2001 From: Andy Street Date: Mon, 7 Nov 2016 05:59:47 -0800 Subject: [PATCH] Make CSSNode#measure final, cache more correct methodid Summary: @public In the JNI portion of CSSLayout, there's a subtle bug where we were caching the jmethodid of the 'measure' of the first object that had measure called on it. However, if that class had overriden measure, then the jmethodid would be specific to that subclass's implementation and would not work for other classes. Conversely, if a regular CSSNode had been called first, then the super method would be called on the subclass instead of the proper overriden method. Since there's not really a reason to overriden measure anyway (since you can just provide a different measure function), it's safest to just mark it final and explicitly cache the appropriate methodid Reviewed By: emilsjolander Differential Revision: D4132428 fbshipit-source-id: 6fb51597d80f1c03681e6611153b52b73b392245 --- java/com/facebook/csslayout/CSSNode.java | 7 ++++++- java/jni/CSSJNI.cpp | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/java/com/facebook/csslayout/CSSNode.java b/java/com/facebook/csslayout/CSSNode.java index b928c212..6d5d61b3 100644 --- a/java/com/facebook/csslayout/CSSNode.java +++ b/java/com/facebook/csslayout/CSSNode.java @@ -494,8 +494,13 @@ public class CSSNode implements CSSNodeAPI { jni_CSSNodeSetHasMeasureFunc(mNativePointer, measureFunction != null); } + // Implementation Note: Why this method needs to stay final + // + // We cache the jmethodid for this method in CSSLayout code. This means that even if a subclass + // were to override measure, we'd still call this implementation from layout code since the + // overriding method will have a different jmethodid. This is final to prevent that mistake. @DoNotStrip - public long measure(float width, int widthMode, float height, int heightMode) { + public final long measure(float width, int widthMode, float height, int heightMode) { if (!isMeasureDefined()) { throw new RuntimeException("Measure function isn't defined!"); } diff --git a/java/jni/CSSJNI.cpp b/java/jni/CSSJNI.cpp index 3d9d488b..87c67cc8 100644 --- a/java/jni/CSSJNI.cpp +++ b/java/jni/CSSJNI.cpp @@ -51,8 +51,8 @@ static CSSSize _jniMeasureFunc(CSSNodeRef node, CSSMeasureMode heightMode) { auto obj = adopt_local(Environment::current()->NewLocalRef(reinterpret_cast(CSSNodeGetContext(node)))); - static auto measureFunc = - obj->getClass()->getMethod("measure"); + static auto measureFunc = findClassLocal("com/facebook/csslayout/CSSNode") + ->getMethod("measure"); _jniTransferLayoutDirection(node, obj); const auto measureResult = measureFunc(obj, width, widthMode, height, heightMode);