Skip to content

fix(android): wait for the page being rendered before recycle() closes the document - #1053

Open
Junveloper wants to merge 1 commit into
wonday:masterfrom
Junveloper:fix/android-drain-renderer-before-recycle
Open

Junveloper wants to merge 1 commit into
wonday:masterfrom
Junveloper:fix/android-drain-renderer-before-recycle

Conversation

@Junveloper

Copy link
Copy Markdown

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. RenderingHandler only catches PageRenderingException, so the renderer thread throws and the app crashes. Depending on timing, the crash is Get page pdf document null, Already closed, or a NullPointerException in PdfiumCore.renderPageBitmap. #1052 has the traces and a repro.

This overrides recycle() in PdfView to drain the renderer before the base class closes the document:

  • It stops the renderer and drops its queued render tasks.
  • It posts a marker to the front of the renderer's queue and waits, for up to 1 s, for it to run.
  • Once the marker has run, the page that was being drawn has finished and the queue is empty. So super.recycle()'s dispose() can't overlap a render, and no task can start after pdfFile is 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.

RenderingDrain lives in com.github.barteksc.pdfviewer because PDFView.renderingHandler and RenderingHandler are 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 clears pdfFile first, and a task the renderer has already dequeued then hits a NullPointerException in RenderingHandler.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:

  • median 4.4 ms;
  • p95 26 ms;
  • p99 103 ms;
  • max 394 ms.

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 takes PdfiumCore.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 FabricExample that flips spacing every 120 ms. Each flip runs drawPdf() → Configurator.load() → recycle().

  1. On master with 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.
  2. On master with fix(android): defer PdfFile dispose to the rendering thread to fix SIGSEGV in libpdfium #1045 (head 2941304) and the same two commits, pdfium's Already closed warnings are gone. But the app crashed in 2 of 3 runs, after 356 and 72 loads, with NullPointerException: Attempt to invoke virtual method 'boolean com.github.barteksc.pdfviewer.PdfFile.openPage(int)' on a null object reference at RenderingHandler.proceed(RenderingHandler.java:91). The third run lasted 5 minutes (1,330 loads).
  3. On this branch with the same two commits, start the loop and leave it for 5 minutes. 3 of 3 runs finished with no crash: 5,429 loads in total, at the same load rate as master. pdfium logged no Already closed warnings (one master run logged 28 before it crashed).
  4. With the loop running, press back to close the activity, so onDetachedFromWindow() runs recycle() 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

OS Implemented
iOS ❌
Android ✅

iOS doesn't use AndroidPdfViewer.

…s the document

Co-authored-by: leonam-paula-fp <278753932+leonam-paula-fp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Android: reloading or closing a PDF while a page is rendering crashes the app (PDF renderer thread)

1 participant