Fix stale lead image / wrong-refresh after a superseded page load T395597#6678
Fix stale lead image / wrong-refresh after a superseded page load T395597#6678motiz88 wants to merge 1 commit into
Conversation
…95597) wikimedia#6659 cancels the previous page-load Job, but Kotlin cancellation is cooperative and only takes effect at a suspension point. When a superseded load's requests were served from cache -- e.g. the article that was open before the user followed an external link to a new one -- its coroutine resumes past the already-complete await()s without ever observing the cancellation and runs createPageModel() anyway. Because createPageModel() builds the page as Page(title = model.title, pageProperties = thisResponse), the stale response's lead image is fused with the current article's title: the new article's text ends up under the previous article's lead image, and pull-to-refresh reloads the old article (it reads the reverted model.title). Guard the commit so a superseded load is a no-op: after the awaits, bail out if the job is no longer active or model.title is no longer the title this load captured. Also clear the lead image at the start of loadPage() so a previous article's image can't linger under the new article while its metadata loads. Add PageLoadSupersessionTest, which reproduces the interleaving deterministically (via a manually-pumped dispatcher, no new test dependencies): without the guard the model ends torn, with it the current article stays intact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
motiz88
changed the title
|
|
||
| /** | ||
| * Regression test for T395597 ("text of a newly-opened article rendered under the lead | ||
| * image of a previously-open article; pull-to-refresh reloads the wrong article"). |
There was a problem hiding this comment.
This test is a very inadequate representation of the actual logic in PageFragmentLoadState.
Retrofit wraps all calls in a suspendCancellableCoroutine block, which will respond to cancellation whether it was served from cache or from the real network.
And, crucially, this test does not .cancel() the previous Job before launching the new Job, which is exactly the fix originally made in #6659.
If summaryA and summaryB were both cancellable blocks instead of a plain CompletableDeferred, the existing logic would work as expected, without any changes needed.
| // A newer page load may have superseded this one while we were awaiting the | ||
| // network. Cancellation is cooperative, so a load whose requests were served | ||
| // from cache (e.g. the article that was already open before a new one was | ||
| // launched via an external link) can resume past these await()s without ever |
There was a problem hiding this comment.
I'm afraid this doesn't make sense. Retrofit enqueues its calls inside a suspendCancellableCoroutine block, which means it will respond to cancellation regardless of whether it returned the content from cache or from the real network. If there was a chance that a Retrofit call ignores cancellation, then we would need to check for isActive after every network call, which would be extremely inconvenient.
|
Yeah fair enough - I'm happy to try to pin down repro steps if it helps. (Also just realised that the last Play Store publish was in May so #6659 isn't included in it.) |
|
With cautious optimism, all evidence suggests that the issue is resolved. |
2 participants
Claude-authored attempt to fix T395597 building on #6659. Claude's description follows.
#6659 cancels the previous page-load Job, but Kotlin cancellation is cooperative and only takes effect at a suspension point. When a superseded load's requests were served from cache -- e.g. the article that was open before the user followed an external link to a new one -- its coroutine resumes past the already-complete await()s without ever observing the cancellation and runs createPageModel() anyway. Because createPageModel() builds the page as Page(title = model.title, pageProperties = thisResponse), the stale response's lead image is fused with the current article's title: the new article's text ends up under the previous article's lead image, and pull-to-refresh reloads the old article (it reads the reverted model.title).
Guard the commit so a superseded load is a no-op: after the awaits, bail out if the job is no longer active or model.title is no longer the title this load captured. Also clear the lead image at the start of loadPage() so a previous article's image can't linger under the new article while its metadata loads.
Add PageLoadSupersessionTest, which reproduces the interleaving deterministically (via a manually-pumped dispatcher, no new test dependencies): without the guard the model ends torn, with it the current article stays intact.
What does this do?
Why is this needed?
Phabricator:
T395597