fix: include console messages logged before the server attached#2383
fix: include console messages logged before the server attached#2383thomasbachem wants to merge 1 commit into
Conversation
OrKoN
left a comment
There was a problem hiding this comment.
Thanks for the PR. I think we should change Puppeteer's behavior here to emit buffered messages as soon as there is a subscriber to the event. I think we should not be fixing this in the MCP server. I think there should be no distinction between BufferedConsoleMessage and ConsoleMessage.
cc962f6 to
89558e4
Compare
|
Makes sense – with the replay already in Puppeteer's hands at connect, emitting it on first subscription is strictly better: Args stay live JSHandles, every consumer benefits, and the two ugliest parts of this PR (a first tool call racing the backfill session, and an open dialog stalling its Happy to send the Puppeteer PR. I'd leave this PR open until then – the end-to-end test scenario carries over. |
To these questions: I think also Issues need the same treatment. I would probably say let's emit buffered messages on the first subscriber. Let's probably clear buffers if the frame navigates. I think CDP has a cap on the backend so we should just accept whatever the backend sends. Let us think about this more. Alternatively: I wonder if perhaps emitting console messages (and others) on the BrowserContext is better? we can include the reference to the Page on the BrowserContext. It would simplify PageCollector and also allow us avoid buffering logic. |
|
cc @Lightning00Blade wdyt about emitting page events on browser context to cover the initial connection case? |
Fixes #2382.
ConsoleCollectornow opens a throwaway CDP session at attach time, harvests theRuntime.enableandLog.enablereplays (Chromium's buffer holds console messages, uncaught exceptions, and log entries), prepends the recovered messages to its storage, and detaches – which also releases the replayed args' remote objects.Console reads await the backfill, so a first tool call right after attach can't race it – relevant because the context is created lazily inside that first call. An open dialog pauses the renderer and the backfill with it, so reads then return the live-collected data instead of stalling, since
list_console_messagesmust work while a dialog is open.Instead of deduplicating overlapping messages, the backfill drops every replayed event stamped after the attach time – the live listeners own everything from that point on. That's the fork from the issue – happy to rework it into a dedup if you'd rather.
Replayed args can't be adopted as live JSHandles and Puppeteer's
ConsoleMessageconstructor is internal, so recovered messages become a newBufferedConsoleMessage(following theUncaughtErrorpattern). Only resolved arg values are lost – text, args count, and stack traces come through, andget_console_messagestill symbolizes the latter. If the page navigates while the backfill session connects, recovered messages land in the navigation bucket that was current at attach time.Not covered: Extension service worker consoles have the same pre-attach gap (
ServiceWorkerSubscriberenablesRuntimebefore its listeners attach) – left for a follow-up. And the pre/post-attach split compares event timestamps against the attach wall clock, so a message racing the attach boundary could in theory surface twice – consecutive grouping would absorb that.The end-to-end test logs a console message, an uncaught error, and a network error before the context exists and asserts all three surface exactly once and in order – it fails without the fix. Unit tests cover the conversion, the merge of both domains' replays, post-attach drops, and the navigation race.