Fixed all -Xcheck:jni warnings #70

CHANGELOG.md

@@ -1,3 +1,9 @@
+# 0.91.2 - Apr 29, 2021
+
+Fixed:
+
+- `-Xcheck:jni` warnings #70
+
 # 0.91.1 - Apr 28, 2021
 
 Fixed:

examples/kwinit/script/run.py

@@ -50,7 +50,7 @@ def main():
     + ['-Djava.awt.headless=true',
     '-enableassertions',
     '-enablesystemassertions',
-    # '-Xcheck:jni',
+    '-Xcheck:jni',
     '-Dskija.logLevel=DEBUG',
     'noria.kwinit.impl.Main'] + (["--verbose"] if args.verbose else []),
     env=env)

examples/lwjgl/script/run.py

@@ -45,6 +45,7 @@ def main():
     + ['-Djava.awt.headless=true',
     '-enableassertions',
     '-enablesystemassertions',
+    '-Xcheck:jni',
     '-Dskija.logLevel=DEBUG',
     'org.jetbrains.skija.examples.lwjgl.Main'])
 

examples/scenes/src/Scenes.java

@@ -6,7 +6,7 @@
 
 public class Scenes {
     public static TreeMap<String, Scene> scenes;
-    public static String currentScene = "Matrix";
+    public static String currentScene = "Drawable";
     public static HUD hud = new HUD();
     public static boolean vsync = true;
     public static boolean stats = true;

native/src/Codec.cc

@@ -75,10 +75,11 @@ extern "C" JNIEXPORT jobject JNICALL Java_org_jetbrains_skija_Codec__1nGetFrames
     SkCodec::FrameInfo info;
     std::vector<SkCodec::FrameInfo> frames = instance->getFrameInfo();
     jobjectArray res = env->NewObjectArray(frames.size(), skija::AnimationFrameInfo::cls, nullptr);
+    if (java::lang::Throwable::exceptionThrown(env))
+        return nullptr;
     for (int i = 0; i < frames.size(); ++i) {
-        jobject infoObj = skija::AnimationFrameInfo::toJava(env, frames[i]);
-        env->SetObjectArrayElement(res, i, infoObj);
-        env->DeleteLocalRef(infoObj);
+        skija::AutoLocal<jobject> infoObj(env, skija::AnimationFrameInfo::toJava(env, frames[i]));
+        env->SetObjectArrayElement(res, i, infoObj.get());
     }
     return res;
 }

native/src/Drawable.cc

@@ -9,26 +9,32 @@ class SkijaDrawableImpl: public SkDrawable {
     }
 
     ~SkijaDrawableImpl() {
-        fEnv->DeleteWeakGlobalRef(fObject);
+        JNIEnv* env;
+        if (fJavaVM->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_8) == JNI_OK)
+          env->DeleteWeakGlobalRef(fObject);
     }
 
     void init(JNIEnv* e, jobject o) {
         fEnv = e;
+        fEnv->GetJavaVM(&fJavaVM);
         fObject = fEnv->NewWeakGlobalRef(o);
     }
 
 protected:
     void onDraw(SkCanvas* canvas) override {
         fEnv->CallVoidMethod(fObject, skija::Drawable::onDraw, reinterpret_cast<jlong>(canvas));
+        java::lang::Throwable::exceptionThrown(fEnv);
     }
 
     SkRect onGetBounds() override {
-        jobject rect = fEnv->CallObjectMethod(fObject, skija::Drawable::onGetBounds);
-        return *(skija::Rect::toSkRect(fEnv, rect));
+        skija::AutoLocal<jobject> rect(fEnv, fEnv->CallObjectMethod(fObject, skija::Drawable::onGetBounds));
+        java::lang::Throwable::exceptionThrown(fEnv);
+        return *(skija::Rect::toSkRect(fEnv, rect.get()));
     }
 
 private:
     JNIEnv* fEnv;
+    JavaVM* fJavaVM;
     jobject fObject;
 };
 

native/src/Font.cc

@@ -294,7 +294,8 @@ extern "C" JNIEXPORT jobjectArray JNICALL Java_org_jetbrains_skija_Font__1nGetBo
 
     jobjectArray res = env->NewObjectArray(count, skija::Rect::cls, nullptr);
     for (int i = 0; i < count; ++i) {
-        env->SetObjectArrayElement(res, i, skija::Rect::fromSkRect(env, bounds[i]));
+        skija::AutoLocal<jobject> boundsObj(env, skija::Rect::fromSkRect(env, bounds[i]));
+        env->SetObjectArrayElement(res, i, boundsObj.get());
     }
 
     return res;

native/src/Picture.cc

@@ -19,7 +19,10 @@ class BooleanSupplierAbort: public SkPicture::AbortCallback {
         this->supplier = supplier;
     }
     bool abort() override {
-        return env->CallBooleanMethod(supplier, java::util::function::BooleanSupplier::apply);
+        bool res = env->CallBooleanMethod(supplier, java::util::function::BooleanSupplier::apply);
+        if (java::lang::Throwable::exceptionThrown(env))
+          return false;
+        return res;
     }
 private:
     JNIEnv* env;

native/src/interop.cc

@@ -75,8 +75,8 @@ namespace java {
 
             bool exceptionThrown(JNIEnv* env) {
                 if (env->ExceptionCheck()) {
-                    jthrowable th = env->ExceptionOccurred();
-                    env->CallVoidMethod(th, printStackTrace);
+                    auto th = skija::AutoLocal<jthrowable>(env, env->ExceptionOccurred());
+                    env->CallVoidMethod(th.get(), printStackTrace);
                     env->ExceptionCheck(); // ignore
                     return true;
                 } else
@@ -165,15 +165,16 @@ namespace skija {
                     blend = SkBlendMode::kSrc;
                     break;
             }
-            return env->NewObject(cls, ctor,
-                                  i.fRequiredFrame,
-                                  i.fDuration,
-                                  i.fFullyReceived,
-                                  static_cast<jint>(i.fAlphaType),
-                                  i.fHasAlphaWithinBounds,
-                                  static_cast<jint>(i.fDisposalMethod),
-                                  static_cast<jint>(blend),
-                                  IRect::fromSkIRect(env, i.fFrameRect));
+            jobject res = env->NewObject(cls, ctor,
+                                         i.fRequiredFrame,
+                                         i.fDuration,
+                                         i.fFullyReceived,
+                                         static_cast<jint>(i.fAlphaType),
+                                         i.fHasAlphaWithinBounds,
+                                         static_cast<jint>(i.fDisposalMethod),
+                                         static_cast<jint>(blend),
+                                         IRect::fromSkIRect(env, i.fFrameRect));
+            return java::lang::Throwable::exceptionThrown(env) ? nullptr : res;
         }
     }
 
@@ -250,12 +251,11 @@ namespace skija {
             jsize featuresLen = featuresArr == nullptr ? 0 : env->GetArrayLength(featuresArr);
             std::vector<SkShaper::Feature> features(featuresLen);
             for (int i = 0; i < featuresLen; ++i) {
-                jobject featureObj = env->GetObjectArrayElement(featuresArr, i);
-                features[i] = {static_cast<SkFourByteTag>(env->GetIntField(featureObj, skija::FontFeature::tag)),
-                               static_cast<uint32_t>(env->GetIntField(featureObj, skija::FontFeature::value)),
-                               static_cast<size_t>(env->GetLongField(featureObj, skija::FontFeature::start)),
-                               static_cast<size_t>(env->GetLongField(featureObj, skija::FontFeature::end))};
-                env->DeleteLocalRef(featureObj);
+                skija::AutoLocal<jobject> featureObj(env, env->GetObjectArrayElement(featuresArr, i));
+                features[i] = {static_cast<SkFourByteTag>(env->GetIntField(featureObj.get(), skija::FontFeature::tag)),
+                               static_cast<uint32_t>(env->GetIntField(featureObj.get(), skija::FontFeature::value)),
+                               static_cast<size_t>(env->GetLongField(featureObj.get(), skija::FontFeature::start)),
+                               static_cast<size_t>(env->GetLongField(featureObj.get(), skija::FontFeature::end))};
             }
             return features;
         }
@@ -410,7 +410,8 @@ namespace skija {
         }
 
         jobject fromSkIRect(JNIEnv* env, const SkIRect& rect) {
-            return env->CallStaticObjectMethod(cls, makeLTRB, rect.fLeft, rect.fTop, rect.fRight, rect.fBottom);
+            jobject res = env->CallStaticObjectMethod(cls, makeLTRB, rect.fLeft, rect.fTop, rect.fRight, rect.fBottom);
+            return java::lang::Throwable::exceptionThrown(env) ? nullptr : res;
         }
 
         std::unique_ptr<SkIRect> toSkIRect(JNIEnv* env, jobject obj) {
@@ -496,7 +497,8 @@ namespace skija {
         jobjectArray fromSkPoints(JNIEnv* env, const std::vector<SkPoint>& ps) {
             jobjectArray res = env->NewObjectArray((jsize) ps.size(), cls, nullptr);
             for (int i = 0; i < ps.size(); ++i) {
-                env->SetObjectArrayElement(res, i, fromSkPoint(env, ps[i]));
+                skija::AutoLocal<jobject> pointObj(env, fromSkPoint(env, ps[i]));
+                env->SetObjectArrayElement(res, i, pointObj.get());
             }
             return res;
         }
@@ -566,12 +568,15 @@ namespace skija {
                               env->GetFloatField(rectObj, top), 
                               env->GetFloatField(rectObj, right), 
                               env->GetFloatField(rectObj, bottom));
+                if (java::lang::Throwable::exceptionThrown(env))
+                    return std::unique_ptr<SkRect>(nullptr);
                 return std::unique_ptr<SkRect>(rect);
             }
         }
 
         jobject fromLTRB(JNIEnv* env, float left, float top, float right, float bottom) {
-            return env->CallStaticObjectMethod(cls, makeLTRB, left, top, right, bottom);
+            jobject res = env->CallStaticObjectMethod(cls, makeLTRB, left, top, right, bottom);
+            return java::lang::Throwable::exceptionThrown(env) ? nullptr : res;
         }
 
         jobject fromSkRect(JNIEnv* env, const SkRect& rect) {
@@ -724,7 +729,11 @@ namespace skija {
             if (surfacePropsObj == nullptr)
                 return std::unique_ptr<SkSurfaceProps>(nullptr);
             uint32_t flags = static_cast<uint32_t>(env->CallIntMethod(surfacePropsObj, _getFlags));
+            if (java::lang::Throwable::exceptionThrown(env))
+                std::unique_ptr<SkSurfaceProps>(nullptr);
             SkPixelGeometry geom = static_cast<SkPixelGeometry>(env->CallIntMethod(surfacePropsObj, _getPixelGeometryOrdinal));
+            if (java::lang::Throwable::exceptionThrown(env))
+                std::unique_ptr<SkSurfaceProps>(nullptr);
             return std::make_unique<SkSurfaceProps>(flags, geom);
         }
     }
@@ -1009,8 +1018,10 @@ std::vector<SkString> skStringVector(JNIEnv* env, jobjectArray arr) {
 
 jobjectArray javaStringArray(JNIEnv* env, const std::vector<SkString>& strings) {
     jobjectArray res = env->NewObjectArray((jsize) strings.size(), java::lang::String::cls, nullptr);
-    for (jint i = 0; i < (jsize) strings.size(); ++i)
-        env->SetObjectArrayElement(res, i, javaString(env, strings[i]));
+    for (jint i = 0; i < (jsize) strings.size(); ++i) {
+        skija::AutoLocal<jstring> str(env, javaString(env, strings[i]));
+        env->SetObjectArrayElement(res, i, str.get());
+    }
     return res;
 }
 

native/src/interop.hh

@@ -1,4 +1,5 @@
 #pragma once
+#include <iostream>
 #include <jni.h>
 #include <memory>
 #include <vector>
@@ -85,6 +86,29 @@ namespace skija {
         jobject toJava(JNIEnv* env, const SkCodec::FrameInfo& i);
     }
 
+    template <typename T>
+    class AutoLocal {
+    public:
+        AutoLocal(JNIEnv* env, T ref): fEnv(env), fRef(ref) {
+        }
+
+        AutoLocal(const AutoLocal&) = delete;
+        AutoLocal(AutoLocal&&) = default;
+        AutoLocal& operator=(AutoLocal const&) = delete;
+
+        ~AutoLocal() {
+            if (fRef)
+                fEnv->DeleteLocalRef(fRef);
+        }
+
+        T get() {
+            return fRef;
+        }
+    private:
+        JNIEnv* fEnv;
+        T fRef;
+    };
+
     namespace Color4f {
         extern jclass cls;
         extern jmethodID ctor;

native/src/shaper/Shaper.cc

@@ -129,14 +129,17 @@ class SkijaRunIterator: public RunIteratorSubclass {
       fEndOfRun(0)
     {
         fHasNext = fEnv->CallBooleanMethod(fIteratorObj, java::util::Iterator::hasNext);
+        java::lang::Throwable::exceptionThrown(fEnv);
     }
 
     void consume() override {
         SkASSERT(fHasNext);
-        jobject runObj = fEnv->CallObjectMethod(fIteratorObj, java::util::Iterator::next);
-        jint endOfRun16 = onConsume(runObj);
+        skija::AutoLocal<jobject> runObj(fEnv, fEnv->CallObjectMethod(fIteratorObj, java::util::Iterator::next));
+        java::lang::Throwable::exceptionThrown(fEnv);
+        jint endOfRun16 = onConsume(runObj.get());
         fEndOfRun = fIndicesConverter.from16To8(endOfRun16);
         fHasNext = fEnv->CallBooleanMethod(fIteratorObj, java::util::Iterator::hasNext);
+        java::lang::Throwable::exceptionThrown(fEnv);
     }
 
     size_t endOfCurrentRun() const override {
@@ -169,6 +172,7 @@ class SkijaFontRunIterator: public SkijaRunIterator<SkShaper::FontRunIterator> {
 
     jint onConsume(jobject runObj) override {
         jlong fontPtr = fEnv->CallLongMethod(runObj, skija::shaper::FontRun::_getFontPtr);
+        java::lang::Throwable::exceptionThrown(fEnv);
         fFont = reinterpret_cast<SkFont*>(static_cast<uintptr_t>(fontPtr));
         return fEnv->GetIntField(runObj, skija::shaper::FontRun::_end);
     }
@@ -245,29 +249,33 @@ class SkijaRunHandler: public SkShaper::RunHandler {
 
     void beginLine() {
         fEnv->CallVoidMethod(fRunHandler, skija::shaper::RunHandler::beginLine);
+        java::lang::Throwable::exceptionThrown(fEnv);
     }
 
     void runInfo(const SkShaper::RunHandler::RunInfo& info) {
-        jobject runInfoObj = skija::shaper::RunInfo::toJava(fEnv, info, fIndicesConverter);
-        fEnv->CallVoidMethod(fRunHandler, skija::shaper::RunHandler::runInfo, runInfoObj);
-        fEnv->SetLongField(runInfoObj, skija::shaper::RunInfo::_fontPtr, 0);
+        skija::AutoLocal<jobject> runInfoObj(fEnv, skija::shaper::RunInfo::toJava(fEnv, info, fIndicesConverter));
+        fEnv->CallVoidMethod(fRunHandler, skija::shaper::RunHandler::runInfo, runInfoObj.get());
+        java::lang::Throwable::exceptionThrown(fEnv);
+        fEnv->SetLongField(runInfoObj.get(), skija::shaper::RunInfo::_fontPtr, 0);
     }
 
     void commitRunInfo() {
         fEnv->CallVoidMethod(fRunHandler, skija::shaper::RunHandler::commitRunInfo);
+        java::lang::Throwable::exceptionThrown(fEnv);
     }
 
     SkShaper::RunHandler::Buffer runBuffer(const SkShaper::RunHandler::RunInfo& info) {
         fGlyphs    = std::vector<jshort>(info.glyphCount);
         fPositions = std::vector<SkPoint>(info.glyphCount);
         fClusters  = std::vector<jint>(info.glyphCount);
 
-        jobject runInfoObj = skija::shaper::RunInfo::toJava(fEnv, info, fIndicesConverter);
-        jobject point = fEnv->CallObjectMethod(fRunHandler, skija::shaper::RunHandler::runOffset, runInfoObj);
-        fEnv->SetLongField(runInfoObj, skija::shaper::RunInfo::_fontPtr, 0);
+        skija::AutoLocal<jobject> runInfoObj(fEnv, skija::shaper::RunInfo::toJava(fEnv, info, fIndicesConverter));
+        skija::AutoLocal<jobject> point(fEnv, fEnv->CallObjectMethod(fRunHandler, skija::shaper::RunHandler::runOffset, runInfoObj.get()));
+        java::lang::Throwable::exceptionThrown(fEnv);
+        fEnv->SetLongField(runInfoObj.get(), skija::shaper::RunInfo::_fontPtr, 0);
 
-        jfloat x = fEnv->GetFloatField(point, skija::Point::x);
-        jfloat y = fEnv->GetFloatField(point, skija::Point::y);
+        jfloat x = fEnv->GetFloatField(point.get(), skija::Point::x);
+        jfloat y = fEnv->GetFloatField(point.get(), skija::Point::y);
 
         return SkShaper::RunHandler::Buffer{
             reinterpret_cast<SkGlyphID*>(fGlyphs.data()),
@@ -281,17 +289,19 @@ class SkijaRunHandler: public SkShaper::RunHandler {
         size_t begin = fIndicesConverter.from8To16(info.utf8Range.fBegin);
         for (int i = 0; i < fClusters.size(); ++i)
             fClusters[i] = fIndicesConverter.from8To16(fClusters[i]);
-        jintArray clusters = javaIntArray(fEnv, fClusters);
+        skija::AutoLocal<jintArray> clusters(fEnv, javaIntArray(fEnv, fClusters));
         size_t end = fIndicesConverter.from8To16(info.utf8Range.fBegin + info.utf8Range.fSize);
-        jobject runInfoObj = skija::shaper::RunInfo::toJava(fEnv, info, begin, end);
-        jshortArray glyphs = javaShortArray(fEnv, fGlyphs);
-        jobjectArray positions = skija::Point::fromSkPoints(fEnv, fPositions);
-        fEnv->CallObjectMethod(fRunHandler, skija::shaper::RunHandler::commitRun, runInfoObj, glyphs, positions, clusters);
-        fEnv->SetLongField(runInfoObj, skija::shaper::RunInfo::_fontPtr, 0);
+        skija::AutoLocal<jobject> runInfoObj(fEnv, skija::shaper::RunInfo::toJava(fEnv, info, begin, end));
+        skija::AutoLocal<jshortArray> glyphs(fEnv, javaShortArray(fEnv, fGlyphs));
+        skija::AutoLocal<jobjectArray> positions(fEnv, skija::Point::fromSkPoints(fEnv, fPositions));
+        fEnv->CallVoidMethod(fRunHandler, skija::shaper::RunHandler::commitRun, runInfoObj.get(), glyphs.get(), positions.get(), clusters.get());
+        java::lang::Throwable::exceptionThrown(fEnv);
+        fEnv->SetLongField(runInfoObj.get(), skija::shaper::RunInfo::_fontPtr, 0);
     }
 
     void commitLine() {
         fEnv->CallVoidMethod(fRunHandler, skija::shaper::RunHandler::commitLine);
+        java::lang::Throwable::exceptionThrown(fEnv);
     }
 
 private: