core: fix protocol errors from late frame navigation #14716
Conversation
core/gather/driver/prepare.js
Outdated
| @@ -29,7 +31,13 @@ async function enableAsyncStacks(session) { | |||
| // See https://bugs.chromium.org/p/chromium/issues/detail?id=990945&q=setSkipAllPauses&can=2 | |||
| session.on('Page.frameNavigated', event => { | |||
| if (event.frame.parentId) return; | |||
| enable().catch(err => log.error('Driver', err)); | |||
| enable().catch(err => { | |||
There was a problem hiding this comment.
mentioned this in chat, but maybe it's better to move this to the instrumentation or sensitiveInstrumentation periods and purposefully disable it after it's done? Then the driver is known connected (unless there's a major error) and keeps this as-is
There was a problem hiding this comment.
#14717 is one way to do that, although I think you're suggesting doing this outside a gatherer right?
There was a problem hiding this comment.
Yes, looking at the balance between gatherers taking care of themselves and gatherers actually being independent units that don't interfere with each other, can be ordered however, etc. It's definitely helpful to have something as impactful as having the Debugger domain turned on out here where it can be turned on and off independent of the particular gatherer, and other future gatherers wouldn't have to coordinate (if we ever do style invalidation or whatever).
There was a problem hiding this comment.
I agree with your concerns, however I think they would be better solved by creating a separate session for the the async stack stuff. That would allow us to encapsulate the async stack stuff in the DevtoolsLog gatherer and keep it's handling of the debugger domain independent from other instrumentation.
|
The suggestion in #14716 (comment) deserved a better look if we're going to hold on #14717. A also moved the smoke test stuff from #14717 into this PR. |
There was a problem hiding this comment.
generally LGTM, maybe file an issue for the issues you ran into with moving to the DevtoolsLog gatherer and what you want to explore to resolve them? Also @paulirish's thinking on keeping up with protocol traffic as we navigate frames. Or is #14717 sufficient for tracking and we'll just try to keep it not too far back in the queue?
|
|
||
| await enable(); | ||
|
|
||
| return async () => { |
There was a problem hiding this comment.
to me this is calling out to be moved onto something with state (e.g. Driver) rather than requiring the caller to hold onto it. I realize it's less of a big deal outside of legacy since it can just be a local, but a general pattern of requiring the enabler to keep around the disabler when we're already keeping a bunch of state for them (in the event listeners, at the protocol enable/disable level, etc) seems excessive. I guess the refactor TODO below may make that irrelevant, though
There was a problem hiding this comment.
Part of the reason I designed it this way was to reduce code duplication between FR and legacy. I didn't want to create a new AsyncStackHandler() that's shared between the two implementations.
I think a combination of moving this logic to the DT log gatherer (which has state) and killing legacy runner once and for all will eliminate the need for this kind of pattern.
…hthouse into fix-target-close-errors
Fixes the protocol errors we see from basic usage: