Repository navigation
fix(android): wait for the page being rendered before recycle() closes the document - #1053
Open
Junveloper wants to merge 1 commit into
Open
Junveloper wants to merge 1 commit into
Junveloper wants to merge 1 commit into
Conversation
…s the document Co-authored-by: leonam-paula-fp <278753932+leonam-paula-fp@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1052.
On Android,
PDFView.recycle()closes the pdfium document on the UI thread while the "PDF renderer" thread may still be drawing a page from it.RenderingHandleronly catchesPageRenderingException, so the renderer thread throws and the app crashes. Depending on timing, the crash isGet page pdf document null,Already closed, or aNullPointerExceptioninPdfiumCore.renderPageBitmap. #1052 has the traces and a repro.This overrides
recycle()inPdfViewto drain the renderer before the base class closes the document:super.recycle()'sdispose()can't overlap a render, and no task can start afterpdfFileis cleared.Every recycle (a reload after a prop change, detaching from the window, a load error) calls the virtual
recycle(), so they all go through the override.RenderingDrainlives incom.github.barteksc.pdfviewerbecausePDFView.renderingHandlerandRenderingHandlerare package-private there. #1045 uses the same approach for its helper.The drain is the approach @leonam-paula-fp proposed in their review of #1045, so the commit credits them as co-author. #1045 moves
dispose()to the renderer thread but clearspdfFilefirst, and a task the renderer has already dequeued then hits aNullPointerExceptioninRenderingHandler.proceed. On the same reload loop, #1045 crashed that way in 2 of 3 runs (see the test plan). This PR replaces it. Thanks @GaelCO for the analysis there.The UI thread waits only for the page being drawn, if there is one. With temporary logging in one of the test runs below, the wait over 1,774 recycles was:
It never reached the 1 s timeout. That's an emulator reloading every 120 ms, so a page is almost always in flight.
PdfDocument.close()already takesPdfiumCore.lock, which pdfium holds while it draws, so the UI thread could already wait on a page being drawn before this change.Test Plan
The repro branch from #1052 adds a Start reload loop button to
FabricExamplethat flipsspacingevery 120 ms. Each flip runsdrawPdf()→Configurator.load()→recycle().masterwith the repro branch's two example commits, start the loop. The app crashes on the "PDF renderer" thread within a minute: 4 of 4 runs, after 102, 151, 160 and 324 loads.masterwith fix(android): defer PdfFile dispose to the rendering thread to fix SIGSEGV in libpdfium #1045 (head 2941304) and the same two commits, pdfium'sAlready closedwarnings are gone. But the app crashed in 2 of 3 runs, after 356 and 72 loads, withNullPointerException: Attempt to invoke virtual method 'boolean com.github.barteksc.pdfviewer.PdfFile.openPage(int)' on a null object referenceatRenderingHandler.proceed(RenderingHandler.java:91). The third run lasted 5 minutes (1,330 loads).master. pdfium logged noAlready closedwarnings (onemasterrun logged 28 before it crashed).onDetachedFromWindow()runsrecycle()mid-render. I did this 3 times, with no crash or ANR. The drain on detach took 537 ms, 3 ms and 0.1 ms. The 537 ms one ran while the host machine was heavily loaded.Checked on an Android 16 emulator (API 36, arm64), debug build.
Compatibility
iOS doesn't use AndroidPdfViewer.