fix: do not count JavaScript invocations in a closed UI - #25474
fix: do not count JavaScript invocations in a closed UI#25474totally-not-ai[bot] wants to merge 7 commits into
Conversation
Scheduling a JavaScript invocation counts it as undelivered in the UI its owner belongs to. A detached node keeps referencing the state tree of the UI it was attached to, so an owner that is reused after its UI has been closed still resolves to that UI, whose session has been cleared by then, and updating the count dereferenced it. Skip counting when the UI has no session, the same way a closed UI is already skipped when an invocation stops being counted. The invocation starts being counted once its owner is attached to a UI again.
An invocation scheduled for an owner that was detached while its UI was still open is counted in that UI, and closing the UI does not detach an already detached node, so nothing released the invocation from it. Reusing the owner in another UI then left the invocation uncounted there and still referencing the closed UI. Release an invocation counted in a UI without a session when its owner is attached again, so that it is counted in the UI it now belongs to.
| return; | ||
| } | ||
| if (countedIn != null) { | ||
| if (countedIn.getSession() != null) { |
There was a problem hiding this comment.
If stopCounting() is sensitive to the status of countedIn, then that should be fixed in stopCounting() so that all paths leading there will be protected.
There was a problem hiding this comment.
That branch is gone. Now that counting starts only when the owner is attached, countedIn can no longer end up pointing at a closed UI, so countWhenAttached() is back to its plain "already counted" check and no caller inspects the state of countedIn. The session check inside stopCounting() stays as the single place that knows a closed UI has no count left to update.
| // referencing a UI of its own only because of the count | ||
| return null; | ||
| } | ||
| if (internals.getSession() == null) { |
There was a problem hiding this comment.
Isn't this caused by calling invocationScheduled eagerly from the PendingJavaScriptInvocation constructor, rather than relying on it to be called only through runWhenAttached and executionRestored which both should happen only when the UI is attached? If that's the case, then we could remove the call from the constructor and instead only have an assert here to verify that the method is used correctly
There was a problem hiding this comment.
Done — the eager call is gone from the constructor. Counting now happens only through countWhenAttached(), which the framework calls when the owner is attached, so invocationScheduled just asserts that the owner is attached to an open UI (every node of a closed UI is detached, so a resolved UI always has a session).
Two things that surfaced while doing it:
Page.executeJsand the JS initializer registrations add their invocation straight toUIInternals.addJavaScriptInvocationwithout going throughrunWhenAttached, so they were counted only by the constructor.addJavaScriptInvocationnow counts as well; counting is idempotent, so an invocation that arrives there through its owner being attached is unaffected. Covered by a new test.- An invocation scheduled for an owner that is currently detached is no longer counted until the owner is attached again. That was the only detached case that was counted anyway — an owner detached after scheduling already stops counting — so the count now consistently means "waiting to be sent from an attached owner".
There was a problem hiding this comment.
Follow-up on the assert: the invariant turns out not to be guaranteed. UIInternals.setSession(null) catches an IllegalStateException from detaching the UI node and only logs "Error detaching closed UI", so a failed detach leaves nodes attached to a UI without a session. With assertions off that would have become an NPE out of executeJs, which is a bad trade for a counter that only backs a warning — addUndeliveredJsInvocations clamps rather than asserts for the same reason.
The assert now covers only the "owner belongs to a UI" part, and a UI without a session makes counting a no-op, so the invocation is counted in the UI it is attached to next. Added a test that closes a UI whose detach listener throws and then schedules an invocation for the owner left attached.
There was a problem hiding this comment.
One more turn on this: the assert is now gone entirely. Asserting that the owner belongs to a UI and then handling the case where it does not is a contradiction (static analysis flags the guard as always false), and the guard is the half that has to stay — besides the failed-detach case, the counting is now also reached from UIInternals.addJavaScriptInvocation, which is public and does not force its caller to have an attached owner. The contract is documented in the comment there instead, and both branches are covered by tests.
There was a problem hiding this comment.
One more turn on this: the assert is now gone entirely. Asserting that the owner belongs to a UI and then handling the case where it does not is a contradiction (static analysis flags the guard as always false), and the guard is the half that has to stay — besides the failed-detach case, the counting is now also reached from UIInternals.addJavaScriptInvocation, which is public and does not force its caller to have an attached owner. The contract is documented in the comment there instead, and both branches are covered by tests.
Counting an invocation as undelivered when it is created resolved the UI from the state tree that its owner keeps referencing while detached, which is what made the count reach a UI that has been closed, or one that the invocation is never delivered through. Count an invocation when the framework puts it on its way to a client instead: when its owner is attached, and when it is queued for a UI, which covers the invocations that are added to the queue directly, such as the ones from Page.executeJs. The owner is then always attached to an open UI, so the counting helper only asserts that.
Closing a UI logs a failure to detach its nodes and carries on, so an owner can be left attached to a UI that no longer has a session. Counting only backs a warning about undelivered invocations, which is not worth failing an application over, so skip it instead of dereferencing the session that a closed UI no longer has. The invocation is counted in the UI it is attached to next. Also replace a test that no longer covered anything after invocations stopped being counted when they are created.
…tion Asserting that the owner of a counted invocation belongs to a UI and then handling the case where it does not is a contradiction, and the guard is the part that has to stay: it also covers a UI left without a session by a failed detach, and the counting is reached from a public method that nothing forces to be called for an attached owner. Cover counting for an owner outside any UI with a test instead.
|
2 participants
What
PendingJavaScriptInvocationkeeps a count of invocations that are waiting to be sent, so that a UI can warn about invocations that are never delivered. That count was updated when an invocation was created, which resolved the UI through the owner's state node — and a detached node keeps referencing the state tree of the UI it was attached to. An owner reused after its UI had been closed therefore still resolved to that closed UI, whose session has already been cleared, and updating the count dereferenced it.This changes when an invocation is counted, and hardens the counting against a UI without a session:
Page.executeJs—UIInternals.addJavaScriptInvocationnow callscountWhenAttached(), which is idempotent, so an invocation already counted through its owner being attached is unaffected.invocationScheduledreturnsnullin that case rather than dereferencing the missing session. The invocation is counted in the UI it is attached to next.Net effect: an invocation for a detached owner, or for an owner whose UI has been closed, is simply not counted; it starts being counted once its owner is attached to an open UI again, which is the UI it will actually be delivered through.
Tests
PendingJavaScriptInvocationUtilTestis updated to schedule invocations the way the framework now does (create, thencountWhenAttached()), and gains coverage for the fixed scenarios:executeJsForDetachedOwner_countedWhenTheOwnerIsAttachedAgain— not counted while detached, counted on re-attach.executeJsForOwnerOfClosedUI_countedWhenAttachedToAnotherUI— an owner reused in another UI after its original UI was closed gets its invocation counted in the new UI.executeJsAfterFailedDetachOnClose_ownerAttachedWithoutSession_notCounted— a failed detach on close leaves the owner attached to a session-less UI; counting must not throw.pageExecuteJs_countedUntilSentToBrowser— an invocation queued directly for the UI is counted, and stops being counted once dumped to the browser.countInvocationForOwnerOutsideAnyUI_notCountedandcancelInvocationForOwnerOutsideAnyUI_notCountedWhenAttached— an owner outside any UI, and a canceled invocation whose owner is later attached, are never counted.A test that no longer covered anything once invocations stopped being counted at creation time was replaced.