Andreas Tolfsen ❲:ato❳ Reporter
	Description • 8 years ago

The WebDriver specification recently changed to require a JSON serialisation of the Window object when you return it from Execute Script or Execute Async Script:

    http://w3c.github.io/webdriver/webdriver-spec.html#executing-script

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 1 • 6 years ago

It's actually kinda easy to get done now.

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 4 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b33fac82df3c27b890a6f1db4f8ea87f89296377&selectedJob=199165487

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 6 • 6 years ago

Comment on attachment 9008869 [details] [diff] [review]
[wdspec] Add tests for execute (async) script to return element, window, and frame identifiers

Review of attachment 9008869 [details] [diff] [review]:
-----------------------------------------------------------------

I just have seen `json_serialize_windowproxy.py` which has test cases for this scenario. I will have to refactor the tests a bit.

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 9 • 6 years ago

The latest update of the patch returns the correct window handle now for both WebWindow and WebFrame types. Strangely the WindowProxy as returned by a non-privileged sandbox via `function() { return window; }` seems to be double wrapped, and as such needs an extra call to `XPCNativeWrapper()` before properties like `windowUtils` can be accessed. This can be seen in the first patch which is for Marionette.

Bobby, I was already chatting with Olli about this behavior and he was referring me to you. Do you have an idea why code in a JS module as loaded via `ChromeUtils.import()` from inside a frame script (tabchildglobal) has to unwrap the WindowProxy a second time before it gets access to privileged properties and methods? I tried to simulate the same in a Scratchpad but wasn't successful. In such a case the WindowProxy isn't wrapped anymore, and I have direct access to `windowUtils`. Thanks!

	Bobby Holley (:bholley)
	Comment 10 • 6 years ago

Double-wrapped isn't really a thing, and XPCNativeWrapper is a deprecated name.

All XPCNativeWrapper() does is forward to Cu.unwaiveXrays(). Presumably that means that the object is waived, which you can verify by confirming that obj != Cu.unwaiveXrays(obj) (which it sounds like you effectively have). Privileged properties are only accessible via the Xray view, so that sounds like what's happening.

The question of _why_ it's happening is interesting. Are you waiving Xrays on the sandbox somehow before calling the function? You can always strip the waiver via Cu.unwaiveXrays() per above, but it would be nice to understand where it comes from.

	Bobby Holley (:bholley)
	Comment 11 • 6 years ago

(and note that waivers are transitive: Cu.waiveXrays(x).y (equivalent to x.wrappedJSObject.y) will be waived).

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 12 • 6 years ago

(In reply to Bobby Holley (:bholley) from comment #10)
> All XPCNativeWrapper() does is forward to Cu.unwaiveXrays(). Presumably that
> means that the object is waived, which you can verify by confirming that obj
> != Cu.unwaiveXrays(obj) (which it sounds like you effectively have).

Correct. I added this check and it returns `true` as you expected.

> The question of _why_ it's happening is interesting. Are you waiving Xrays
> on the sandbox somehow before calling the function? You can always strip the
> waiver via Cu.unwaiveXrays() per above, but it would be nice to understand
> where it comes from.

I don't know how someone would waive Xrays, maybe via Cu.waiveXrays()? But that is not what we are doing in Marionette. The only options we set on the sandbox when creating it are those as listed here:

https://dxr.mozilla.org/mozilla-central/rev/e923330d5bd3aef1f687eddf96a06ad5ec3860ed/testing/marionette/evaluate.js#430-457

By checking those options I looked at:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.Sandbox

And what's interesting is the `wantXrays` option, and especially the note in the red box which exactly matches what we have here:

> Setting wantXrays to false also causes Xrays to be waived on objects in the sandbox.
> If you are creating a sandbox with lower privileges than the current compartment,
> this is probably not what you want.

So I assume this is a failure in our code and we should always keep it at true when executing a script in content scope, but probably not in chrome scope. Or should this always be true?

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 13 • 6 years ago

Oh please note that with geckodriver and Selenium we also have to make sure that testers can retrieve expandos as set on DOM nodes. Maybe that is related here?

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 14 • 6 years ago

When I always unwaive the return value of the script evaluation in the sandbox some tests fail like the following:

https://dxr.mozilla.org/mozilla-central/rev/e923330d5bd3aef1f687eddf96a06ad5ec3860ed/testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py#332-340

The script here returns an object with a `toJSON()` method, and later when we try to access it the following failure is thrown:

> [Child 67815, Main Thread] WARNING: Silently denied access to property "toJSON": value is callable (@chrome://marionette/content/evaluate.js:281:13): file /builds/worker/workspace/build/src/js/xpconnect/wrappers/XrayWrapper.cpp, line 226

So it looks like I cannot always unwaive a return value.

	Bobby Holley (:bholley)
	Comment 15 • 6 years ago

wantXrays=true is the default, so there's no need to specify it.

The only reason you might want to specify wantXrays=false is if you want your sandbox to have non-Xray vision to other _same-origin_ globals. That has the additional unfortunate side-effect of causing the result of the Cu.Sandbox constructor (the sandbox itself) to be waived, which is kind of non-sensical. You can get that behavior yourself by just doing Cu.waiveXrays on that value, and you can undo it by doing Cu.unwaiveXrays on it.

I agree that this side-effect is super weird and annoying, and we may be able to just get rid of it now that we've dropped legacy extension. Let's see what breaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f54c99cadbc50e595b35d692c4e3d2defe0105c9

	Bobby Holley (:bholley)
	Comment 16 • 6 years ago

Patches in bug 1491488 should clear this up. That doesn't solve your specific issue in this bug per se, but makes the semantics of what's happening more clear.

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 17 • 6 years ago

Thanks Bobby! I will then wait for bug 1491488 being fixed first before continuing on that patch.

	Henrik Skupin [:whimboo][⌚️UTC+2] Assignee
	Comment 18 • 6 years ago

It might also be worth waiting for bug 1491490 to be fixed.