Skip to content

Commit 877166f

Browse files
kblokclaude
andcommitted
fix: defer NotifyDestroyed to frameNavigated to correctly cancel pre-navigation contexts
With Chrome's RenderDocument feature, executionContextCreated may fire before frameNavigated. The previous approach of calling NotifyDestroyed in SetContext caused premature cancellation: for evaluations that return a value before navigation (e.g. window.location + return [42]), Chrome sends the callFunctionOn response before frameNavigated, but SetContext fires when executionContextCreated arrives (also before frameNavigated), cancelling the evaluation prematurely. The correct fix: defer NotifyDestroyed to Frame_FrameNavigated, which fires after Chrome sends callFunctionOn responses. Track the old context in SetContext via _contextToDestroyOnNavigation so the frameNavigated handler destroys the right (pre-navigation) context regardless of whether _context already points to the new context. For the pure RenderDocument case (no context events), fall back to destroying _context directly since it still holds the old context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 469c5f8 commit 877166f

1 file changed

Lines changed: 53 additions & 7 deletions

File tree

lib/PuppeteerSharp/IsolatedWorld.cs

Lines changed: 53 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ internal class IsolatedWorld : Realm, IDisposable, IAsyncDisposable
3333
private string _worldId;
3434
private string _origin;
3535

36+
// Tracks the old context to destroy when Page.frameNavigated fires.
37+
// Chrome sends callFunctionOn responses before frameNavigated, so deferring
38+
// NotifyDestroyed to frameNavigated ensures valid return values are captured first.
39+
private ExecutionContext _contextToDestroyOnNavigation;
40+
private bool _hasPendingContextDestruction;
41+
3642
public IsolatedWorld(
3743
Frame frame,
3844
WebWorker worker,
@@ -47,6 +53,11 @@ public IsolatedWorld(
4753

4854
_detached = false;
4955
FrameUpdated();
56+
57+
if (Frame != null)
58+
{
59+
Frame.FrameNavigated += Frame_FrameNavigated;
60+
}
5061
}
5162

5263
/// <inheritdoc/>
@@ -211,6 +222,12 @@ internal void Detach()
211222
{
212223
_detached = true;
213224
Client.MessageReceived -= Client_MessageReceived;
225+
226+
if (Frame != null)
227+
{
228+
Frame.FrameNavigated -= Frame_FrameNavigated;
229+
}
230+
214231
TaskManager.TerminateAll(new PuppeteerException("waitForFunction failed: frame got detached."));
215232
}
216233

@@ -268,6 +285,8 @@ internal void ClearContext()
268285
_contextResolveTaskWrapper = new TaskCompletionSource<ExecutionContext>(TaskCreationOptions.RunContinuationsAsynchronously);
269286
_context?.Dispose();
270287
_context = null;
288+
_hasPendingContextDestruction = false;
289+
_contextToDestroyOnNavigation = null;
271290
Frame?.ClearDocumentHandle();
272291
}
273292

@@ -286,20 +305,47 @@ internal void SetContext(ExecutionContext context)
286305
throw new ArgumentNullException(nameof(context));
287306
}
288307

289-
// Cancel any pending evaluations on the old context before replacing it.
290-
// With Chrome's RenderDocument feature, the new context may arrive without a
291-
// preceding executionContextDestroyed event, so we cannot rely on ClearContext
292-
// to signal cancellation. NotifyDestroyed is idempotent, so it's safe to call
293-
// even when ClearContext already disposed the old context.
294-
var oldContext = _context;
308+
// Save the old context so Frame_FrameNavigated can signal its destruction.
309+
// We defer NotifyDestroyed to frameNavigated rather than calling it here because
310+
// Chrome sends callFunctionOn responses before frameNavigated — deferring ensures
311+
// valid return values are captured before we signal cancellation.
312+
_contextToDestroyOnNavigation = _context;
313+
_hasPendingContextDestruction = true;
295314
_context = context;
296-
oldContext?.NotifyDestroyed();
297315

298316
_contextBindings.Clear();
299317
_contextResolveTaskWrapper.TrySetResult(context);
300318
TaskManager.RerunAll();
301319
}
302320

321+
private void Frame_FrameNavigated(object sender, FrameNavigatedEventArgs e)
322+
{
323+
var hasPending = _hasPendingContextDestruction;
324+
var ctxToDestroy = _contextToDestroyOnNavigation;
325+
_hasPendingContextDestruction = false;
326+
_contextToDestroyOnNavigation = null;
327+
328+
// Don't cancel for within-document navigations or BFCache restores.
329+
// For BFCache, Chrome sends executionContextCreated with the cached context BEFORE
330+
// frameNavigated, so _context is already the restored context — we must not destroy it.
331+
if (e.NavigatedWithinDocument || e.Type == NavigationType.BackForwardCacheRestore)
332+
{
333+
return;
334+
}
335+
336+
if (hasPending)
337+
{
338+
// executionContextCreated fired before frameNavigated (normal or RenderDocument with new ctx):
339+
// ctxToDestroy is the old context that had the pending evaluation.
340+
ctxToDestroy?.NotifyDestroyed();
341+
}
342+
else
343+
{
344+
// RenderDocument with no context events: _context is still the old context.
345+
_context?.NotifyDestroyed();
346+
}
347+
}
348+
303349
private async void Client_MessageReceived(object sender, MessageEventArgs e)
304350
{
305351
try

0 commit comments

Comments
 (0)