Merge "Improve error logging for parsing failures" into nyc-dev

core/java/android/util/PathParser.java

@@ -31,12 +31,7 @@ public class PathParser {
             throw new IllegalArgumentException("Path string can not be null.");
         }
         Path path = new Path();
-        boolean hasValidPathData = nParseStringForPath(path.mNativePath, pathString,
-                pathString.length());
-        if (!hasValidPathData) {
-            throw new IllegalArgumentException("Path string: " + pathString +
-                    " does not contain valid path data");
-        }
+        nParseStringForPath(path.mNativePath, pathString, pathString.length());
         return path;
     }
 
@@ -104,7 +99,6 @@ public class PathParser {
             }
             super.finalize();
         }
-
     }
 
     /**
@@ -123,7 +117,7 @@ public class PathParser {
     }
 
     // Native functions are defined below.
-    private static native boolean nParseStringForPath(long pathPtr, String pathString,
+    private static native void nParseStringForPath(long pathPtr, String pathString,
             int stringLength);
     private static native void nCreatePathFromPathData(long outPathPtr, long pathData);
     private static native long nCreateEmptyPathData();

core/jni/android_graphics_drawable_VectorDrawable.cpp

@@ -176,6 +176,9 @@ static void setPathString(JNIEnv* env, jobject, jlong pathPtr, jstring inputStr,
     PathParser::ParseResult result;
     PathData data;
     PathParser::getPathDataFromString(&data, &result, pathString, stringLength);
+    if (result.failureOccurred) {
+        doThrowIAE(env, result.failureMessage.c_str());
+    }
     path->mutateStagingProperties()->setData(data);
     env->ReleaseStringUTFChars(inputStr, pathString);
 }

core/jni/android_util_PathParser.cpp

@@ -15,6 +15,7 @@
  */
 
 #include "jni.h"
+#include "GraphicsJNI.h"
 
 #include <PathParser.h>
 #include <SkPath.h>
@@ -27,7 +28,7 @@ namespace android {
 
 using namespace uirenderer;
 
-static bool parseStringForPath(JNIEnv* env, jobject, jlong skPathHandle, jstring inputPathStr,
+static void parseStringForPath(JNIEnv* env, jobject, jlong skPathHandle, jstring inputPathStr,
         jint strLength) {
     const char* pathString = env->GetStringUTFChars(inputPathStr, NULL);
     SkPath* skPath = reinterpret_cast<SkPath*>(skPathHandle);
@@ -36,9 +37,8 @@ static bool parseStringForPath(JNIEnv* env, jobject, jlong skPathHandle, jstring
     PathParser::parseStringForSkPath(skPath, &result, pathString, strLength);
     env->ReleaseStringUTFChars(inputPathStr, pathString);
     if (result.failureOccurred) {
-        ALOGE(result.failureMessage.c_str());
+        doThrowIAE(env, result.failureMessage.c_str());
     }
-    return !result.failureOccurred;
 }
 
 static long createEmptyPathData(JNIEnv*, jobject) {
@@ -62,7 +62,7 @@ static long createPathDataFromStringPath(JNIEnv* env, jobject, jstring inputStr,
         return reinterpret_cast<jlong>(pathData);
     } else {
         delete pathData;
-        ALOGE(result.failureMessage.c_str());
+        doThrowIAE(env, result.failureMessage.c_str());
         return NULL;
     }
 }
@@ -100,7 +100,7 @@ static void setSkPathFromPathData(JNIEnv*, jobject, jlong outPathPtr, jlong path
 }
 
 static const JNINativeMethod gMethods[] = {
-    {"nParseStringForPath", "(JLjava/lang/String;I)Z", (void*)parseStringForPath},
+    {"nParseStringForPath", "(JLjava/lang/String;I)V", (void*)parseStringForPath},
     {"nCreateEmptyPathData", "!()J", (void*)createEmptyPathData},
     {"nCreatePathData", "!(J)J", (void*)createPathData},
     {"nCreatePathDataFromString", "(Ljava/lang/String;I)J", (void*)createPathDataFromStringPath},

libs/hwui/PathParser.cpp

@@ -156,6 +156,12 @@ static void getFloats(std::vector<float>* outPoints, PathParser::ParseResult* re
     return;
 }
 
+bool PathParser::isVerbValid(char verb) {
+    verb = tolower(verb);
+    return verb == 'a' || verb == 'c' || verb == 'h' || verb == 'l' || verb == 'm' || verb == 'q'
+            || verb == 's' || verb == 't' || verb == 'v' || verb == 'z';
+}
+
 void PathParser::getPathDataFromString(PathData* data, ParseResult* result,
         const char* pathStr, size_t strLen) {
     if (pathStr == NULL) {
@@ -171,6 +177,12 @@ void PathParser::getPathDataFromString(PathData* data, ParseResult* result,
         end = nextStart(pathStr, strLen, end);
         std::vector<float> points;
         getFloats(&points, result, pathStr, start, end);
+        if (!isVerbValid(pathStr[start])) {
+            result->failureOccurred = true;
+            result->failureMessage = "Invalid pathData. Failure occurred at position "
+                    + std::to_string(start) + " of path: " + pathStr;
+        }
+        // If either verb or points is not valid, return immediately.
         if (result->failureOccurred) {
             return;
         }
@@ -182,10 +194,15 @@ void PathParser::getPathDataFromString(PathData* data, ParseResult* result,
     }
 
     if ((end - start) == 1 && start < strLen) {
+        if (!isVerbValid(pathStr[start])) {
+            result->failureOccurred = true;
+            result->failureMessage = "Invalid pathData. Failure occurred at position "
+                    + std::to_string(start) + " of path: " + pathStr;
+            return;
+        }
         data->verbs.push_back(pathStr[start]);
         data->verbSizes.push_back(0);
     }
-    return;
 }
 
 void PathParser::dump(const PathData& data) {
@@ -218,7 +235,8 @@ void PathParser::parseStringForSkPath(SkPath* skPath, ParseResult* result, const
     // Check if there is valid data coming out of parsing the string.
     if (pathData.verbs.size() == 0) {
         result->failureOccurred = true;
-        result->failureMessage = "No verbs found in the string for pathData";
+        result->failureMessage = "No verbs found in the string for pathData: ";
+        result->failureMessage += pathStr;
         return;
     }
     VectorDrawableUtils::verbsToPath(skPath, pathData);

libs/hwui/PathParser.h

@@ -44,6 +44,7 @@ public:
     ANDROID_API static void getPathDataFromString(PathData* outData, ParseResult* result,
             const char* pathStr, size_t strLength);
     static void dump(const PathData& data);
+    static bool isVerbValid(char verb);
 };
 
 }; // namespace uirenderer

libs/hwui/tests/unit/VectorDrawableTests.cpp

@@ -231,11 +231,12 @@ struct StringPath {
 };
 
 const StringPath sStringPaths[] = {
-    {"3e...3", false},
-    {"L.M.F.A.O", false},
-    {"m 1 1", true},
-    {"z", true},
-    {"1-2e34567", false}
+    {"3e...3", false}, // Not starting with a verb and ill-formatted float
+    {"L.M.F.A.O", false}, // No floats following verbs
+    {"m 1 1", true}, // Valid path data
+    {"z", true}, // Valid path data
+    {"1-2e34567", false}, // Not starting with a verb and ill-formatted float
+    {"f 4 5", false} // Invalid verb
 };