[fix]: ctx.addInitScript() iframe issues#1664
Draft
seanmcguire12 wants to merge 4 commits intomainfrom
Draft
Conversation
🦋 Changeset detectedLatest commit: 7a9accb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
Member
Author
Contributor
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 6 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant B as Browser / CDP
participant C as V3Context
participant S as CDPSession (Page/OOPIF)
participant P as Piercer / ExecutionContext
Note over B,S: Target Attachment & Init Script Flow
B-->>C: Target.attachedToTarget (Target Paused)
C->>C: NEW: installTargetSessionListeners()
rect rgb(23, 37, 84)
Note right of C: Session Initialization (onAttachedToTarget)
C->>S: Page.enable()
C->>S: Runtime.enable()
C->>S: NEW: Target.setAutoAttach(waitForDebuggerOnStart: true, flatten: true)
Note right of S: Ensures nested OOPIFs also pause on start
opt Has Init Scripts
C->>S: CHANGED: Page.addScriptToEvaluateOnNewDocument(source, runImmediately: true)
end
C->>S: NEW: Runtime.runIfWaitingForDebugger()
Note right of S: Target resumes ONLY after scripts are registered
end
Note over C,P: Piercer Installation Flow
C->>S: NEW: Page.getFrameTree()
S-->>C: frameTree (All nested frames)
loop Each Frame in Tree
C->>P: NEW: waitForMainWorld(frameId)
P-->>C: executionContextId
C->>S: Runtime.evaluate(v3ScriptContent, contextId)
C->>S: Runtime.evaluate(reRenderScriptContent, contextId)
end
B-->>C: NEW: Page.frameNavigated (Event)
C->>P: Re-install piercer in new main world context
alt Unhappy Path: Session Closed
S-->>C: Error: Session with given id not found
C->>C: Cleanup session listeners & tracking
end
Contributor
Greptile OverviewGreptile SummaryFixed Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Context as V3Context
participant CDP as CDP Connection
participant Browser as Browser/Target
participant Session as CDP Session
participant Frame as OOPIF/SPIF
Note over Context,Browser: Target Attachment Flow (Fixed)
Context->>CDP: enableAutoAttach(waitForDebuggerOnStart: true)
Browser->>Context: Target.attachedToTarget (paused)
Context->>Session: installTargetSessionListeners()
Note over Session: Attach nested target listeners
Context->>Session: Page.enable
Context->>Session: Runtime.enable
Context->>Session: Target.setAutoAttach(waitForDebuggerOnStart: true, flatten: true)
Note over Context,Session: Propagate auto-attach to nested sessions
loop For each init script
Context->>Session: Page.addScriptToEvaluateOnNewDocument(runImmediately: true)
end
Context->>Session: Runtime.runIfWaitingForDebugger
Note over Session,Browser: Resume AFTER init scripts registered
Browser->>Frame: Frame created (OOPIF/SPIF)
Frame->>Session: Target.attachedToTarget (nested, paused)
Session->>Frame: Install init scripts (propagated)
Session->>Frame: Runtime.runIfWaitingForDebugger
Note over Frame: Init scripts execute before page load
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+46
to
+50
| session.on<Protocol.Page.FrameNavigatedEvent>( | ||
| "Page.frameNavigated", | ||
| (evt) => { | ||
| void installInFrame(evt.frame.id); | ||
| }, |
Contributor
There was a problem hiding this comment.
listener not removed on session close
The Page.frameNavigated listener is added but never removed. In a long-lived session with many navigations, this could lead to memory issues if the same session is reused.
Consider adding cleanup logic to remove the listener when the session closes or when installV3PiercerIntoSession completes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/lib/v3/understudy/piercer.ts
Line: 46:50
Comment:
listener not removed on session close
The `Page.frameNavigated` listener is added but never removed. In a long-lived session with many navigations, this could lead to memory issues if the same session is reused.
Consider adding cleanup logic to remove the listener when the session closes or when `installV3PiercerIntoSession` completes.
How can I resolve this? If you propose a fix, please make it concise.| ); | ||
| } | ||
| } | ||
| installPromises.push(resume()); |
Contributor
There was a problem hiding this comment.
resume() added to promise array without await
The resume() call is added to installPromises but this creates a race condition. If resume() executes before init scripts are registered, the scripts may miss the document load event.
The fix should ensure resume() only executes after all init script registrations have been sent to CDP. Consider wrapping the init script loop in a Promise.all() and chaining resume() after.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/lib/v3/understudy/context.ts
Line: 489:489
Comment:
`resume()` added to promise array without `await`
The `resume()` call is added to `installPromises` but this creates a race condition. If `resume()` executes before init scripts are registered, the scripts may miss the document load event.
The fix should ensure `resume()` only executes after all init script registrations have been sent to CDP. Consider wrapping the init script loop in a `Promise.all()` and chaining `resume()` after.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
why
context.addInitScript()were not being applied in the following scenarios:what changed
runImmediately: truetoPage.addScriptToEvaluateOnNewDocumentcalls to so that scripts execute before any other code runsTarget.setAutoAttachwithwaitForDebuggerOnStart: trueto nested sessions, ensuring OOPIFs are paused before they can executeRuntime.runIfWaitingForDebugger) to happen after init scripts are registered to prevent init scripts being missedTarget.attachedToTargetlisteners to properly handle nested iframe attachment chainsTarget.targetCreatedfallback attach logic, relying instead on the properly configured auto-attach cascadetest plan
addInitScriptiframe cases #1663Summary by cubic
Fixes context.addInitScript so init scripts run in OOPIFs and in popup pages with SPIF iframes. Scripts now execute before any page code and reliably propagate across nested targets.
Written for commit 7a9accb. Summary will update on new commits. Review in cubic