Fix main thread hang on iOS - #772
Conversation
|
Summary of the PR. Please correct me if I misunderstood something
Regarding the questions
|
|
Hey! I think the questions from the PR description were covered according to your answers @war-in! |
|
That's great! Could we link this PR as a git dependency in your E/App PR, so C+ could jump in and test that solution in the Expensify app? We should make sure we won't introduce any regressions 🙏 |
|
The changes can be tested via this PR! |
| handler(); | ||
| } | ||
| }]; | ||
| return; |
There was a problem hiding this comment.
This early returns before MarkdownFormatter runs whenever the measure pass is on the main thread and the parser cache misses.
Before, parse: was synchronous, so the string handed to setStateData (MarkdownTextInputDecoratorShadowNode.mm:308, :323) was always formatted.
formatAttributedString is what applies h1 font size, code/pre font, blockquote indent and emoji size — so skipping it changes the measured height.
The first measure after an input mounts is a guaranteed miss: markdownUtils_ (and its empty MarkdownParser cache) is created one line earlier at :224.
Repro steps
Example app:
- The input is pre-filled with
# header1`, `> blockquote`, ```codeblock```, 😀🍕🍔
Note how tall the input box is and that all of it is visible inside the border.
2. Rotate the device to landscape.
3. Rotate back to portrait.
Expected: the box stays sized to the formatted text through both rotations.
Actual: on the rotation the box is briefly sized to plain-text metrics — the header line and code block lose their extra height, so the bottom lines sit outside or flush against the grey border before it snaps back.
E/App:
- On iPad, open a chat and paste into the composer:
# heading
```code```
😀😀😀
- Put the app in Split View and drag the divider, or rotate the iPad.
- The composer collapses toward single-line height for a frame before correcting.
There was a problem hiding this comment.
main
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-08-20.at.10.21.55.mov
There was a problem hiding this comment.
This branch
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-08-20.at.10.31.49.mov
There was a problem hiding this comment.
Apparently the behaviour is exactly the same and there is no glitch
| // Dirty both nodes explicitly instead of relying on | ||
| // YGNodeMarkDirty()'s upward propagation: the child is usually already dirty | ||
| // from completeClone(), in which case propagation short-circuits and the | ||
| // decorator would stay clean. Ancestors then pick the dirty flag up on their | ||
| // own, because they are cloned with new children and updateYogaChildren() | ||
| // propagates child dirtiness upwards. | ||
| if (needsRemeasure_ != nullptr && needsRemeasure_->exchange(false)) { | ||
| yogaNode->setDirty(true); | ||
| yogaNode_.setDirty(true); | ||
| } |
There was a problem hiding this comment.
The wrong height stays wrong instead of snapping back
Repro step: (example app)
- Tap the input and start typing continuously into the middle of the text (each keystroke is a commit, which is what makes the recovery commit lose the revision race).
2.. While still typing, rotate the device to landscape. - Stop typing and look at the box.
- If it recovered, tap Blur, rotate back, and repeat from step 1. It typically takes a handful of tries.
Expected: box returns to formatted height once rotation settles.
Actual: the box stays at the plain-text height with the last lines clipped, and stays there indefinitely. It only corrects on the next keystroke (which changes the text and forces a fresh measure). Blur/focus, scrolling, and further rotation do not fix it.
There was a problem hiding this comment.
Can't see any regression:
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-08-20.at.10.33.46.mov
|
I've verified the AI comments and couldn't see any regression that was pointed out as potential |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios1.movios2.moviOS: mWeb SafariMacOS: Chrome / Safari |
| // the calling thread. Used when the main thread needs ranges during layout but | ||
| // must not wait on the worklet runtime (see Sentry APP-EF1). | ||
| // | ||
| // Requests are coalesced latest-wins: the most recent (text, parserId) always |
There was a problem hiding this comment.
Let's update and simplify all the comments to be more human readable.
i.e. coalesced
4 participants
Details
On iOS,
RCTMarkdownUtilsandMarkdownParsernest an Objective-C lock outside the markdown worklet runtime'srecursive_mutex. That ordering lets a background thread hold the ObjC lock while it waits on the runtime mutex, which blocks the main thread inobjc_sync_enterduring Yoga measure until the iOS watchdog kills the app.Today's code:
Both locks are taken before entering the runtime, and the parse is synchronous, so the lock is held for the entire duration of the worklet call.
The two threads in the captured hang:
com.facebook.react.runtime.JavaScript— holds both@synchronizedlocks, blocked on the runtime mutex:Main thread — same measure path, blocked acquiring the lock the thread above is holding:
Fatal App Hang Fully Blocked— main thread blocked ≥ 2000 ms, culprit-[RCTMarkdownUtils applyMarkdownFormatting:withDefaultTextAttributes:markdownStyle:parserId:].The fix
Two changes, both aimed at removing "main thread waits on a runtime-bound lock":
1.
MarkdownParser— never hold an ObjC lock across the runtime call.parse:is split into a cache lookup, an unlockedparseUncached:that enters the runtime, and a locked memo write. The lock now only guards the_prev*ivars, so it is never held while waiting onrecursive_mutexand the inversion is gone. Two concurrent misses may parse the same text twice; the runtime serializes them, results are identical, and last-writer-wins on a single-entry memo is safe.Two new entry points:
cachedRangesForText:withParserId:— memo lookup only, never touches the runtime, safe from the measure path on the main thread.warmCacheAsyncForText:withParserId:— schedules a parse on a serialQOS_CLASS_USER_INITIATEDqueue, guarded by an_asyncParseInFlightflag so warm-ups don't pile up.2.
RCTMarkdownUtils— main thread formats from cache only.The lock narrows to the
_markdownStyle/_parserIdwrites; formatting then uses the local parameters rather than re-reading the ivars, which preserves the per-call consistency the original single lock was there to provide. On a cache miss:parse:no longer holds any lock the main-thread measure path can block on, and the watchdog only monitors the main thread.Tradeoffs / things I'd like reviewer input on
Flagging these because they're real and I'd rather they get caught here:
A main-thread cache miss measures unformatted text. Styles that change metrics (
h1font size,pre/codefont, blockquote indent, emoji font size) would measure wrong for that pass. In practice the main thread should almost always hit the cache, because text changes are committed and parsed on background threads first — but "almost always" is doing some work in that sentence.Nothing explicitly re-measures after the async warm-up lands. The comment says a subsequent measure/commit picks up the cached ranges, and that's true whenever something else dirties layout, but there's no invalidation triggered by the warm-up itself. If Fabric has cached the measurement for that shadow node, a wrong height could persist until the text changes again. Should
warmCacheAsyncForText:complete by dirtying the shadow node / requesting a new commit? That would make convergence guaranteed rather than incidental.The memo is single-entry, and now shared with a background writer. Two alternating text values thrash it, and in that state the main thread would rarely hit the cache — falling into case 1 repeatedly. A small LRU (even 2–4 entries) keyed on
(text, parserId)would make the main-thread fast path much more reliable. Happy to add it here if you'd prefer._asyncParseInFlightcan skip the newest text. If a warm-up is already running for stale text, the newer text's warm-up is dropped and relies on a later measure pass observing the miss. Combined with (2), there's a path where it doesn't converge promptly. Coalescing to "latest requested text" instead of a boolean would be tighter.An alternative worth considering: keep formatting synchronous everywhere and instead make the runtime entry non-blocking, or ensure the parse always happens on commit (before measure) so the measure path is cache-only by construction. That's a larger change; this PR is scoped to stopping the watchdog kills.
Related Issues
Expensify/App#93831
Internal: Sentry
APP-EF1(https://expensify.sentry.io/issues/APP-EF1)Manual Tests
There is no ObjC test harness in this repo, so this was verified manually. Verified in the Expensify app (HybridApp, iOS) carrying this as a
patch-packagepatch over@expensify/react-native-live-markdown@0.1.334:code,pre, blockquote, headings, mentions, links, emoji.parserIdapplied).Reviewers: please pay particular attention to composer height on first render of a heavily formatted draft — that's where tradeoff (1)/(2) above would show up as a layout jump or wrong height.
Linked PRs