From 6a08dfd5c8cbce8a44594dd2973a9d3626bf0518 Mon Sep 17 00:00:00 2001 From: Nikita Prokopov Date: Thu, 29 Apr 2021 00:38:33 +0200 Subject: [PATCH] Fixed all `-Xcheck:jni` warnings #70 --- CHANGELOG.md | 6 ++++ examples/kwinit/script/run.py | 2 +- examples/lwjgl/script/run.py | 1 + examples/scenes/src/Scenes.java | 2 +- native/src/Codec.cc | 7 +++-- native/src/Drawable.cc | 12 +++++-- native/src/Font.cc | 3 +- native/src/Picture.cc | 5 ++- native/src/interop.cc | 55 ++++++++++++++++++++------------- native/src/interop.hh | 24 ++++++++++++++ native/src/shaper/Shaper.cc | 42 +++++++++++++++---------- 11 files changed, 111 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3663f205..846783cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# 0.91.2 - Apr 29, 2021 + +Fixed: + +- `-Xcheck:jni` warnings #70 + # 0.91.1 - Apr 28, 2021 Fixed: diff --git a/examples/kwinit/script/run.py b/examples/kwinit/script/run.py index 4a080a15..c3377e19 100755 --- a/examples/kwinit/script/run.py +++ b/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) diff --git a/examples/lwjgl/script/run.py b/examples/lwjgl/script/run.py index ecf3e481..9e38f9cc 100755 --- a/examples/lwjgl/script/run.py +++ b/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']) diff --git a/examples/scenes/src/Scenes.java b/examples/scenes/src/Scenes.java index 72e7bcff..57b96444 100644 --- a/examples/scenes/src/Scenes.java +++ b/examples/scenes/src/Scenes.java @@ -6,7 +6,7 @@ public class Scenes { public static TreeMap 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; diff --git a/native/src/Codec.cc b/native/src/Codec.cc index 693288b8..971be63b 100644 --- a/native/src/Codec.cc +++ b/native/src/Codec.cc @@ -75,10 +75,11 @@ extern "C" JNIEXPORT jobject JNICALL Java_org_jetbrains_skija_Codec__1nGetFrames SkCodec::FrameInfo info; std::vector 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 infoObj(env, skija::AnimationFrameInfo::toJava(env, frames[i])); + env->SetObjectArrayElement(res, i, infoObj.get()); } return res; } diff --git a/native/src/Drawable.cc b/native/src/Drawable.cc index 0ced5bd8..0114cbdb 100644 --- a/native/src/Drawable.cc +++ b/native/src/Drawable.cc @@ -9,26 +9,32 @@ class SkijaDrawableImpl: public SkDrawable { } ~SkijaDrawableImpl() { - fEnv->DeleteWeakGlobalRef(fObject); + JNIEnv* env; + if (fJavaVM->GetEnv(reinterpret_cast(&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(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 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; }; diff --git a/native/src/Font.cc b/native/src/Font.cc index 9dcb2539..a3ad52a4 100644 --- a/native/src/Font.cc +++ b/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 boundsObj(env, skija::Rect::fromSkRect(env, bounds[i])); + env->SetObjectArrayElement(res, i, boundsObj.get()); } return res; diff --git a/native/src/Picture.cc b/native/src/Picture.cc index 3c5d917c..3b86bcd4 100644 --- a/native/src/Picture.cc +++ b/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; diff --git a/native/src/interop.cc b/native/src/interop.cc index 3d51f9bc..4b76bd74 100644 --- a/native/src/interop.cc +++ b/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(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(i.fAlphaType), - i.fHasAlphaWithinBounds, - static_cast(i.fDisposalMethod), - static_cast(blend), - IRect::fromSkIRect(env, i.fFrameRect)); + jobject res = env->NewObject(cls, ctor, + i.fRequiredFrame, + i.fDuration, + i.fFullyReceived, + static_cast(i.fAlphaType), + i.fHasAlphaWithinBounds, + static_cast(i.fDisposalMethod), + static_cast(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 features(featuresLen); for (int i = 0; i < featuresLen; ++i) { - jobject featureObj = env->GetObjectArrayElement(featuresArr, i); - features[i] = {static_cast(env->GetIntField(featureObj, skija::FontFeature::tag)), - static_cast(env->GetIntField(featureObj, skija::FontFeature::value)), - static_cast(env->GetLongField(featureObj, skija::FontFeature::start)), - static_cast(env->GetLongField(featureObj, skija::FontFeature::end))}; - env->DeleteLocalRef(featureObj); + skija::AutoLocal featureObj(env, env->GetObjectArrayElement(featuresArr, i)); + features[i] = {static_cast(env->GetIntField(featureObj.get(), skija::FontFeature::tag)), + static_cast(env->GetIntField(featureObj.get(), skija::FontFeature::value)), + static_cast(env->GetLongField(featureObj.get(), skija::FontFeature::start)), + static_cast(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 toSkIRect(JNIEnv* env, jobject obj) { @@ -496,7 +497,8 @@ namespace skija { jobjectArray fromSkPoints(JNIEnv* env, const std::vector& 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 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(nullptr); return std::unique_ptr(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(nullptr); uint32_t flags = static_cast(env->CallIntMethod(surfacePropsObj, _getFlags)); + if (java::lang::Throwable::exceptionThrown(env)) + std::unique_ptr(nullptr); SkPixelGeometry geom = static_cast(env->CallIntMethod(surfacePropsObj, _getPixelGeometryOrdinal)); + if (java::lang::Throwable::exceptionThrown(env)) + std::unique_ptr(nullptr); return std::make_unique(flags, geom); } } @@ -1009,8 +1018,10 @@ std::vector skStringVector(JNIEnv* env, jobjectArray arr) { jobjectArray javaStringArray(JNIEnv* env, const std::vector& 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 str(env, javaString(env, strings[i])); + env->SetObjectArrayElement(res, i, str.get()); + } return res; } diff --git a/native/src/interop.hh b/native/src/interop.hh index d6487094..fd6564f0 100644 --- a/native/src/interop.hh +++ b/native/src/interop.hh @@ -1,4 +1,5 @@ #pragma once +#include #include #include #include @@ -85,6 +86,29 @@ namespace skija { jobject toJava(JNIEnv* env, const SkCodec::FrameInfo& i); } + template + 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; diff --git a/native/src/shaper/Shaper.cc b/native/src/shaper/Shaper.cc index 79cb4890..69e7b46d 100644 --- a/native/src/shaper/Shaper.cc +++ b/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 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 { jint onConsume(jobject runObj) override { jlong fontPtr = fEnv->CallLongMethod(runObj, skija::shaper::FontRun::_getFontPtr); + java::lang::Throwable::exceptionThrown(fEnv); fFont = reinterpret_cast(static_cast(fontPtr)); return fEnv->GetIntField(runObj, skija::shaper::FontRun::_end); } @@ -245,16 +249,19 @@ 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 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) { @@ -262,12 +269,13 @@ class SkijaRunHandler: public SkShaper::RunHandler { fPositions = std::vector(info.glyphCount); fClusters = std::vector(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 runInfoObj(fEnv, skija::shaper::RunInfo::toJava(fEnv, info, fIndicesConverter)); + skija::AutoLocal 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(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 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 runInfoObj(fEnv, skija::shaper::RunInfo::toJava(fEnv, info, begin, end)); + skija::AutoLocal glyphs(fEnv, javaShortArray(fEnv, fGlyphs)); + skija::AutoLocal 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: