Fix PageGotoTests

[leonardo-fernandes]

I started investigating why PageGotoTests were failing sometimes with NullReferenceException (e.g. https://github.com/hardkoded/puppeteer-sharp/actions/runs/5257640661/jobs/9500723964).

I found that, when the tests fails, the Response object wasn't initialized yet in the test's thread, but another thread was concurrently processing a message that would initialize it. More investigation indicated that the timing and order of two messages would determine if the test passed/failed.

• The Page.navigate message is sent to initiate the navigation. This message awaits for NewDocumentNavigationTask from LifecycleWatcher.
• Two messages, Network.responseReceived and Network.responseReceivedExtraInfo, are received as a result of the navigation. It seems that Network.responseReceived is always delivered before the Page.navigate resolves. But the order that the two messages are delivered varies with each test execution.
• When Network.responseReceivedExtraInfo is delivered before Network.responseReceived, the code in NetworkManager.OnResponseReceived is able to find the extraInfo and proceeds to execute EmitResponseEvent() which sets the Response object.
• When Network.responseReceived is delivered before Network.responseReceivedExtraInfo, the execution of NetworkManager.OnResponseReceived does not find the extraInfo object and queues the response event. In this state, the NewDocumentNavigationTask can resolve and continue the test execution before the Response object is set.

I compared the relevant code with upstream, and found some differences that were committed in puppeteer/puppeteer#8717. My understanding is that the _newDocumentNavigation flag was being set too early by a same-document navigation (somehow related to beforeunload events), and thus causing the NewDocumentNavigationTask to resolve before it should have. I am still investigating if that is sufficient to make sure the Response object is set.

I also found that ShouldWorkWhenNavigatingTo404 was failing consistently when executed in headful mode, and this had been fixed upstream by puppeteer/puppeteer#9577.

[kblok]

Two messages, Network.responseReceived and Network.responseReceivedExtraInfo, are received as a result of the navigation. It seems that Network.responseReceived is always delivered before the Page.navigate resolves. But the order that the two messages are delivered varies with each test execution.

Do you think that could be a problem? Do you see that in the logs? Because if that's coming out of order from the browser, puppeteer might be dealing with the same.

[kblok]

I compared the relevant code with upstream, and found some differences that were committed in puppeteer/puppeteer#8717.

We just solved the same thing :)

[leonardo-fernandes]

Do you think that could be a problem? Do you see that in the logs? Because if that's coming out of order from the browser, puppeteer might be dealing with the same.

For reference, I forced a Thread.Sleep() just before the assignment of the Response object. Without the fix, the test fails with NullPointerException in about 2% of executions (the failure still depends on the order of the Network.responseReceived and Network.responseReceivedExtraInfo messages). With the fix, it didn't fail in 200 executions.