fix(cdp): unblock puppeteer connect, bound Runtime.evaluate, full TargetInfo (#181)
Three independent gaps that hurt browser-based MCP clients connecting
to obscura's CDP endpoint:
- Runtime.enable returned "No page" when called before a session was
attached. puppeteer-extra's FrameManager.initialize fires it on the
browser-level WebSocket BEFORE any page target exists, so the
standard puppeteer.connect / browser.newPage flow died on connect.
Now reply with {} when no session is attached and let the next
Target.attachToTarget emit executionContextCreated.
- Runtime.evaluate had no per-call timeout. A long synchronous JS
expression held the global V8 lock indefinitely and starved every
other CDP command on the session. Honor the CDP `timeout` parameter
and default to 30s (Chrome's protocolTimeout). Returns a clear error
on expiry so client-side deadlines fire cleanly instead of hanging.
- Target.getTargetInfo for the browser target omitted canAccessOpener.
Strict CDP clients (chromiumoxide) panic when the field is missing.
Fill it in (false for the browser target itself).
Partial fix for #122. The remaining items (networkIdle hard-deadline
on SPAs, /json/list hardcoded data, docs on cheap-vs-queued methods)
need bigger or non-code changes and are deferred.
This commit is contained in:
1 parent
71b9b4fbab
commit
f4f49ebab4
2 files changed
+92
-20
No files matched your search
@@ -11,25 +11,37 @@ pub async fn handle(
|
|||||||
) -> Result<Value, String> {
|
) -> Result<Value, String> {
|
||||||
match method {
|
match method {
|
||||||
"enable" => {
|
"enable" => {
|
||||||
let page = ctx.get_session_page(session_id).ok_or("No page")?;
|
// puppeteer-extra's FrameManager.initialize calls Runtime.enable on
|
||||||
let event = crate::types::CdpEvent {
|
// the browser-level connection BEFORE any page target exists. Real
|
||||||
method: "Runtime.executionContextCreated".to_string(),
|
// Chrome replies with `{}` and emits executionContextCreated when
|
||||||
params: json!({
|
// a context appears. Returning "No page" here breaks the standard
|
||||||
"context": {
|
// puppeteer connect/newPage flow. If there's no session, succeed
|
||||||
"id": 1,
|
// silently — the next Target.attachToTarget will set things up.
|
||||||
"origin": page.url_string(),
|
match ctx.get_session_page(session_id) {
|
||||||
"name": "",
|
Some(page) => {
|
||||||
"uniqueId": format!("ctx-{}", page.id),
|
let event = crate::types::CdpEvent {
|
||||||
"auxData": {
|
method: "Runtime.executionContextCreated".to_string(),
|
||||||
"isDefault": true,
|
params: json!({
|
||||||
"type": "default",
|
"context": {
|
||||||
"frameId": page.frame_id,
|
"id": 1,
|
||||||
}
|
"origin": page.url_string(),
|
||||||
}
|
"name": "",
|
||||||
}),
|
"uniqueId": format!("ctx-{}", page.id),
|
||||||
session_id: session_id.clone(),
|
"auxData": {
|
||||||
};
|
"isDefault": true,
|
||||||
ctx.pending_events.push(event);
|
"type": "default",
|
||||||
|
"frameId": page.frame_id,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}),
|
||||||
|
session_id: session_id.clone(),
|
||||||
|
};
|
||||||
|
ctx.pending_events.push(event);
|
||||||
|
}
|
||||||
|
None => {
|
||||||
|
// No session attached yet — that's fine. Just ack.
|
||||||
|
}
|
||||||
|
}
|
||||||
Ok(json!({}))
|
Ok(json!({}))
|
||||||
}
|
}
|
||||||
"evaluate" => {
|
"evaluate" => {
|
||||||
@@ -49,10 +61,31 @@ pub async fn handle(
|
|||||||
.and_then(|v| v.as_bool())
|
.and_then(|v| v.as_bool())
|
||||||
.unwrap_or(false);
|
.unwrap_or(false);
|
||||||
|
|
||||||
|
// CDP `timeout` field (milliseconds). Default to Chrome's
|
||||||
|
// protocolTimeout (30s) so long evaluations don't pin the V8 lock
|
||||||
|
// indefinitely and starve every other CDP command on the same
|
||||||
|
// session.
|
||||||
|
let timeout_ms = params
|
||||||
|
.get("timeout")
|
||||||
|
.and_then(|v| v.as_u64())
|
||||||
|
.unwrap_or(30_000);
|
||||||
|
|
||||||
let page = ctx
|
let page = ctx
|
||||||
.get_session_page_mut(session_id)
|
.get_session_page_mut(session_id)
|
||||||
.ok_or("No page")?;
|
.ok_or("No page")?;
|
||||||
let info = page.evaluate_for_cdp(expression, return_by_value, await_promise).await;
|
let info = match tokio::time::timeout(
|
||||||
|
std::time::Duration::from_millis(timeout_ms),
|
||||||
|
page.evaluate_for_cdp(expression, return_by_value, await_promise),
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
Ok(info) => info,
|
||||||
|
Err(_) => {
|
||||||
|
return Err(format!(
|
||||||
|
"Runtime.evaluate exceeded {timeout_ms}ms timeout"
|
||||||
|
));
|
||||||
|
}
|
||||||
|
};
|
||||||
page.process_pending_navigation().await.map_err(|e| e.to_string())?;
|
page.process_pending_navigation().await.map_err(|e| e.to_string())?;
|
||||||
|
|
||||||
Ok(json!({ "result": remote_object_from_info(&info) }))
|
Ok(json!({ "result": remote_object_from_info(&info) }))
|
||||||
@@ -358,4 +391,18 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Regression for #122 item 7: puppeteer-extra's FrameManager.initialize
|
||||||
|
/// fires Runtime.enable on the browser-level WebSocket BEFORE any page
|
||||||
|
/// target exists. Real Chrome replies with `{}`; before the fix Obscura
|
||||||
|
/// returned `{"error":{"code":-32601,"message":"No page"}}` and the
|
||||||
|
/// puppeteer connect flow died.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn enable_succeeds_when_no_session_attached() {
|
||||||
|
let mut ctx = CdpContext::new();
|
||||||
|
let result = handle("enable", &json!({}), &mut ctx, &None)
|
||||||
|
.await
|
||||||
|
.expect("Runtime.enable must succeed even with no session");
|
||||||
|
assert_eq!(result, json!({}));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -224,6 +224,9 @@ pub async fn handle(method: &str, params: &Value, ctx: &mut CdpContext) -> Resul
|
|||||||
}))
|
}))
|
||||||
}
|
}
|
||||||
None => {
|
None => {
|
||||||
|
// canAccessOpener is required on every TargetInfo per the
|
||||||
|
// CDP spec. Strict clients (chromiumoxide) panic if it's
|
||||||
|
// missing. The browser target itself has no opener.
|
||||||
Ok(json!({
|
Ok(json!({
|
||||||
"targetInfo": {
|
"targetInfo": {
|
||||||
"targetId": "browser",
|
"targetId": "browser",
|
||||||
@@ -231,6 +234,7 @@ pub async fn handle(method: &str, params: &Value, ctx: &mut CdpContext) -> Resul
|
|||||||
"title": "",
|
"title": "",
|
||||||
"url": "",
|
"url": "",
|
||||||
"attached": true,
|
"attached": true,
|
||||||
|
"canAccessOpener": false,
|
||||||
}
|
}
|
||||||
}))
|
}))
|
||||||
}
|
}
|
||||||
@@ -277,4 +281,25 @@ mod tests {
|
|||||||
.expect_err("unknown methods must surface as errors");
|
.expect_err("unknown methods must surface as errors");
|
||||||
assert!(err.contains("Unknown Target method"));
|
assert!(err.contains("Unknown Target method"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Regression for #122 item 5: every TargetInfo payload must carry the
|
||||||
|
/// `canAccessOpener` field. The browser-target branch of getTargetInfo
|
||||||
|
/// (no targetId passed → no page) used to omit it; strict CDP clients
|
||||||
|
/// like chromiumoxide panic when the field is missing.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn get_target_info_browser_target_includes_can_access_opener() {
|
||||||
|
let mut ctx = CdpContext::new();
|
||||||
|
// No targetId → falls through to the browser-target branch.
|
||||||
|
let result = handle("getTargetInfo", &json!({}), &mut ctx)
|
||||||
|
.await
|
||||||
|
.expect("getTargetInfo with no targetId must return browser info");
|
||||||
|
|
||||||
|
let info = &result["targetInfo"];
|
||||||
|
assert_eq!(info["type"], "browser", "must be the browser target");
|
||||||
|
assert!(
|
||||||
|
info.get("canAccessOpener").is_some(),
|
||||||
|
"canAccessOpener must be present on every TargetInfo, got: {result}"
|
||||||
|
);
|
||||||
|
assert_eq!(info["canAccessOpener"], false);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
Reference in new issue
Block a user