Initial round of stability fixes updated from the old spike branch #19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously this was silently skipping out of the waitFor loop, I don't think that can be right. There's no comment or information in the code or commit history to indicate why this was the case - it has been there ever since the very first commit. Possibly it was originally meant to be a `continue`? Even if we do encounter this happening, it seems entirely wrong to allow the calling code to continue without waiting for any further events or any indication that the thing it was actually waiting for has happened. Therefore throw an exception and we can then explore if / when / how to deal with it once we know when & why it happens.
Setting manual request headers on a Chrome instance seems somewhat unlikely, but it is exposed by the Mink interface and implemented by the driver so they do need to be cleaned up as part of ->reset() if present. However, if the browser or devtools connection has crashed / hit a problemn then calling Network.setExtraHttpHeaders can fail in a way that masks and/or makes it harder to trace the actual failure. Therefore, avoid sending this call unless it is actually required (e.g. some custom headers have been set since the session started or was last reset).
Animation events produce a flood of events on the dev-tools protocol particularly (in many projects) around the time of click, page-load etc events. This may be contributing to backlogging the socket and the driver occasionally missing a more important page completion etc event. The rate of animation events is particularly high because we are also setting a massive animation playback rate so animations start and stop very quickly. It is therefore virtually impossible to actually interact with an element during an animation. The code *looks* like it is sleeping when animations begin - this was introduced in b023983 and "fixed" in 0b8113f. However, in practice that fix actually just disabled the sleep because the parameter from DevTools is actually in `$data['params']['animation']['source']['duration']` (note the extra `animation` in the path). So the `if (!empty(` has always been returning false and the driver has never actually been running any code in response to these events. If there was a desire / need to reintroduce sleeping during animations in future, we'd need to review the units of the `duration` value, and whether just multiplying that by 10 is even close to the actual time taken when playing back at the forced accelerated rate.
DevTools has deprecated the old flow of setting overrideCertificateErrors and then handling an event notification. Instead they are just ignored upfront. The driver was migrated to the new flow in 457160a in 2018, but this (now-dead) code was not removed at the time. These events will not have been arriving since then, but leaves another place in the code where it looks like the page state might change.
acoulton
added a commit
that referenced
this pull request
Oct 17, 2024
Squashed commit of the following: commit 69658a3 Author: acoulton <andrew@ingenerator.com> Date: Wed May 11 16:33:43 2022 +0100 Remove handler for Security.certificateError DevTools has deprecated the old flow of setting overrideCertificateErrors and then handling an event notification. Instead they are just ignored upfront. The driver was migrated to the new flow in 457160a in 2018, but this (now-dead) code was not removed at the time. These events will not have been arriving since then, but leaves another place in the code where it looks like the page state might change. commit 8bd398f Author: acoulton <andrew@ingenerator.com> Date: Wed May 11 16:26:20 2022 +0100 Don't take `Animation` events, they aren't actually used Animation events produce a flood of events on the dev-tools protocol particularly (in many projects) around the time of click, page-load etc events. This may be contributing to backlogging the socket and the driver occasionally missing a more important page completion etc event. The rate of animation events is particularly high because we are also setting a massive animation playback rate so animations start and stop very quickly. It is therefore virtually impossible to actually interact with an element during an animation. The code *looks* like it is sleeping when animations begin - this was introduced in b023983 and "fixed" in 0b8113f. However, in practice that fix actually just disabled the sleep because the parameter from DevTools is actually in `$data['params']['animation']['source']['duration']` (note the extra `animation` in the path). So the `if (!empty(` has always been returning false and the driver has never actually been running any code in response to these events. If there was a desire / need to reintroduce sleeping during animations in future, we'd need to review the units of the `duration` value, and whether just multiplying that by 10 is even close to the actual time taken when playing back at the forced accelerated rate. commit 7106c4b Author: acoulton <andrew@ingenerator.com> Date: Tue May 10 14:40:11 2022 +0100 Only reset request headers after scenario if they were set Setting manual request headers on a Chrome instance seems somewhat unlikely, but it is exposed by the Mink interface and implemented by the driver so they do need to be cleaned up as part of ->reset() if present. However, if the browser or devtools connection has crashed / hit a problemn then calling Network.setExtraHttpHeaders can fail in a way that masks and/or makes it harder to trace the actual failure. Therefore, avoid sending this call unless it is actually required (e.g. some custom headers have been set since the session started or was last reset). commit 74528a2 Author: acoulton <andrew@ingenerator.com> Date: Mon May 9 23:13:53 2022 +0100 Treat a `null` websocket return as an exception Previously this was silently skipping out of the waitFor loop, I don't think that can be right. There's no comment or information in the code or commit history to indicate why this was the case - it has been there ever since the very first commit. Possibly it was originally meant to be a `continue`? Even if we do encounter this happening, it seems entirely wrong to allow the calling code to continue without waiting for any further events or any indication that the thing it was actually waiting for has happened. Therefore throw an exception and we can then explore if / when / how to deal with it once we know when & why it happens.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://gitlab.com/behat-chrome/chrome-mink-driver/-/merge_requests/188