Skip to content

fix(android): reset readiness when the editor crashes - #640

Open
dcalhoun wants to merge 6 commits into
fix/reset-host-readiness-on-editor-crashfrom
fix/android-reset-readiness-on-editor-crash
Open

dcalhoun wants to merge 6 commits into
fix/reset-host-readiness-on-editor-crashfrom
fix/android-reset-readiness-on-editor-crash

Conversation

@dcalhoun

@dcalhoun dcalhoun commented Sep 9, 2026

Copy link
Copy Markdown
Member

What?

Android never reset isEditorLoaded, so calls into the editor kept reaching the web view after it crashed.

Why?

The Android half of CMM-2008.

After a crash, history and content commands throw inside the web view, content reads fail with a JSON error rather than reporting the editor unavailable, and hosts can't tell the editor is gone.

isEditorLoaded was set in onEditorLoaded and never cleared, and several calls into the editor didn't check it.

How?

  • onEditorUnavailable clears isEditorLoaded, dismisses an open block inserter, and notifies the new EditorUnavailableListener, set with setEditorDidBecomeUnavailable.
  • Calls into the editor go through evaluateIfLoaded, which refuses them until the editor loads, logs refusals at debug level, and runs them on the main thread. Content reads report EditorNotReadyException.

Note for review

Robolectric needs merged resources to show the block inserter, so the module's unit tests include Android resources, and GutenbergViewTest no longer sets manifest = Config.NONE.

Testing Instructions

In the demo app, trip the boundary from chrome://inspect, in either the visual or code editor:

const editor = wp.data.select("core/editor");
editor.getEditorMode = () => {
  throw new Error("repro");
};
wp.data.dispatch("core/editor").updateEditorSettings({});
  1. adb logcat | grep GutenbergView shows EditorUnavailable received in native code.
  2. Tap Undo or Redo — no TypeError appears in the web view console.
  3. Repeat with the block inserter open — it closes.

Regression check, in a fresh session with no crash: editing, undo/redo, block insertion and saving are unchanged.

Accessibility Testing Instructions

N/A — no UI changes.

Screenshots or screencast

N/A

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5

@github-actions github-actions Bot added the [Type] Bug An existing feature does not function as intended label Sep 9, 2026
@dcalhoun
dcalhoun added this pull request to stack #641 September 9, 2026 16:24
@wpmobilebot

wpmobilebot commented Sep 9, 2026

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/640")

Built from f9338dd

dcalhoun and others added 6 commits September 15, 2026 19:48
`isEditorLoaded` was set once and never cleared — not on an editor crash, and
not even on WebView process termination, which iOS does handle. Every guard
built on it therefore kept passing after the editor's `ErrorBoundary` unmounted
the editor and deleted the `window.editor.*` bridge methods.

Receive the `onEditorUnavailable` message the editor now emits, clear
readiness, and expose an `EditorUnavailableListener` so hosts can disable the
controls that depend on the editor while leaving saving and closing available.

This is a second line of defense behind #639, which stops the one call site
that turned a failed read into an empty title. Here the calls stop being made
at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VuxMbKtUsaUF8nUVgKdxwK
`undo`, `redo` and `dismissTopModal` never checked `isEditorLoaded`, so
resetting it left them unaffected: after a crash they still evaluated
JavaScript against methods that had been deleted, throwing inside the web view
where nothing on the Kotlin side could see it.

iOS already guards all three with `guard isReady`. Match that, so readiness
means the same thing on both platforms.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VuxMbKtUsaUF8nUVgKdxwK
A crash before the editor finishes loading reports it unavailable without a
preceding `onEditorAvailable`, so hosts should not assume that order.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
The editor now catches crashes with its own error boundary, which shows an
error message rather than Gutenberg's `ErrorBoundary` fallback. Update the
unavailable listener and callback docs to match.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
Each bridge call checked readiness on its own, and inconsistently. Content
changes logged a refusal as an error, although a crashed or reloading editor
now makes it expected, while history and inserter calls were refused
silently. `setContent`, `setTitle` and `setMediaUploadAttachment` also called
the web view on the caller's thread rather than the main thread.

Route them through `evaluateIfLoaded`, which posts to the main thread and logs
each refused call at debug level, as iOS does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
A block inserter open when the editor crashed stayed on screen, and blocks
picked from it were silently refused.

Dismiss it when the editor becomes unavailable. The view tests now load the
merged Android resources so Robolectric can show the inserter.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
@dcalhoun
dcalhoun force-pushed the fix/android-reset-readiness-on-editor-crash branch from ac35e2f to f9338dd Compare September 16, 2026 00:28
@dcalhoun
dcalhoun marked this pull request as ready for review September 16, 2026 12:04
@dcalhoun
dcalhoun requested a review from adalpari September 16, 2026 12:04
@adalpari

Copy link
Copy Markdown
Contributor

It looks good to me, so I'm approving it.

Claude found a few points. I think some of them are worth noting for you to have a look:

  1. Permanent spinner when the editor crashes before it loads — HIGH
    GutenbergView.kt:973 — onEditorUnavailable() resets isEditorLoaded and dismisses the inserter, but never transitions the UI phase. showReadyPhase() only runs inside the didFireEditorLoaded guard in onEditorLoaded() (line 944), so if the crash lands first, spinnerView stays visible and webView.alpha stays 0f forever. The error boundary’s “The editor has encountered an unexpected error.” message is rendered but invisible behind the alpha-0 WebView, and there’s no reload path to escape. The user sees an infinite spinner. This is the exact case the new KDoc calls out. iOS has the same gap (EditorViewController.swift:937).

  2. setMediaUploadAttachment loses the context id on refusal — MEDIUM
    GutenbergView.kt:1050-1061 — currentMediaContextId = null now runs unconditionally after evaluateIfLoaded(...). When the call is refused, the attachment is dropped and the id is gone, so any retry fails with “called without contextId”. The pre-PR early return preserved it.

  3. textEditorEnabled still bypasses the central guard — MEDIUM
    GutenbergView.kt:214-221 — the setter calls handler.post { webView.evaluateJavascript("editor.switchEditorMode('$mode');") } directly. It’s the one remaining raw editor.* call and will throw a TypeError after a crash, which undercuts the PR’s stated goal of refusing editor calls in one place. One-line fix: route it through evaluateIfLoaded.

@adalpari adalpari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dcalhoun

Copy link
Copy Markdown
Member Author

@adalpari thank you for the review.

  1. Permanent spinner when the editor crashes before it loads — HIGH

Given this is not a regression in #640 and is also fixed in #642, I suggest we let the latter address it.

  1. setMediaUploadAttachment loses the context id on refusal — MEDIUM

This appears to accurately describe the diff but the issue is benign.

The retained ID is dead as the JS callbackRegistry entry is deleted when the media upload hook unmounts, and a reload resets both the registry and the ID counter. Preserving the ID only swaps one failure for another: No callback found for contextId from JS instead of called without contextId from Kotlin. Neither recovers the attachment, so discarding the stale ID seems like the better of the two. I suggest we leave it.

  1. textEditorEnabled still bypasses the central guard — MEDIUM

This is addressed in #642. Given it has no user impact in this branch, I suggest we let #642 deliver the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants