Closed Bug 1568991 Opened 5 months ago Closed 5 months ago

Marionette sets window.onunload in ways that are web-exposed

Categories

(Testing :: Marionette, defect, P1)

Version 3
defect

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
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.

Forwarding the needinfo from James for Andreas via IRC to Bugzilla.

Flags: needinfo?(ato)

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.

Flags: needinfo?(ato)
Priority: -- → P3
See Also: → 1353074

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...

Flags: needinfo?(ato)

(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 replacing addEventListener 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.

Flags: needinfo?(ato)

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.

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.

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.

Assignee: nobody → james
Status: NEW → ASSIGNED
Priority: P3 → P1
Attachment #9080945 - Attachment description: Bug Bug 1568991 - Test that content overriding listeners doesn't affect script evaluation, → Bug 1568991 - Test that content overriding listeners doesn't affect script evaluation,
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)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.