fix bug in WaitLoad when event has circular reference

[15joeybloom]

This fixes a bug in WaitLoad that causes it to fail with "Object reference chain is too long". This happens when we take the branch of the WaitLoad js that listens for the "load" event. In this case the load event object is returned by the function, and the object's value is loaded from the browser by rod. Sometimes this object can have a circular reference, causing the CDP to throw the Object reference chain is too long error.

I have seen the event object have a circular reference on sites using bootstrap.bundle.min.js. That library adds a delegateTarget property to event objects. The way it does this is by mutating the event object in a "load" event handler, so delegateTarget may not always be added to the event object received by other "load" event listeners depending on the order in which the handlers fire. I set up an example site that shows the issue - here is a small script to hit the site over and over until the issue occurs. It usually only takes two tries for me to reproduce the issue.

package main

import (
	"github.com/go-rod/rod"
)

func main() {
	browser := rod.New().MustConnect()
	defer browser.MustClose()

	page := browser.MustPage()

	for {
		page.MustNavigate("https://joeysandbox.weebly.com")
		page.MustWaitLoad()
	}
}

The fix was very simple - just stop returning the event object from WaitLoad, because it's not necessary.

I added a unit test, but given the complex and timing-dependent nature of the bug, the test ended up being pretty complicated. And it's still not 100% robust as ultimately it depends on the firing order of event handlers.

I didn't look to see whether the same bug might be possible in any of the other js functions.

[ysmood]

Suggested change

	window.addEventListener('load', _=>resolve())
	window.addEventListener('load', resolve)

[ysmood]

Suggested change

	this.addEventListener('load', _=>resolve())
	this.addEventListener('load', resolve)

[15joeybloom]

@ysmood Could you explain your latest suggestions? I'm afraid I'm missing something. It looks like you're suggesting that I revert all my changes to helper.js.

[15joeybloom]

It's been a week so I'm going to re-request review as a reminder. Could you explain your latest suggestions? I'm afraid I'm missing something. It looks like you're suggesting that I revert all my changes to helper.js.

[ysmood]

Sorry for the late reply, it has been a busy season for me.

For javascript code like:

this.addEventListener('load', _=>resolve())

Is effectively the same as:

this.addEventListener('load', resolve)

Can you tell me in what case they will behave differently?

[15joeybloom]

Sorry for the late reply, it has been a busy season for me.

For javascript code like:

this.addEventListener('load', _=>resolve())

Is effectively the same as:

this.addEventListener('load', resolve)

Can you tell me in what case they will behave differently?

No worries!

These two expressions are actually not the same at all. Let me explain:

this.addEventListener('load', resolve) is equivalent to this.addEventListener('load', (e)=>resolve(e)). The event object is passed to the resolve function.

My change is this.addEventListener('load', _=>resolve()), which ignores the event object and passes nothing to the resolve function.

This is a critical distinction, because whatever value is passed to the resolve function, rod tries to load it into the Go process from the browser. In the case I explained in the PR description, when the event object has a circular reference, rod will get an "Object reference chain is too long" error if it tries to load the event object from the browser. But if we instead call resolve() with no arguments, rod will not attempt to load the event object, and so we avoid the "Object reference chain is too long" error.