Honor AlreadyClosedBehavior in openPage/openTextPage (v1.0.35-based, for the Expensify JitPack pin)#2
Honor AlreadyClosedBehavior in openPage/openTextPage (v1.0.35-based, for the Expensify JitPack pin)#2wildan-m wants to merge 3 commits into
Conversation
These were the only PdfDocument methods using an unconditional check(!isClosed) instead of handleAlreadyClosed(), so a configured AlreadyClosedBehavior.IGNORE never reached them. A render that lands on a just-closed document threw IllegalStateException on the renderer thread (PdfiumCore.renderPageBitmap -> openPage), which the barteksc RenderingHandler does not catch, crashing the app. Now they route through handleAlreadyClosed() like every sibling method and, when IGNORE is set, return a page bound to the closed document so all further ops no-op via the existing doc.isClosed guards.
|
How to land this — approve + tag, don't merge. This fork only needs to host the pinned build, so this doesn't merge into
Then I'll repoint the App patch (Expensify/App#94894) from |
|
Looking into this, permissions need to be updated |
|
Thanks @mountiny! Pull-only here, so the tag's on your side — could you tag |
There was a problem hiding this comment.
@wildan-m, I don't think this should be needed, shouldn't this already be published to our local Maven repository?
There was a problem hiding this comment.
@eVoloshchak nothing's pre-published — com.github.Expensify:… is a JitPack coordinate, so JitPack builds this repo from source on demand, and without jitpack.yml it fails (its default JDK 11 can't run the Hilt plugin, which needs 17 — tag .1 errored with No build artifacts found).
|
@wildan-m Can you check if this is right? com.github.Expensify:PdfiumAndroidKt:expensify-already-closed-fix-v1 The tag was created |
|
Thanks @mountiny! 🙏 One snag: JitPack's build of Could you hit Rebuild on the |
|
Update: to unblock in the meantime I've pointed patch 004 at the commit coordinate ( |
3 participants
Brings the "Already closed" fix into
Expensify/PdfiumAndroidKtso JitPack can buildcom.github.Expensify:PdfiumAndroidKt:1.0.35-already-closed-fix.3— the coordinate thatExpensify/App'sreact-native-pdfpatch pins (Expensify/App#94894).What it fixes: on Android,
PdfDocument.openPage()/openTextPage()throwIllegalStateException("Already closed")when a render lands on a just-closed document. They're the only two methods that don't route throughhandleAlreadyClosed(), so they ignore the app's configuredAlreadyClosedBehavior.IGNORE. That's the crash behind SentryAPP-1VG. This change makes both honor the config (returning a no-op page bound to the closed document, guarded like every other method) instead of throwing on the renderer thread.3 commits on top of the
v1.0.35release:Honor AlreadyClosedBehavior in openPage/openTextPage— the fix (one file,PdfDocument.kt)Add jitpack.yml+accept Android SDK/NDK licenses— so JitPack builds only the:pdfiumandroidmodulev1.0.35release, notmain(which is the newer 3.x redesign with a differentopenPageAPI). The intended outcome is to tag this branch1.0.35-already-closed-fix.3so JitPack builds the coordinate above — it isn't meant to be merged into the 3.xmain(it'll show a conflict there). Happy to retarget if you'd prefer a dedicated1.0.35base branch.