Closed Bug 1329184 Opened 7 years ago Closed 6 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P3)

Version 3
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: webdriver
@:ato , Sir i would like to take this up.
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.
I have submitted the patch [here](https://reviewboard.mozilla.org/r/111766/) . Please let me know what else needs to done. :)
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 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 :)
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 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
Priority: P3 → P2
Summary: Missing type check for Execute (Async) Script in Marionette → WebDriver:ExecuteScript is missing type check for script_args
Priority: P2 → P3
Summary: WebDriver:ExecuteScript is missing type check for script_args → Marionette.{execute_script,execute_async_script} are missing type checks for script_args
Attachment #8836309 - Attachment is obsolete: true
Attachment #8828788 - Attachment is obsolete: true
Assignee: nobody → ato
Status: NEW → ASSIGNED
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-
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 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.
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 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.
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 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.
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
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.
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: