Marionette.{execute_script,execute_async_script} are missing type checks for script_args

RESOLVED WONTFIX

Status

defect
P3
normal
RESOLVED WONTFIX
2 years ago
a year ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 1 bug)

Version 3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Marionette does not type check the input of the `script_args` keyword argument.  This means you can pass any type to it, but it should only accept arrays.
(Assignee)

Updated

2 years ago
Blocks: webdriver

Comment 1

2 years ago
@:ato , Sir i would like to take this up.
(Assignee)

Comment 2

2 years ago
Please read https://wiki.mozilla.org/User:Mjzffr/New_Contributors and attach your patch for review here.
07yogeshgupta, I would suggest that you read chapter 7) in the document Andreas pointed out. Best is to use mozreview for any changes you do, which will be patches and not full files. Thanks.

Comment 7

2 years ago
I have submitted the patch [here](https://reviewboard.mozilla.org/r/111766/) . Please let me know what else needs to done. :)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8836309 [details]
Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list

https://reviewboard.mozilla.org/r/111766/#review113228

Something seems to have went wrong with uploading the diff. The files as contained here are not realted to the bug at all. Please check again, and get a correct patch uploaded. Thanks.

Btw. if a patch is ready you want to request review. For this bug it will be :ato. You can do that directly from within mozreview, or by adding `r?ato` at the end of your commit message.

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8836309 [details]
Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list

https://reviewboard.mozilla.org/r/111766/#review113228

Sorry sir, to reply so late. I can see there are 3 files but i wanted to upload just `testing/marionette/client/marionette_driver/marionette.py` . can you please tell me if the changes in `marionette.py` file is correct? 
if not then can you please comment on the bugzilla page and explain the issue again in little more detail please. thank you :)
(Assignee)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8836309 [details]
Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list

https://reviewboard.mozilla.org/r/111766/#review115400

::: .ycm_extra_conf.py:19
(Diff revision 1)
>  if not os.path.exists(path):
>      path = os.path.join(os.path.dirname(__file__), 'config.status')
> -    config = imp.load_module('_buildconfig', open(path), path, ('', 'r', imp.PY_SOURCE))
> +    config = imp.load_module('_buildconfig', open(
> +        path), path, ('', 'r', imp.PY_SOURCE))
>      path = os.path.join(config.topsrcdir, 'mach')
> -mach_module = imp.load_module('_mach', open(path), path, ('', 'r', imp.PY_SOURCE))
> +mach_module = imp.load_module('_mach', open(
> +    path), path, ('', 'r', imp.PY_SOURCE))
>  
>  sys.dont_write_bytecode = old_bytecode
>  
> +

Unrelated changes.

::: client.py:24
(Diff revision 1)
>  
>  topsrcdir = os.path.dirname(__file__)
>  if topsrcdir == '':
>      topsrcdir = '.'
>  
> +

All the changes to client.py are unnecessary whitespace changes as far as I can tell.

::: testing/marionette/client/marionette_driver/marionette.py:1792
(Diff revision 1)
>                  "line": int(frame[1]),
>                  "filename": os.path.basename(frame[0])}
>          rv = self._send_message("executeScript", body, key="value")
>          return self._from_json(rv)
>  
> -    def execute_async_script(self, script, script_args=(), new_sandbox=True,
> +    def execute_async_script(self, script, script_args=[], new_sandbox=True,

Arguments should be given as tuples in Python.  The type check should be made in the Marionette server, not the client: http://searchfox.org/mozilla-central/source/testing/marionette/driver.js#841
Attachment #8836309 - Flags: review-

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8836309 [details]
Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list

https://reviewboard.mozilla.org/r/111766/#review115400

> Arguments should be given as tuples in Python.  The type check should be made in the Marionette server, not the client: http://searchfox.org/mozilla-central/source/testing/marionette/driver.js#841

in `executeAsyncScript`, i cannot find `script_args` argument. there are `script` and `args` two arguments. so should i type check these two?
That's correct, and you may also want to include the timeout argument too.
Priority: -- → P3
(Assignee)

Updated

a year ago
Priority: P3 → P2
Summary: Missing type check for Execute (Async) Script in Marionette → WebDriver:ExecuteScript is missing type check for script_args
(Assignee)

Updated

a year ago
Priority: P2 → P3
(Assignee)

Updated

a year ago
Summary: WebDriver:ExecuteScript is missing type check for script_args → Marionette.{execute_script,execute_async_script} are missing type checks for script_args
(Assignee)

Updated

a year ago
Attachment #8836309 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8828788 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

Andreas, why do we have to add this check to the Python client? It should really be added to Marionette itself, given that I do not see any type checks for parameters in driver.js for both methods.
Attachment #8954154 - Flags: review?(hskupin) → review-
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

It would be fine to add type checks to the server too.  I just
checked, and they are missing.

This bug arised from incorrect use of the client API in the past,
so I don’t see why checking the type before it is sent is so bad?

Comment 16

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

None of the existing methods have such a check, so I wouldn't start doing it now with just those two methods. We simply pass-through everything.
(Assignee)

Comment 17

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

Well I propose adding it here because we have had bugs with this in the past.  I’m trying to solve a real issue here.

Comment 18

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

We had those bugs because we don't do type checks in the server component. And yes, I ca see why we want type checks here similar to geckodriver, but why now and only for those methods. Also please note that with your implementation the error types differ between what the client will raise, and what is raised by the server.
(Assignee)

Comment 19

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

So I don’t understand your point exactly.  Do we expect the error
types that are raised to be in sync between the client and the server?
That is not how most clients APIs are implemented.

If indeed you think it is a bad idea to have type checking in the
client (again I don’t understand why) would you be inclined to
accept a patch that adds the type checking to the server?

Comment 20

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

> Do we expect the error types that are raised to be in sync between the client and the server? That is not how most clients APIs are implemented.

I don't care about other client APIs. Fact is that both methods would cause problems for consumers of Marionette client because they throw a differnt kind of exception. Please lets avoid that.

And yes, I would be happy to see a patch for Marionette server.
(Assignee)

Comment 21

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

> I don't care about other client APIs.

That is a strange position to take.

> Fact is that both methods would cause problems for consumers
> of Marionette client because they throw a differnt kind of
> exception. Please lets avoid that.

FWIW I’m not sure I agree with this assessment, as the client
would inherently be safer.  But I will close this bug as WONTFIX and
file a new one to add the error to the server.
(Assignee)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX

Comment 22

a year ago
mozreview-review-reply
Comment on attachment 8954154 [details]
Bug 1329184 - Add script_args type checks to Python client.

https://reviewboard.mozilla.org/r/223314/#review229466

Maybe it was too harsh. What I wanted to say is that I don't see a value to change the client behavior for only two methods based on what other clients might handle API calls. If we want to chagne something here we should do it all at once. But I doubt that this is something we have time for.
You need to log in before you can comment on or make changes to this bug.