Sending optional fields break certain cdp-commands

[david-detectify]

Hello,

The most recent commit (91afd10) to cdproto library broke some commands when communicating with Chrome. The reason for them being broken is the seemingly inconsequential change to the models that removed ,omitempty,omitzero from optional fields.

After debugging this locally I found that the Target.createTarget returns

{
    "code": -32000,
    "message": "Failed to open new tab - no browser is open"
}

when the optional field newWindow is set as false. Previously this field was only ever sent to chrome when set to true, but now it will be sent regardless.

It's strange that Chrome behaves this way, but I've been able to reproduce it locally by manually re-adding ,omitempty,omitzero to newWindow and then things started working again.

Snippets:

Before:

"[chrome comms] -> [{\"id\":1,\"method\":\"Target.createTarget\",\"params\":{\"url\":\"about:blank\"}}\n]"
"[chrome comms] <- [{\"id\":1,\"result\":{\"targetId\":\"5CCE5EFCF7F8D5113EEAAF94EB41FEE7\"}}]"

Now:

"[chrome comms] -> [{\"id\":1,\"method\":\"Target.createTarget\",\"params\":{\"url\":\"about:blank\",\"enableBeginFrameControl\":false,\"newWindow\":false,\"background\":false,\"forTab\":false}}\n]"
"[chrome comms] <- [{\"id\":1,\"error\":{\"code\":-32000,\"message\":\"Failed to open new tab - no browser is open\"}}]"

I have at the time of writing only found this to be a problem for Target.createTarget but wouldn't be surprised if there's more strange undefined behaviors in chrome that result in similar issues.

Problematic change:
91afd10#diff-4a863240ed8fc01708523a79f3c4ba046b61bd186bddc2d794d23e55bb2d17e1R309

Let me know if you need any additional information.

[kenshaw]

@david-detectify The issue here is that Go is "more explicit" than how the CDP works. This can be thought of as a failure of the chromedp package that has now been corrected. What's changed is that I am now explicitly setting the true/false boolean based on the CDP documentation on the type, and always sending the bool values. Unfortunately, the CDP is not consistent in how it treats "optional" boolean fields.

I'll look at this specific command, but I believe what you can do here in the interim is to set either the ForTab field to true (.WithForTab(true)).

[kenshaw]

@david-detectify This btw is not "strange" behavior; it's just a difference in how C++ (which Chrome/ium is written in), how Go works, and how JSON treats fields. Additionally, the CDP is very inconsistent in how it handles these things. Even though it's not documented, I'm reading the PDL now and the forTab field for this probably needs to be set to "true" by default, even though it's not properly documented in the PDL.

[david-detectify]

Yeah, "strange" was poor choice of words from my end. I was referring to the same thing you hinted at, CDP being inconsistent in how things are done internally.

Unfortunately I don't think there's an easy way for me to work around this issue because the CreateTarget command is triggered from within chromedp and not in "my" code. It's triggered here https://github.com/chromedp/chromedp/blob/master/chromedp.go#L375 as part of calling chromedp.Run() for the first time (using RemoteAllocator, causing c.first to be false as per https://github.com/chromedp/chromedp/blob/master/chromedp.go#L138)

[david-detectify]

Did some further testing and setting WithForTab(true)) when creating the initial browser isn't what is needed, but rather WithNewWindow(true).

This works:

"[chrome comms] -> [{\"id\":1,\"method\":\"Target.createTarget\",\"params\":{\"url\":\"about:blank\",\"enableBeginFrameControl\":false,\"newWindow\":true,\"background\":false,\"forTab\":false}}\n]"
"[chrome comms] <- [{\"id\":1,\"result\":{\"targetId\":\"8F289F77716E42D273299458A52A231A\"}}]"

This doesn't:

"[chrome comms] -> [{\"id\":1,\"method\":\"Target.createTarget\",\"params\":{\"url\":\"about:blank\",\"enableBeginFrameControl\":false,\"newWindow\":false,\"background\":false,\"forTab\":true}}\n]"
"[chrome comms] <- [{\"id\":1,\"error\":{\"code\":-32000,\"message\":\"Failed to open new tab - no browser is open\"}}]"

These tests are from me using a modified version of chromedp where https://github.com/chromedp/chromedp/blob/master/chromedp.go#L375 sets these values.

[kenshaw]

@david-detectify It would appear the various boolean fields for createTarget are mutually exclusive. It's been something like 5 years since I've touched that code, so I apologize. I've tagged and pushed v0.13.6 which adds the WithNewWindow by default on the CreateTarget call invoked by Run.

It's worth noting as I've been making changes, I have been testing chromedp against various scraping scenarios, and have not encountered issues -- could you share more about how/where you're encountering the problem? I assume many of the issues is how I use chromedp vs how others use it; which is I don't reuse browsers/sessions/etc. I'd like chromedp to work for all persons in all scenarios, but it's difficult to completely duplicate the context and way in which others may be using the package.

[david-detectify]

I completely understand the challenges with being able to test every scenario.

I'd like to share complete examples I have on my end but it's sadly multiple abstractions that makes sharing it tricky. Having said that I made a small snippet that can reproduce the issue:

package main

import (
	"context"
	"encoding/json"
	"fmt"
	"github.com/chromedp/cdproto/runtime"
	"github.com/chromedp/chromedp"
	"io"
	"net/http"
)

func main() {
	// Start chrome using
	// google-chrome --headless=new --no-first-run --remote-debugging-port=9223
	wsURL, err := getWSURL("http://localhost:9223/json/version")
	if err != nil {
		panic(err)
	}

	ctx := context.Background()
	allocContext, cancel := chromedp.NewRemoteAllocator(ctx, wsURL)
	defer cancel()

	chromeCtx, cancelCtx := chromedp.NewContext(allocContext)
	defer cancelCtx()

	err = chromedp.Run(chromeCtx, runtime.Enable())
	if err != nil {
               // chromedp before v0.13.6 panic here with the "no browser is open" error
		panic(err)
	}
	fmt.Println("all good")
}

func getWSURL(url string) (string, error) {
	resp, err := http.Get(url)
	if err != nil {
		return "", err
	}
	defer resp.Body.Close()
	switch resp.StatusCode {
	case http.StatusOK:
		jsonResp, err := parseJSONResult(resp.Body)
		if err != nil {
			return "", err
		}
		return jsonResp.WebSocketDebuggerURL, nil
	default:
		return "", fmt.Errorf("couldn't retrieve ws endpoint. status_code: %d", resp.StatusCode)
	}
}

type browserMetadata struct {
	WebSocketDebuggerURL string `json:"webSocketDebuggerUrl"`
}

func parseJSONResult(rdr io.Reader) (*browserMetadata, error) {
	data, err := io.ReadAll(rdr)
	if err != nil {
		return nil, err
	}
	var res *browserMetadata
	if err := json.Unmarshal(data, &res); err != nil {
		return nil, err
	}
	return res, nil
}

I also tested with v0.13.6 and so far it's looking good. Will run the entire test suite later during the day to see if there's any more issues that pop up.

Thanks for your assistance!

[david-detectify]

I've encountered one more issue so far, however it's located in my code so it's something I can fix.

More specifically, if you register request interception using (https://chromedevtools.github.io/devtools-protocol/tot/Fetch/#method-enable) like this:

fetch.Enable().
	WithHandleAuthRequests(true).
	WithPatterns(
		[]*fetch.RequestPattern{
			{
				URLPattern:   "",
				ResourceType: "",
				RequestStage: fetch.RequestStageRequest,
			},
			{
				URLPattern:   "",
				ResourceType: "",
				RequestStage: fetch.RequestStageResponse,
			},
		}),

and then continue the request using

fetch.ContinueRequest(ev.RequestID)

then no response event will be sent because unless you also, explicitly add WithInterceptResponse(true) like this

fetch.ContinueRequest(ev.RequestID).
	WithInterceptResponse(true)

According to the documentation, WithInterceptResponse is meant to be used as an override, but now it's a requirement that always need to match what you defined before. Previously no value would be sent and Chrome would send events based on the patterns defined before.

I suspect there's other cases like this, which are really hard to spot unless you know what you're looking for or if you don't have tests that spot this. Specially if you have code that worked before this change, and now silently fail. This makes me a bit hesitant to dare to deploy to production.

Looking at other projects such as Puppeteer (https://github.com/puppeteer/puppeteer/blob/main/packages/puppeteer-core/src/cdp/Browser.ts#L338), it seems like the common approach is to not be as explicit as chromedp is after this change. It seems like it prevents Chrome from being allowed to infer things.

What are you thoughts?

// David