-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[fix]: dont attach to targets if already attached #1346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 688f44f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile OverviewGreptile SummaryFixed intermittent "main world not ready for frame" errors by preventing duplicate CDP session creation. Added check in Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Chrome
participant bootstrap
participant conn as CdpConnection
participant session as CDP Session
Note over bootstrap: bootstrap() called
bootstrap->>conn: enableAutoAttach()
Note over Chrome,conn: Auto-attach occurs for some targets
Chrome->>conn: Target.attachedToTarget (auto)
conn->>session: Create session
session->>session: Runtime.executionContextCreated
bootstrap->>conn: getTargets()
conn-->>bootstrap: List of targets (some with attached=true)
loop For each target
alt Target NOT already attached
bootstrap->>conn: attachToTarget(targetId)
conn->>session: Create new session
Note over session: This session never sees<br/>Runtime events!
else Target already attached (NEW)
bootstrap->>bootstrap: Skip (continue)
Note over bootstrap: Prevents duplicate session
end
end
Note over session: OOPIF tries to resolve context
session->>session: waitForMainWorld(frameId)
alt With duplicate session (OLD)
session-->>session: Timeout: "main world not ready"
else Single session (NEW)
session-->>session: Success: context found
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
why
Target.attachToTargetproduces a new flattened session ID, so we ended up with two independent root CDP sessions for the same page.Runtime.executionContextCreated)executionContexts.waitForMainWorldto time out even though the context already existedwhat changed
bootstrap()function (insidepackages/core/lib/v3/understudy/context.ts), after grabbing existing targets viaTarget.getTargets, we now avoid directly callingTarget.attachToTargetfor any entry whose attached flag is already truetest plan
shadow-iframe.spec.ts)Summary by cubic
Skip attaching to targets that are already attached to avoid duplicate attach calls when auto-attach has already run. This prevents redundant work and suppresses re-attach errors.
Written for commit 688f44f. Summary will update automatically on new commits.