Marionette sets window.onunload in ways that are web-exposed
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox70 fixed)
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jgraham)
References
Details
Attachments
(2 files)
See discussion starting at https://github.com/web-platform-tests/wpt/issues/14254#issuecomment-514809871
To summarize, the code at https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/testing/marionette/evaluate.js#133 changes the state of sb.window
in a way that affects the behavior of the webpage loaded in that window. It should really use addEventListener
if possible to avoid that problem.
Comment 1•5 years ago
|
||
Forwarding the needinfo from James for Andreas via IRC to Bugzilla.
Comment 2•5 years ago
|
||
We had issues in the past that websites would replace addEventListener
and try to inspect the unloadHandler
object created in the frame script.
This would cause Permission denied to access property "invoke"
errors
when they tried introspecting the handler.
Presumably setting onload
exhibits the same problem,
because web documents could overload/introspect that too,
so the current approach for evaluating scripts in Marionette is not really sufficient.
Reporter | ||
Comment 3•5 years ago
|
||
We had issues in the past that websites would replace addEventListener
Is the relevant code not running with Xrays to the site? If not, why not? If it is, the site replacing addEventListener
would not be an issue...
Comment 4•5 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)
We had issues in the past that websites would replace addEventListener
Is the relevant code not running with Xrays to the site? If not, why not?
If it is, the site replacingaddEventListener
would not be an issue...
It’s not, see here: https://searchfox.org/mozilla-central/rev/0bcdfa99339c0512e3730903835fb6f8ed45e3c4/testing/marionette/evaluate.js#488
I think there was some reason that {wantsXrays: true}
caused scripts evaluated in the sandbox
not to have lasting effects on the DOM or something, but my memory may be failing me.
Assignee | ||
Comment 5•5 years ago
|
||
So, aiui the WebDriver spec expects that injected script runs as if it were content script i.e. without Xrays. So I think the problem here is that we are using addEventListener
(or onunload
) in the injected script sandbox, and should instead run it in a different sandbox with Xrays enabled. I have a patch and a test at https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa3e8f10d89df81565dd6d7f19bd8fa5e5e10788 but it's possible that approach is overkill and we can just run the event linseners inside the same context as evaluate.js
itself.
Assignee | ||
Comment 6•5 years ago
|
||
Internal marionette listeners should neither be visible to content
script, nor have their registation affected by changes made in
content. The evaluate method was breaking these constraints by
creating listeners in a sandbox with Xrays disabled, which is
appropriate to the injected script itself but not to the
harness-internal parts.
Use a different sandbox for the harness code compared to the injected
code, and move away from using onunload to using addEventListener for
the unload handler.
Assignee | ||
Comment 7•5 years ago
|
||
Gecko had a bug where content would be able to see an internal
onunload handler used to handle the case where the page navigates when
script is running. This test ensures that onunload isn't set when
running a script and that changes to addEventListener from content
aren't visible to the harness internal code.
Updated•5 years ago
|
Updated•5 years ago
|
Pushed by james@hoppipolla.co.uk: https://hg.mozilla.org/integration/autoland/rev/a4c7c6d1e546 Ensure marionette event listeners are isolated from content, r=ato,bzbarsky https://hg.mozilla.org/integration/autoland/rev/341ba1609c44 Test that content overriding listeners doesn't affect script evaluation, r=ato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18149 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/18149 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/H90iO0hQTBWFFFIjrW0Ovw)
Comment 11•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a4c7c6d1e546
https://hg.mozilla.org/mozilla-central/rev/341ba1609c44
Upstream PR merged
Updated•1 year ago
|
Description
•