Display: an init() during the old EDT's teardown starts its own dispatch thread - #5694
Open
shai-almog wants to merge 6 commits into
Open
Display: an init() during the old EDT's teardown starts its own dispatch thread#5694shai-almog wants to merge 6 commits into
shai-almog wants to merge 6 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 938c78d467
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 811f7c9ed1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
…tch thread
This is the bug behind the intermittent
FormTest timed out after 5000ms; edt=display-not-initialized
that has failed a different core-unittests class each time it appeared --
ValidatorTest, AutoCompleteTextComponentTest -- and that never reproduced
locally, because the window is only open while a loaded machine is descheduling
a thread.
A thread that has left mainEDTLoop's loop stays isAlive() for the whole of its
teardown: Desktop.disposeAll() and impl.deinitialize(), either of which can
block. init() decided whether to start a dispatch thread on exactly that
evidence, so an init() landing in that window adopted a thread on its way out and
started nothing. The new generation then has no dispatch at all -- everything it
queues waits for ever and isInitialized() stays false while codenameOneRunning
stays true, a state init() cannot repair because it guards on that flag. Every
test in the class times out.
The fix publishes the fact instead of inferring it. edtDispatching is renounced
by the departing thread inside the same monitor init() takes to decide adoption,
so the two orderings become the two correct outcomes rather than a race: either
the thread renounces first and init() starts a dispatch thread, or init() gets
there first -- and it has already set codenameOneRunning back to true, so the
thread's next test keeps it in the loop, which is the adoption that is
legitimately free. Claimed in init() rather than by the new thread, because
between start() and its first instruction a thread is alive and not yet
dispatching.
Two things that look like tidying are load-bearing:
- The thread stays the recorded edt until the END. The teardown is meant to run
AS the EDT -- Desktop.disposeAll() disposes windows on the thread their tree
expects -- so clearing edt early to dodge adoption, the obvious first fix,
makes isEdt() false for exactly that call.
- The final clear is now guarded on identity. Once adoption stops, the two
generations overlap by design, and clearing the field unconditionally disowned
the LIVE successor: isEdt() then answered false on the dispatch thread itself.
The harness has been patched for this twice, in UITestBase and
FormTestInterceptor, and both comments treat it as test infrastructure. They took
ValidatorTest from twelve failures to one; neither can close the window, because
the harness is not what is wrong. Their recoveries are left alone -- they also
cover the unrelated half-initialised state DisplayRecoveryTest describes.
EdtHandoverTest holds the window open with an implementation that blocks inside
deinitialize() rather than racing for it. Both tests fail on master in 10.8s and
pass in 1.9s, and each half is proven alone: removing only the identity guard
fails the second test and leaves the first passing.
Full core-unittests module green; SpotBugs, PMD, copyright and control-character
gates clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CompareObjectsWithEquals is on the PR quality gate's forbidden list, and it fails the build for any finding. Identity is exactly what this comparison needs -- the question is whether the departing thread is still the recorded EDT, and a Thread that considered itself equal to another would still be a different thread -- so it takes the marker the rest of the tree uses for the same situation rather than a rewrite. Found only because the identical construct in the continuity branch tripped the check there. The gate run on this branch had covered SpotBugs and not the forbidden PMD list, so this would have gone red in CI. core-unittests: forbidden PMD 0, SpotBugs 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
The adoption fix closed one of the two ways a Display generation ends up reporting display-not-initialized. This closes the rest of them. Now that an init() during the old thread's teardown starts a dispatch thread of its own instead of adopting a departing one, the two generations overlap by design -- and the teardown was still reading the process-wide singletons rather than saying which generation it was tearing down: - impl is a single slot that init() overwrites, so `impl.deinitialize()` after the loop deinitialized the SUCCESSOR's implementation. That leaves isInitialized() false while codenameOneRunning stays true, a state init() cannot repair because it guards on that flag -- the same unrepairable half-up display, reached through the other door. The harness already describes this failure in UITestBase's own comment. - Identity scoping is not enough on its own, because the implementation may be SHARED: UITestBase reuses one TestCodenameOneImplementation from class to class, so "tear down the one I served" is still the live one. The teardown now asks whether the implementation is IN SERVICE. - Desktop is a process singleton with one window registry, so disposeAll() from a superseded thread closes the successor's windows, on the successor's dispatch thread. Two more races in the same handover: - The exception exit tested codenameOneRunning and cleared edtDispatching afterwards, which is the race keepDispatching() exists to close, reopened on the rarer path. It goes through the same helper now. - init() claimed edtDispatching under the lock but recorded `edt` outside it. The two fields describe one thread, and both the next init() and the departing thread's own teardown read them together. And the nondeterminism underneath all of it: the EDT's idle wait tested only shouldEDTSleep(), so an idle EDT woken by deinitialize()'s notify found itself still idle and waited again. "Closes down the EDT" only actually did when something else happened to hand the thread work; otherwise it stayed parked for ever, alive and flagged as dispatching. Whether a deinitialize() ended a generation or left its thread to be adopted decided itself on whether the display happened to be idle at that instant. codenameOneRunning is part of that condition now. EdtHandoverTest gains the two orderings the existing fixture could not reach: it parks the departing thread inside Desktop.disposeAll(), which is before the implementation to tear down is read, where parking inside deinitialize() is after the receiver has already been resolved. Each new test was verified to fail with only its own half of the fix reverted. core-unittests 6144/6144; SpotBugs, PMD and Checkstyle 0. The handover class ran 40 times under CPU load with no failure, where before the idle-wait fix it failed roughly one run in twenty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
fix/edt-handover-adoption
branch
from
September 4, 2026 12:51
811f7c9 to
9c16f05
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c16f05db3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Cloudflare Preview
|
The previous commit tried to make an overlapping teardown safe by asking, before each step, whether it had been superseded. build-test (8) showed why that cannot work: PreferencesTest lost all five of its tests to display-not-initialized. The departing thread asked while it was still the recorded EDT, was descheduled, and an init() completed in the gap -- so the deinitialize() that followed cleared the flag initImpl had just set. A check and the call after it are two statements, and a thread being descheduled between two statements is this entire class of failure. The damage needs no distinct implementations to happen. UITestBase reuses one TestCodenameOneImplementation from class to class and the Android factory returns AndroidImplementation.getInstance(), so the departing generation and its successor routinely hold the SAME object: identity scoping cannot separate them, and neither can any other check made ahead of the call. So the two are ordered rather than guarded. The departing thread claims the teardown inside the same monitor and at the same instant it renounces edtDispatching -- provably before any init() can have begun, because init()'s first act is to set codenameOneRunning true and the renunciation happens with it false. init() waits for that claim to be released. Generations no longer overlap, which is what makes the process-wide state they share -- the impl slot, the implementation's initialized flag, Desktop's single window registry -- safe to touch at all. The wait is bounded. The teardown is the PORT's, and a port is entitled to need a thread that may be the one calling init(): Android hands the tail of its teardown to the activity UI thread, which is also where init() is called from. Waiting indefinitely would turn a slow teardown into a dead application, so on expiry the generations overlap exactly as before and the superseded-checks in the teardown are what covers it. The claim is also released on the exception exit, which runs no teardown at all, and in a finally, so a throwing teardown cannot leave init() waiting out the whole bound. EdtHandoverTest asserts the invariant directly now: an init() racing a parked teardown must NOT run to completion beside it. Verified by deleting the wait, which fails that assertion and takes a second test with it. core-unittests 6144/6144; SpotBugs, PMD forbidden rules and Checkstyle all 0, read out of this checkout's own reports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e25a52c13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review follow-up. One finding taken, two rejected with the reasoning put where it will be read -- in the code, not in a PR thread. Taken: two initializers parked in awaitPreviousTeardown() are released by a single notifyAll, so both would wake, both find codenameOneRunning false and both go on to build a generation, overwriting each other's impl mid-initImpl(). init() always had that check-then-set unsynchronized, but the wait is what synchronizes two callers into hitting it together. The wait and the claim are now one critical section. Rejected, at releaseTeardownClaim(): that the claim should cover AndroidImplementation.deinitialize()'s asynchronous half. It reads two statements too early. startContext() waits out deinitializingEdt and then calls deinitialize() again FROM the UI thread, where it runs its Runnable inline rather than posting it, and it does that before reinit() calls instance.init(i); the posted copy then returns at its own if (!deinitializing) guard. Nothing nulls a freshly initialized myView. The remedy is also worse than the defect it describes: Display.init() runs ON the UI thread during an activity restart, so waiting for work queued on that thread parks the thread that has to run it, and only the bound would break the deadlock -- ten seconds per restart, with the cleanup landing afterwards anyway. Rejected, at the disposeAll() guard: that the ownership check be made atomic with the disposal. It cannot be. Atomicity means holding the display lock across disposeAll(), which fires application window listeners and calls wm.hide/wm.dispose in the port. What makes the sequence safe is that init() waits rather than races; the check is what remains for the expiry case, and narrowing that window to two adjacent statements is all that is available. core-unittests 6144/6144; SpotBugs, PMD forbidden rules and Checkstyle 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 042878181b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review follow-up, taken but not in the shape suggested. The wait treated an interrupt as a successful wait: it restored the flag and returned, and init() went on to claim a generation while the previous one was still tearing down. That is the interleaving the wait exists to prevent, reached by a route nothing else checks. Re-arming the flag inside the loop and continuing -- the first remedy the review offered -- is worse than returning. Object.wait() clears the interrupted status when it throws, so a restored flag makes the very next wait() throw at once and the loop spins out the whole ten second deadline instead of sleeping through it. The interrupt is remembered instead, the serialization is held to the deadline as for any other caller, and the flag is restored once on the way out, so nothing is swallowed either. Aborting init() outright, the review's other suggestion, would leave the caller with no display at all -- a worse outcome than the overlap, and one no caller is written to expect. EdtHandoverTest covers it: an init() interrupted while parked must still be waiting afterwards, must come back once the teardown finishes, and must carry its interrupt out. Verified by restoring the old handling, which fails that test on its own assertion. core-unittests 6145/6145; SpotBugs, PMD forbidden rules and Checkstyle 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
1 participant
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.
Fixes the intermittent
build-testfailurewhich has failed a different
core-unittestsclass each time it appeared (ValidatorTest, AutoCompleteTextComponentTest) and never reproduced locally.The bug
A thread that has left
mainEDTLoop's loop staysisAlive()for the whole of its teardown --Desktop.disposeAll()andimpl.deinitialize(), either of which can block.Display.init()decided whether to start a dispatch thread on exactly that evidence, so aninit()landing in that window adopts a thread on its way out and starts nothing.The new generation then has no dispatch at all: everything it queues waits for ever, and
isInitialized()stays false whilecodenameOneRunningstays true -- a stateinit()cannot repair, because it guards on that flag. Every test in the class times out.The fix
The departing thread publishes the fact instead of leaving it to be inferred.
edtDispatchingis renounced inside the same monitorinit()takes to decide adoption, so the two orderings become the two correct outcomes rather than a race:init()starts a dispatch thread of its own;init()gets there first -- and it has already setcodenameOneRunningback to true, so the thread's next test keeps it in the loop, which is the adoption that is legitimately free.Two things that look like tidying are load-bearing, and both are commented in the code:
edtuntil the end. The teardown is meant to run as the EDT, so clearingedtearly to dodge adoption -- the obvious first fix -- makesisEdt()false for exactly that call.isEdt()then answered false on the dispatch thread itself.Why not in the harness
UITestBaseandFormTestInterceptoreach carry a recovery for this and both comments treat it as test infrastructure. They took ValidatorTest from twelve failures to one; neither can close the window. Their recoveries are left alone -- they also cover the unrelated half-initialised stateDisplayRecoveryTestdescribes.Verification
EdtHandoverTestholds the window open with an implementation that blocks insidedeinitialize()rather than racing for it. Both tests fail on master in 10.8s and pass in 1.9s, and each half of the fix is proven alone: removing only the identity guard fails the second test and leaves the first passing.Full
core-unittestsmodule 6142/6142; SpotBugs 0; PMD, copyright and control-character gates clean.🤖 Generated with Claude Code