Fix HermesNode property access on released JSI objects by kharrop · Pull Request #825 · player-ui/player

kharrop · 2026-03-17T15:07:55Z

This PR prevents a rare but high-impact Android crash in Hermes where the app can hit a native NullPointerException while reading JS backed objects. It does not change Player flow/state behavior; it only improves safety and diagnostics in a teardown race window.

Context

The crash occurs when native code tries to read a property from a Hermes JSI object that has already been released (often due to timing/lifecycle during teardown or navigation).

This shows up as a fatal NullPointerException inside Hermes/JSI property access (getProperty), which crashes the app process.

Testing

bazel test //jvm/hermes:test //jvm/core:test

Change Type (required)

Indicate the type of change your pull request is:

patch
minor
major
N/A

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

Release Notes

Added a defensive guard in HermesNode so property reads on a released runtime/object fail safely by throwing a PlayerRuntimeException with context instead of a fatal native NPE.

Added regression tests:

A deterministic unit test for “released JSI object property read”
A small concurrency test that reads player.state while player.release() is happening

📦 Published PR as canary version: 0.15.2--canary.825.33564

Try this version out locally by upgrading relevant packages to 0.15.2--canary.825.33564

kharrop · 2026-03-17T15:08:08Z

/canary

kharrop · 2026-03-17T15:19:34Z

/canary

kharrop · 2026-03-17T16:10:17Z

/canary

codecov · 2026-03-17T16:10:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (2ce16b6) to head (236f900).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #825   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

intuit-svc · 2026-03-17T16:36:06Z

Build Preview

Your PR was deployed by CircleCI #33564 on Tue, 17 Mar 2026 16:36:06 GMT with this version:

0.15.2--canary.825.33564

📖 Docs (View site)

… throughout, instead of only in the beginning

…ctive (another race condition leading to cancellationException instead of runtime released)

…d wrap

brocollie08 · 2026-03-31T16:55:22Z

-        }
-    }
+internal fun <T> Value.blockingLock(block: Value.() -> T): T = when {
+    context.runtime.lock.isHeldByCurrentThread -> block()


these shouldn't be impacted since they'll be on the same thread

brocollie08 · 2026-03-31T16:56:59Z

-            jsiObject.asValue(runtime)
-        }.let { format.decodeFromRuntimeValue(deserializer, it) }
+            format.decodeFromRuntimeValue(deserializer, jsiObject.asValue(runtime))
+        }


i wrapped the whole thing here and above. mapUndefineToNull as well as decoding can both run into similar race conditions since they need to reach the c++ layer

Don't those operations re-enter evaluateInJSThreadBlocking though? Would we be able to remove that? Or is keeping it in the same block just activating the shortcut?

Do we need to make these updates for the other node impls?

mapUndefinedToNull doesn't, decoding does, i moved decoding back out to minimize changes

sugarmanz

Looks fine — main concern is consistency across runtime impls. Even if functionally equivalent, the more it diverges, the more difficult it is to maintain.

sugarmanz · 2026-04-01T23:46:05Z

-            jsiObject.asValue(runtime)
-        }.let { format.decodeFromRuntimeValue(deserializer, it) }
+            format.decodeFromRuntimeValue(deserializer, jsiObject.asValue(runtime))
+        }


Do we need to make these updates for the other node impls?

sugarmanz · 2026-04-01T23:47:43Z

    // TODO: Put Dedicated Runtime Thread Context in the dispatcher context if we can?
    return if (currentRuntimeThreadContext != null) {
-        block(currentRuntimeThreadContext)
+        ensureNotReleased { block(currentRuntimeThreadContext) }


Why do ensureNotReleased here? You're ignoring it for this case in graal:
https://github.com/player-ui/player/pull/825/changes#r3017129498

Just want to be consistent.

yea you right, this shouldn't be necessary

sugarmanz · 2026-04-01T23:48:33Z

 }

 internal suspend fun <T> Runtime<*>.evaluateInJSThread(block: suspend RuntimeThreadContext.() -> T): T {
-    ensureNotReleased()


Should this whole function be wrapped in ensureNotReleased to avoid needing to call it multiple times?

sugarmanz · 2026-04-01T23:49:56Z

-        runBlocking {
-            evaluateInJSThread(runtime, block)
-        }
+        runBlocking { runtime.ensureNotReleased { evaluateInJSThread(runtime, block) } }


Do we need the check here if we're sure evaluateInJSThread will wrap block with it?

it does, though evaluateInJSThread has the guard as well

intuit-svc added a commit to player-ui/player-ui.github.io that referenced this pull request Mar 17, 2026

Deploying docs for player-ui/player#825

f9e4151

kharrop and others added 4 commits March 30, 2026 17:31

Fix HermesNode property access on released JSI objects

c0bc0a1

linting

7314af3

fixing tests

5c0d35a

ensureNotReleased takes in a block function to catch runtime released…

dbfd56a

… throughout, instead of only in the beginning

brocollie08 force-pushed the hermes-crash branch from 51ce679 to dbfd56a Compare March 31, 2026 00:05

brocollie08 added 6 commits March 30, 2026 20:44

Make test common across all runtimes, fix j2v8 test by remove ensureA…

aa3137c

…ctive (another race condition leading to cancellationException instead of runtime released)

lint

945033a

don't wrap the whole in thing in runblocking

008e0ca

some clean up

f0221af

move all of getSerializable and deserialize into the ensureNotRelease…

6fe5554

…d wrap

lint

ef3c835

brocollie08 reviewed Mar 31, 2026

View reviewed changes

brocollie08 marked this pull request as ready for review March 31, 2026 17:04

brocollie08 requested review from a team as code owners March 31, 2026 17:04

sugarmanz reviewed Apr 1, 2026

View reviewed changes

addressed some PR comments

236f900

brocollie08 requested a review from sugarmanz April 6, 2026 23:48

sugarmanz approved these changes Apr 7, 2026

View reviewed changes

brocollie08 merged commit 3257038 into main Apr 7, 2026
16 of 17 checks passed

brocollie08 deleted the hermes-crash branch April 7, 2026 19:00

Conversation

kharrop commented Mar 17, 2026 • edited by brocollie08 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Testing

Change Type (required)

Does your PR have any documentation updates?

Release Notes

Uh oh!

kharrop commented Mar 17, 2026

Uh oh!

kharrop commented Mar 17, 2026

Uh oh!

kharrop commented Mar 17, 2026

Uh oh!

codecov Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

intuit-svc commented Mar 17, 2026

Build Preview

📖 Docs (View site)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sugarmanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kharrop commented Mar 17, 2026 •

edited by brocollie08

Loading

codecov Bot commented Mar 17, 2026 •

edited

Loading