Remove capabilities that should be handled by the intermediary nodes

RESOLVED INVALID

Status

Testing
Marionette
RESOLVED INVALID
2 years ago
6 months ago

People

(Reporter: automatedtester, Assigned: automatedtester)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Currently when we receive some firefox only capabilities, this can cause issue when people are requesting a new session. These should just be deleted and ignored if they are in desired/required
(Assignee)

Comment 1

2 years ago
Created attachment 8761865 [details]
Bug 1279370: Remove capabilities that we are meant for intermediary nodes.

The following capabilities should have been handled by the intermediary nodes
so we should just delete them before they cause any issues.

Review commit: https://reviewboard.mozilla.org/r/58908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58908/
Attachment #8761865 - Flags: review?(ato)
https://reviewboard.mozilla.org/r/58908/#review55838

::: testing/marionette/driver.js:589
(Diff revision 1)
> +
> +        // Remove capabilities that we can't do anything about and that should
> +        // have been sorted by the intermediary node.
> +        if (["firefox_profile", "args", "binary"].indexOf(cap) > -1) {
> +          try {
> +            delete from.requiredCapabilities[cap]

I thought the point of a required capability was that we would raise an exception if it can't be met, rather than silently ignoring it. Wouldn't it make more sense for intermediary nodes to consume capabilities that are used for routing and not forward them on? Alternatively, it might make sense to separate capabilities that are explicitly used for routing (perhaps these are truly the desired capabilities).
(Assignee)

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/58908/#review55838

> I thought the point of a required capability was that we would raise an exception if it can't be met, rather than silently ignoring it. Wouldn't it make more sense for intermediary nodes to consume capabilities that are used for routing and not forward them on? Alternatively, it might make sense to separate capabilities that are explicitly used for routing (perhaps these are truly the desired capabilities).

These are only for ones that we can't error on because something like GeckoDriver/Selenium Jar should have processed them.

As for certain items not being in in required/desired, James has thoughts on this and I am happy to remove this code is it is not useful after the F2F but this code, to me at least, is just another layer of validation in the process.
Comment on attachment 8761865 [details]
Bug 1279370: Remove capabilities that we are meant for intermediary nodes.

https://reviewboard.mozilla.org/r/58908/#review56590

It’s unclear to me from reading the spec what the implementation should do when faced with capability entries it does not know what to do with.  I think the spec is meant to not say anything explicit about this, as implementation-specific capabilities are supposed to be ignored.  Of course, what we do with these internally is entirely left to our discretion.

So in conclusion, I don’t think this patch is so bad although I do feel these capabilities should have been consumed in geckodriver.  When we discussed capabilities at the last F2F we explicitly talked about how the intermediary can consume and optionally choose to pass on the consumed capabilities to the driver.  I think it should consume these since there’s nothing Marionette can do about them.
Attachment #8761865 - Flags: review?(ato) → review-
(Assignee)

Comment 5

6 months ago
new session has been rewritten so this isnt valid anymore
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.