feat: allow tab to stay opened after context cancel

[mariusziemke]

I have a use case in which it is important to not close the tab / browser after the context gets cancelled.

Therefore I introduced a new ContextOption. Feel free to discuss :-)

[ZekeLu]

Do we really need the change? Will no calling the cancel() function keep the browser/tab open?

[mariusziemke]

I had issues with memory and cpu, because as far as I understand the websocket connection stays open. We are using chrome to record meetings in the cloud and we are doing regular liveness probes to ensure everything is still up and running (e.g. no crashes). I know thats not the typical use case.

[ZekeLu]

The use case looks plausible. At a first glance, I can not think of a way to close the connection while keeping the browser opened without modifying chromedp. But the change still feels weird to me.

Did you observe a low memory and cpu utilization if you apply the change? How about to keep the connection connected while disable all the events? Here is the list of events enabled by chromedp:

chromedp/chromedp.go

Lines 377 to 393 in ac112ec

	// Enable available domains and discover targets.
	actions := []Action{
	log.Enable(),
	network.Enable(),
	}
	// These actions are not available on a worker target.
	if !c.Target.isWorker {
	actions = append(actions, []Action{
	inspector.Enable(),
	page.Enable(),
	dom.Enable(),
	css.Enable(),
	target.SetDiscoverTargets(true),
	target.SetAutoAttach(true, false).WithFlatten(true),
	page.SetLifecycleEventsEnabled(true),
	}...)
	}

And we can create the context with chromedp.WithDebugf() to find out other events (if any) to disable.

[mariusziemke]

Yes we observed a huge improvement in our dev environment.

The two vertical blue lines show our deployments.

We don't want to keep the connection up and running because of potential network disconnects between our regions and regular deployments.

Why exactly does the change feel weird to you? Its a small change with huge impact in resiliency and performance for us.

[ZekeLu]

Sorry for the late reply. It look me some time to figure out the impact of this change.

Why exactly does the change feel weird to you?

My first thought is that the context is supposed to manage the lifetime of its target. If the context is cancelled, its target should be closed too. And there is the word browser in the title. I think to keep the browser opened is a little complicated.

But the use case is indeed plausible. And I asked myself, will I need this feature? The answer is yes, I will use this feature to attach to a tab, do something, and detach from the tab without closing it.

Here are my suggestions to improve this PR:

1. I have a test and it turns out that the change in this PR can not keep a browser staying opened if the browser is allocated with chromedp.ExecAllocator. This is not a problem. In fact, I think this is what it should be. When chromedp.RemoteAllocator is used to connect to a running browser, cancelling the context won't close the running browser. This is the current behavior without this change. So this PR won't change the behavior of the browser. I think we should remove the word browser from the title.
2. So the new feature is to detach from a tab without closing it. It's natural to use it with a remote browser which is connected with chromedp.RemoteAllocator. I think this is your use case. But this feature is not so helpful for the chromedp.ExecAllocator. First, if this option is applied to a context that manages the lifetime of a browser, it can't prevent the browser being closed (this is described in point 1). Second, if this option is applied to a context that manages the lifetime of a tab, it can't prevent the tab being closed if the root context is cancelled (we have to cancel the root context if we want to disconnect from the browser). I think we should document the behavior clearly.
3. I'm not a native English speaker, but the name dontClose does not look good to me. Do you have a better name?
4. The existing documentation for the cancel function should be updated to take this feature into account.
5. I think we should add unit tests to cover these cases:
   1. chromedp.ExecAllocator, apply this option to the root context. The expected result is the browser will be closed.
   2. chromedp.ExecAllocator, apply this option to a tab context. The expected result is the tab will stay opened until the root context is cancelled.
   3. chromedp.RemoteAllocator, apply this option to a context. The expected result is the tab will stay opened after the context is cancelled and the tab can be attached again.

[mariusziemke]

Thanks for the detailed response! I agree with you on all the points you made. 👍🏼

dontClose is quite a bad name indeed.. Here are some suggestions:

• dontCloseTarget
• dontCloseTab
• keepTargetOpen
• keepTabOpen

We could add a suffix like OnCtxCancel, but it would make the variable name quite long. I would suggest a short variable name and a good description in the helper method.

Gonna figure out how to write tests for this project :-)

[ZekeLu]

I would suggest a short variable name and a good description in the helper method.

Totally agree. Among of them, I like dontCloseTarget most.

Gonna figure out how to write tests for this project

Please let me know if you need any help. Thank you very much!