Skip to content

Commit da8d752

Browse files
myk1ytmyk1yt
committed
fix(webview): harden recovery reload and scope sidebar disposal cleanup
What: The recovery reload captures lastWebviewHeartbeatAt when recovery starts and, after the awaited HTML generation, additionally requires the heartbeat timestamp to be unchanged and the captured view to still be visible before reassigning webview.html. The sidebar onDidDispose handler now runs clearWebviewResources() and the codeIndexManager reset only when the disposed view is the currently watched one. Cancellation specs defer getWebviewHtml() itself (instead of getState() with a fabricated state) so they prove disposal and replacement land while recovery is in flight, and new specs cover heartbeat-during-recovery and hide-during-recovery skips. Why: A heartbeat arriving during recovery means the renderer is alive again, and a view that hides mid-recovery no longer proves a dead renderer (hidden webviews throttle timers), so reloading in either case needlessly reloads a healthy or throttled webview. This mitigates the security audit finding LOW-1 (self-reload loop when the postMessage bridge is half-broken): a renderer that resumes heartbeating during the recovery window is no longer reloaded on every watchdog tick. Disposing an outdated (replaced) sidebar view previously wiped the replacement view's resources and code index manager; scoping the cleanup to the watched view keeps the replacement intact.
1 parent 1e6fd39 commit da8d752

2 files changed

Lines changed: 90 additions & 16 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1146,15 +1146,15 @@ export class ClineProvider
11461146
this.log("Disposing ClineProvider instance for tab view")
11471147
await this.dispose()
11481148
} else {
1149-
this.log("Clearing webview resources for sidebar view")
1150-
this.clearWebviewResources()
11511149
if (this.view === webviewView) {
1150+
this.log("Clearing webview resources for sidebar view")
1151+
this.clearWebviewResources()
11521152
// Drop the disposed view so nothing keeps polling it
11531153
// (e.g. the recovery watchdog) for the provider's lifetime.
11541154
this.view = undefined
1155+
// Reset current workspace manager reference when view is disposed
1156+
this.codeIndexManager = undefined
11551157
}
1156-
// Reset current workspace manager reference when view is disposed
1157-
this.codeIndexManager = undefined
11581158
}
11591159
},
11601160
null,
@@ -3452,12 +3452,23 @@ export class ClineProvider
34523452
// Capture the epoch so a disposal or view replacement can invalidate
34533453
// this operation while the HTML is being generated.
34543454
const epoch = this.webviewRecoveryEpoch
3455+
// A heartbeat that arrives while the HTML is being generated means the
3456+
// renderer is alive again; comparing against the timestamp captured here
3457+
// lets the post-await check skip the reload in that case.
3458+
const heartbeatAtRecoveryStart = this.lastWebviewHeartbeatAt
34553459
try {
34563460
const html = await this.getWebviewHtml(view.webview)
3457-
// The await yields; assigning html now that the provider is disposed
3458-
// or the watched view was disposed/replaced would touch a dead or
3459-
// unrelated webview.
3460-
if (this._disposed || this.webviewRecoveryEpoch !== epoch || this.view !== view) {
3461+
// The await yields; assigning html now that the provider is disposed,
3462+
// the watched view was disposed/replaced, the renderer heartbeat
3463+
// recovered, or the view hid again would either touch a dead or
3464+
// unrelated webview or reload one that no longer needs recovery.
3465+
if (
3466+
this._disposed ||
3467+
this.webviewRecoveryEpoch !== epoch ||
3468+
this.view !== view ||
3469+
this.lastWebviewHeartbeatAt !== heartbeatAtRecoveryStart ||
3470+
view.visible !== true
3471+
) {
34613472
return
34623473
}
34633474
view.webview.html = html

‎src/core/webview/__tests__/ClineProvider.spec.ts‎

Lines changed: 71 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -837,19 +837,24 @@ describe("ClineProvider", () => {
837837
await provider.resolveWebviewView(mockWebviewView)
838838
const htmlAfterResolve = mockWebviewView.webview.html
839839

840-
let finishReload: (state: ExtensionState) => void = () => {}
841-
vi.spyOn(provider, "getState").mockImplementation(
840+
// Defer the recovery reload's own HTML generation so the test proves
841+
// the disposal lands while recovery is in flight, not before it starts.
842+
let finishReload: (html: string) => void = () => {}
843+
provider["getWebviewHtml"] = vi.fn().mockImplementation(
842844
() =>
843-
new Promise<ExtensionState>((resolve) => {
845+
new Promise<string>((resolve) => {
844846
finishReload = resolve
845847
}),
846848
)
847849

848850
await vi.advanceTimersByTimeAsync(120_000)
851+
// The stale heartbeat started a recovery reload, and its HTML
852+
// generation is still pending.
853+
expect(provider["getWebviewHtml"]).toHaveBeenCalledTimes(1)
849854
expect(mockWebviewView.webview.html).toBe(htmlAfterResolve)
850855

851856
disposeCallback()
852-
finishReload({ apiConfiguration: {} } as unknown as ExtensionState)
857+
finishReload("<!DOCTYPE html><html><body>recovered</body></html>")
853858
await vi.advanceTimersByTimeAsync(0)
854859

855860
expect(mockWebviewView.webview.html).toBe(htmlAfterResolve)
@@ -859,15 +864,21 @@ describe("ClineProvider", () => {
859864
await provider.resolveWebviewView(mockWebviewView)
860865
const htmlAfterResolve = mockWebviewView.webview.html
861866

862-
let finishReload: (state: ExtensionState) => void = () => {}
863-
vi.spyOn(provider, "getState").mockImplementation(
867+
// Defer the recovery reload's own HTML generation so the test proves
868+
// the replacement lands while recovery is in flight, not before it
869+
// starts.
870+
let finishReload: (html: string) => void = () => {}
871+
provider["getWebviewHtml"] = vi.fn().mockImplementation(
864872
() =>
865-
new Promise<ExtensionState>((resolve) => {
873+
new Promise<string>((resolve) => {
866874
finishReload = resolve
867875
}),
868876
)
869877

870878
await vi.advanceTimersByTimeAsync(120_000)
879+
// The stale heartbeat started a recovery reload, and its HTML
880+
// generation is still pending.
881+
expect(provider["getWebviewHtml"]).toHaveBeenCalledTimes(1)
871882
expect(mockWebviewView.webview.html).toBe(htmlAfterResolve)
872883

873884
// VS Code re-resolves a fresh view (e.g. sidebar re-opened) while the
@@ -885,7 +896,59 @@ describe("ClineProvider", () => {
885896
visible: true,
886897
}
887898

888-
finishReload({ apiConfiguration: {} } as unknown as ExtensionState)
899+
finishReload("<!DOCTYPE html><html><body>recovered</body></html>")
900+
await vi.advanceTimersByTimeAsync(0)
901+
902+
expect(mockWebviewView.webview.html).toBe(htmlAfterResolve)
903+
})
904+
905+
test("skips the recovery reload when a heartbeat arrives while regenerating HTML", async () => {
906+
await provider.resolveWebviewView(mockWebviewView)
907+
const htmlAfterResolve = mockWebviewView.webview.html
908+
909+
let finishReload: (html: string) => void = () => {}
910+
provider["getWebviewHtml"] = vi.fn().mockImplementation(
911+
() =>
912+
new Promise<string>((resolve) => {
913+
finishReload = resolve
914+
}),
915+
)
916+
917+
await vi.advanceTimersByTimeAsync(120_000)
918+
expect(provider["getWebviewHtml"]).toHaveBeenCalledTimes(1)
919+
expect(mockWebviewView.webview.html).toBe(htmlAfterResolve)
920+
921+
// The renderer process reported in again while the recovery HTML was
922+
// still being generated, so the webview is alive and must not reload.
923+
provider.updateWebviewHeartbeat()
924+
925+
finishReload("<!DOCTYPE html><html><body>recovered</body></html>")
926+
await vi.advanceTimersByTimeAsync(0)
927+
928+
expect(mockWebviewView.webview.html).toBe(htmlAfterResolve)
929+
})
930+
931+
test("skips the recovery reload when the view hides while regenerating HTML", async () => {
932+
await provider.resolveWebviewView(mockWebviewView)
933+
const htmlAfterResolve = mockWebviewView.webview.html
934+
935+
let finishReload: (html: string) => void = () => {}
936+
provider["getWebviewHtml"] = vi.fn().mockImplementation(
937+
() =>
938+
new Promise<string>((resolve) => {
939+
finishReload = resolve
940+
}),
941+
)
942+
943+
await vi.advanceTimersByTimeAsync(120_000)
944+
expect(provider["getWebviewHtml"]).toHaveBeenCalledTimes(1)
945+
946+
// The view hid while the recovery HTML was still being generated; a
947+
// hidden webview throttles heartbeats, so the stale heartbeat no
948+
// longer proves a dead renderer.
949+
Object.defineProperty(mockWebviewView, "visible", { value: false, configurable: true })
950+
951+
finishReload("<!DOCTYPE html><html><body>recovered</body></html>")
889952
await vi.advanceTimersByTimeAsync(0)
890953

891954
expect(mockWebviewView.webview.html).toBe(htmlAfterResolve)

0 commit comments

Comments
 (0)