From c1d9527406dbf70972cd49b02f02d254277a1e17 Mon Sep 17 00:00:00 2001 From: Paco Estevez Garcia Date: Wed, 1 Jul 2020 11:03:58 -0700 Subject: [PATCH] Fix potential nullable accesss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: While ramping up to Flipper I found this potential NPE caused by ordering issues. There are only 3 uses downstream that don't use a constant that's part of the default initializer. ``` λ festevezga-mbp xplat → buck build sonar/android/src/main/java/com/facebook/flipper/plugins/inspector/... 2>&1 | rg ' = descriptorForClass' | awk '{$1=$1};1' | sort | uniq -c 5 NodeDescriptor descriptor = descriptorForClass(Object.class); 3 NodeDescriptor descriptor = descriptorForClass(View.class); 1 NodeDescriptor descriptor = descriptorForClass(node.getClass()); 2 final NodeDescriptor descriptor = descriptorForClass(Dialog.class); 12 final NodeDescriptor descriptor = descriptorForClass(Fragment.class); 5 final NodeDescriptor descriptor = descriptorForClass(Object.class); 26 final NodeDescriptor descriptor = descriptorForClass(View.class); 2 final NodeDescriptor descriptor = descriptorForClass(Window.class); 1 final NodeDescriptor descriptor = descriptorForClass(node.getClass()); 1 final NodeDescriptor descriptor = descriptorForClass(topChild.getClass()); 2 final NodeDescriptor objectDescriptor = descriptorForClass(Object.class); ``` Reviewed By: muraziz Differential Revision: D22284669 fbshipit-source-id: b41c8d78c70c5b5acdc08aa6f0e7afa681f4242d --- .../plugins/inspector/DescriptorMapping.java | 3 ++- .../flipper/plugins/inspector/NodeDescriptor.java | 13 +++++++++---- .../descriptors/ApplicationDescriptor.java | 4 +++- .../inspector/descriptors/ObjectDescriptor.java | 5 +++-- .../inspector/descriptors/ViewGroupDescriptor.java | 4 +++- 5 files changed, 20 insertions(+), 9 deletions(-) diff --git a/android/src/main/java/com/facebook/flipper/plugins/inspector/DescriptorMapping.java b/android/src/main/java/com/facebook/flipper/plugins/inspector/DescriptorMapping.java index b41299bc7..99144ad45 100644 --- a/android/src/main/java/com/facebook/flipper/plugins/inspector/DescriptorMapping.java +++ b/android/src/main/java/com/facebook/flipper/plugins/inspector/DescriptorMapping.java @@ -14,6 +14,7 @@ import android.view.View; import android.view.ViewGroup; import android.view.Window; import android.widget.TextView; +import androidx.annotation.Nullable; import com.facebook.flipper.core.FlipperConnection; import com.facebook.flipper.plugins.inspector.descriptors.ActivityDescriptor; import com.facebook.flipper.plugins.inspector.descriptors.ApplicationDescriptor; @@ -66,7 +67,7 @@ public class DescriptorMapping { mMapping.put(clazz, descriptor); } - public NodeDescriptor descriptorForClass(Class clazz) { + public @Nullable NodeDescriptor descriptorForClass(Class clazz) { while (!mMapping.containsKey(clazz)) { clazz = clazz.getSuperclass(); } diff --git a/android/src/main/java/com/facebook/flipper/plugins/inspector/NodeDescriptor.java b/android/src/main/java/com/facebook/flipper/plugins/inspector/NodeDescriptor.java index 9fca55112..c1bb34bc0 100644 --- a/android/src/main/java/com/facebook/flipper/plugins/inspector/NodeDescriptor.java +++ b/android/src/main/java/com/facebook/flipper/plugins/inspector/NodeDescriptor.java @@ -17,13 +17,15 @@ import java.util.Collections; import java.util.List; /** - * A NodeDescriptor is an object which known how to expose an Object of type T to the ew Inspector. + * A NodeDescriptor is an object which known how to expose an Object of type T to the new Inspector. * This class is the extension point for the Flipper inspector plugin and is how custom classes and * data can be exposed to the inspector. */ public abstract class NodeDescriptor { @Nullable protected FlipperConnection mConnection; - private DescriptorMapping mDescriptorMapping; + + // This field is not initialized until setDescriptorMapping is called + @Nullable private DescriptorMapping mDescriptorMapping; void setConnection(FlipperConnection connection) { mConnection = connection; @@ -39,8 +41,11 @@ public abstract class NodeDescriptor { * object it describes. This is highly encouraged instead of subclassing another descriptor * class. */ - protected final NodeDescriptor descriptorForClass(Class clazz) { - return mDescriptorMapping.descriptorForClass(clazz); + protected final @Nullable NodeDescriptor descriptorForClass(Class clazz) { + if (mDescriptorMapping != null) { + return mDescriptorMapping.descriptorForClass(clazz); + } + return null; } /** diff --git a/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ApplicationDescriptor.java b/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ApplicationDescriptor.java index 7f4c38066..a3457fd1b 100644 --- a/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ApplicationDescriptor.java +++ b/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ApplicationDescriptor.java @@ -230,7 +230,9 @@ public class ApplicationDescriptor extends NodeDescriptor { if (childCount > 0) { final Object topChild = getChildAt(node, childCount - 1); final NodeDescriptor descriptor = descriptorForClass(topChild.getClass()); - descriptor.setHighlighted(topChild, selected, isAlignmentMode); + if (descriptor != null) { + descriptor.setHighlighted(topChild, selected, isAlignmentMode); + } } } diff --git a/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ObjectDescriptor.java b/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ObjectDescriptor.java index f3150075c..ac14cb39e 100644 --- a/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ObjectDescriptor.java +++ b/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ObjectDescriptor.java @@ -70,7 +70,8 @@ public class ObjectDescriptor extends NodeDescriptor { @Override public boolean matches(String query, Object node) throws Exception { final NodeDescriptor descriptor = descriptorForClass(node.getClass()); - final List> attributes = descriptor.getAttributes(node); + final List> attributes = + descriptor == null ? Collections.emptyList() : descriptor.getAttributes(node); for (Named namedString : attributes) { if (namedString.getName().equals("id")) { if (namedString.getValue().toLowerCase().contains(query)) { @@ -79,6 +80,6 @@ public class ObjectDescriptor extends NodeDescriptor { } } - return descriptor.getName(node).toLowerCase().contains(query); + return descriptor == null ? false : descriptor.getName(node).toLowerCase().contains(query); } } diff --git a/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ViewGroupDescriptor.java b/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ViewGroupDescriptor.java index bddf55e04..89fc27bb4 100644 --- a/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ViewGroupDescriptor.java +++ b/android/src/main/java/com/facebook/flipper/plugins/inspector/descriptors/ViewGroupDescriptor.java @@ -74,7 +74,9 @@ public class ViewGroupDescriptor extends NodeDescriptor { if (connected()) { if (key.set(node)) { NodeDescriptor descriptor = descriptorForClass(node.getClass()); - descriptor.invalidate(node); + if (descriptor != null) { + descriptor.invalidate(node); + } invalidateAX(node); }