Skip to content

Commit

Permalink
Temporarily merge #19 to release in our fork
Browse files Browse the repository at this point in the history
Squashed commit of the following:

commit 69658a3
Author: acoulton <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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.
  • Loading branch information
acoulton committed Oct 17, 2024
1 parent 443f6d4 commit 369451a
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 19 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ Changelog

## Unreleased

## 2.9.4.2 (CUSTOM INGENERATOR RELEASE) (2024-10-17)

* Some minor connection enhancements #19

## 2.9.4.1 (CUSTOM INGENERATOR RELEASE) (2024-09-30)

* Fully resets our fork back to the unreleased version of upstream, plus adds PHP8.3 support. The majority of our
Expand Down
7 changes: 4 additions & 3 deletions src/ChromeDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ public function start()
}

if (isset($this->options['validateCertificate']) && $this->options['validateCertificate'] === false) {
$this->page->send('Security.enable');
$this->page->send('Security.setIgnoreCertificateErrors', ['ignore' => true]);
}
}
Expand Down Expand Up @@ -239,8 +238,10 @@ public function reset()
}
$this->switchToWindow($this->main_window);
$this->page->reset();
$this->request_headers = [];
$this->sendRequestHeaders();
if ($this->request_headers !== []) {
$this->request_headers = [];
$this->sendRequestHeaders();
}
}

/**
Expand Down
15 changes: 0 additions & 15 deletions src/ChromePage.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ public function connect($url = null): void
$this->send('Page.enable');
$this->send('DOM.enable');
$this->send('Network.enable');
$this->send('Animation.enable');
$this->send('Animation.setPlaybackRate', ['playbackRate' => 100000]);
$this->send('Console.enable');
}
Expand Down Expand Up @@ -232,20 +231,6 @@ protected function processResponse(array $data): bool
break;
case 'Inspector.targetCrashed':
throw new DriverException('Browser crashed');
case 'Animation.animationStarted':
if (!empty($data['params']['source']['duration'])) {
usleep($data['params']['source']['duration'] * 10);
}
break;
case 'Security.certificateError':
if (isset($data['params']['eventId'])) {
$this->send(
'Security.handleCertificateError',
['eventId' => $data['params']['eventId'], 'action' => 'continue']
);
$this->page_ready = false;
}
break;
case 'Console.messageAdded':
$this->console_messages[] = $data['params']['message'];
break;
Expand Down
7 changes: 6 additions & 1 deletion src/DevToolsConnection.php
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,12 @@ protected function waitFor(callable $is_ready)
}

if (is_null($response)) {
return null;
// There seems to be no valid case where we could actually get an explicit `null` value from the
// underlying websocket, other than perhaps the socket being closed by Chrome. Certainly, calling code
// (which does not generally check the response) should not blindly continue at this point.
throw new DriverException(
'Received unexpected NULL payload from Chrome websocket'
);
}

if ($data = json_decode($response, true)) {
Expand Down

0 comments on commit 369451a

Please sign in to comment.