Complete the migration from marionette-specific scripts. #10691
Conversation
| } | ||
| method = "WebDriver:ExecuteAsyncScript" if async else "WebDriver:ExecuteScript" | ||
| rv = self.marionette._send_message(method, body, key="value") | ||
| return self.marionette._from_json(rv) |
There was a problem hiding this comment.
I don't know all this code at all, so maybe I'm not the best reviewer. But I want to state that using the private message _send_message from Marionette isn't what I want to see. Can you please explain why this is necessary?
Also note that a script timeout of None will result in a hang if something goes wrong in a call to an async script.
There was a problem hiding this comment.
marionette.py doesn't expose the possibility to not set a sandbox. This is replicating the message that's set in geckodriver and it's tested in-tree in gecko, so I'm reasonably confident it won't break.
There was a problem hiding this comment.
@andreastt I would refer to you here. I don't like the _send_message usage at all, but maybe you have a better picture for that in regards of wptrunner.
There was a problem hiding this comment.
Wouldn’t method(script, sandbox=None, new_sandbox=False) work?
The problem we’re trying to circumvent here is that the Python client uses the string default as the default value for the sandbox kwarg, but if this is explicitly set to None it will pass {"sandbox": null, …} to Marionette.
Previously we removed the wrappedJSObject from executor_marionette.js. However it turns out that this migration was incomplete; in order to fully avoid the wrappedJSObject, we need to ensure that we never create a sandbox on the marionette side. As a result of this omission we were getting an error trying to call the timeout() function, causing more EXTERNAL-TIMEOUTs and so performance issues. The solution is to ensure that the sandbox is None for all calls to (content) js.
jgraham
force-pushed
the
marionette_sandbox_fixes
branch
from
6c95902
to
464dd8a
Compare
|
This apparently fixes the |
Previously we removed the wrappedJSObject from
executor_marionette.js. However it turns out that this migration was
incomplete; in order to fully avoid the wrappedJSObject, we need to
ensure that we never create a sandbox on the marionette side. As a
result of this omission we were getting an error trying to call the
timeout() function, causing more EXTERNAL-TIMEOUTs and so performance
issues.
The solution is to use the same approach as geckodriver, which isn't
exposed through the js marionette client. Essentially we have to
ensure that the sandbox is None for all calls to (content) js. Since
this isn't exposed in the client we construct the message directly and
ensure that all calls to content js co through the base protocolpart
rather than calling marionette.execute_* directly.
This change is