1396824 - Support HEAD requests

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

andreastt in https://github.com/mozilla/webdriver-rust/issues/51:

> In order to make imperative checks at runtime for support of
> certain commands, such as the window size- and positioning
> commands which may be unsupported on certain configurations, we
> need to support HTTP HEAD requests for all commands.

Comment 1

[Andreas Tolfsen ❲:ato❳]

This is actually a conformance issue.

Comment 2

[Greg Fraley]

Attachment: Handle HEAD requests as GETs and omit response body

Comment 3

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8955840 [details] [diff] [review]
Handle HEAD requests as GETs and omit response body

Review of attachment 8955840 [details] [diff] [review]:
-----------------------------------------------------------------

First of all, thanks for the patch!

The purpose of allowing HEAD requests to geckodriver would be to
check if a certain command is available by querying its endpoint.
Whilst it is true that the HEAD method is meant to be identical to
GET except for not returning a body, the fact that WebDriver maps
a remote-control interface on top of HTTP means this isn’t exactly
what is needed.

Instead of processing the command on a request with the HEAD method,
we could simply return 200 with an empty message-body if the command
exists, and 404 otherwise.  This would also apply to commands that
are POST requests.

I hope that makes sense.

Comment 4

[Greg Fraley]

Makes sense!  In that case, WebDriverHttpApi is probably a better place to handle the HEAD method.  That's where there is access to the method call and the code to make the call to handle the request.  Depending on the former, we can handle the HEAD shortroute by checking for the presence of the request handler, but omit the call itself.

If this is the correct approach, I can make a patch for the change as needed.

Comment 5

[Greg Fraley]

Attachment: Bug 1396824 - Handle head requests by sending 200 in event of WebDriverMessage existing

Review commit: https://reviewboard.mozilla.org/r/225676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225676/

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8956696 [details]
Bug 1396824 - Handle head requests by sending 200 in event of WebDriverMessage existing

https://reviewboard.mozilla.org/r/225676/#review231626

I don’t think this will work as expected because you will only end
up here if you match a route in WebDriverHttpApi::routes, and this
route list doesn’t mention any HEAD paths.

You will probably want to modify WebDriverHttpApi::decode_request()
to always return a 200 success if the route matches and the method
is HEAD:

    https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/testing/webdriver/src/httpapi.rs#225-244

Comment 7

[Greg Fraley]

It definitely appears you're correct; I was mistaken earlier (I misinterpreted the valid headers returned from the error message as valid *ok* headers).

It looks like the decode_request method does the mapping from HTTP call to WebDriverMessage (WebDriverCommand/Session pair), and doesn't provide an easy way to return out a generic 200, which happens one call up, in HttpHandler::handle.

Perhaps a combination of both approaches, whereby we map the HEAD call to a GET, send it to decode_request, and if it successfully decodes the command, we return headers instead of sending it down the the HttpHandler.chan channel, where it's actually handled/executed.

I'll give it a shot, and if it's the correct approach/it works, I'll push a patch to review.

Comment 8

[Greg Fraley]

Attachment: Bug 1396824 - Treat HEAD requests as GET requests when decoding and return 200 before executing

Review commit: https://reviewboard.mozilla.org/r/225788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225788/

Comment 9

[Greg Fraley]

I pushed a change with the fix that seems to address the problem at hand, but it does lack tests.  Would a test case of checking the HEAD of one of the commands (say, "url") suffice, or would we want something more comprehensive?

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8956828 [details]
Bug 1396824 - Treat HEAD requests as GET requests when decoding and return 200 before executing

https://reviewboard.mozilla.org/r/225788/#review232008

Your latest patch actually appears to work, but only for GET commands.

We can file a follow-up bug that we need WPT tests for all commands
for this.  You don’t have to add them here.

::: testing/webdriver/src/server.rs:179
(Diff revision 1)
> +        let (head_req, equivalent_req_method) = match req.method {
> +            Method::Head => (true, Method::Get),
> +            method => (false, method)
> +        };

Can you find some way of getting rid of this?  I don’t think it
is needed.

::: testing/webdriver/src/server.rs:180
(Diff revision 1)
>          }
>  
>          debug!("-> {} {} {}", req.method, req.uri, body);
>  
> +        let (head_req, equivalent_req_method) = match req.method {
> +            Method::Head => (true, Method::Get),

This will only make it work for GET commands.  I.e. /session/{session
id}/refresh will still return 404.

Comment 11

[Greg Fraley]

Attachment: Bug 1396824 - Treat HEAD requests as GET requests when decoding and return 200 before executing

Review commit: https://reviewboard.mozilla.org/r/226150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226150/

Comment 12

[Greg Fraley]

At a glance, it does appear that adding HEAD support for standard POST routes would require more substantial changes, so I did opt to solve GET routes only on this ticket.

The updated patch should omit the block upfront.  It does require a "req.method.clone()" because "api.decode_requests()" consumes.  It's minor, but I'm still mostly unaware to what standards are in place for these projects.

Comment 13

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-1396824-Support-HEAD-requests-in-geckodriver.patch

Sorry it has taken my long to get back to you on this.

I looked into this and have concluded any approach to fix this
without changing the existing program structure too much is futile.
I’m attaching my own attempt which is imperect in a lot of ways.

I’ve not requested review for it because I think a better and more
sustainable long-term approach would be to move geckodriver to use
one of the higher-level web frameworks which come with built-in
routing/route matching.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8975280 [details] [diff] [review]
0001-Bug-1396824-Support-HEAD-requests-in-geckodriver.patch

Now that we use serde in geckodriver, we will consider using either
a Rust web framework or a request router not written by us to
correctly supply HEAD requests.  This renders the attached patch
obsolete.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

And this cannot be done with hyper? Or why would we need a different framework?

Comment 16

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #15)

> And this cannot be done with hyper? Or why would we need a
> different framework?

All frameworks I know of use hyper internally, but hyper itself
does not to request routing.

eijebong suggested we look at warp, which is written by the maintainer
of hyper: https://seanmonstar.com/post/176530511587/warp

AIUI it is a sort of opinionated “mini” web framework, which at its
core provides request routing.

An important requirement to note here is that we need a solution
that can be supported in Rust stable.  This excludes frameworks
such as Rocket.

Comment 17

[Bastien Orivel [:eijebong]]

For information:
PoC patch here: https://github.com/Eijebong/gecko-dev/commit/15a78bcf4b0caec5407e168f17e2ceb999cfe4ab
Required to avoid adding a 4th version of rand in m-c: https://github.com/seanmonstar/warp/pull/229

Comment 18

[Bastien Orivel [:eijebong]]

Try build
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52b3ef40f76329f61e5407c86e42d2bf8daf7413

Hopefully I got everything correct in how I submitted it

Comment 19

[Bastien Orivel [:eijebong]]

Attachment: Bug 1396824 - Part 1: Replace the webdriver router by warp. r?ato

This allows for easy support for HEAD requests without any code on our
side.

Comment 20

[Bastien Orivel [:eijebong]]

Attachment: Bug 1396824 - Part 2: Revendor dependencies. r?ato

Depends on D34631

Comment 21

[Andreas Tolfsen ❲:ato❳]

We will also need WPT tests for this, so I’ve filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1559324 as a follow-up.

Comment 22

[Andreas Tolfsen ❲:ato❳]

I rebased the patches on Phabricator again, and I’m still seeing
405 Method Not Allowed.

eijebong, did you say this was an easy fix?

Comment 23

[Bastien Orivel [:eijebong]]

Yeah sorry, I had fixed it but forgot to push. This should be good now (also rebased)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbb1c8fa2aba
Part 1: Replace the webdriver router by warp. r=ato
https://hg.mozilla.org/integration/autoland/rev/4d31fcd3085d
Part 2: Revendor dependencies. r=ato

Comment 25

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/dbb1c8fa2aba
https://hg.mozilla.org/mozilla-central/rev/4d31fcd3085d