Open Bug 1274251 Opened 4 years ago Updated Last year

Provide JSON serialisation of Window object

Categories

(Testing :: Marionette, defect, P1)

Version 3
defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: ato, Assigned: whimboo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: pi-marionette-server)

Attachments

(2 files, 2 obsolete files)

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
Depends on: 1274274
Depends on: 1274638
Priority: -- → P2
It's actually kinda easy to get done now.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
Duplicate of this bug: 1420461
Attachment #9008868 - Flags: review?(ato)
Attachment #9008869 - Flags: review?(ato)
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.
Attachment #9008869 - Flags: review?(ato)
Attachment #9008868 - Flags: review?(ato)
Attachment #9008868 - Attachment is obsolete: true
Attachment #9008869 - Attachment is obsolete: true
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!
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
(and note that waivers are transitive: Cu.waiveXrays(x).y (equivalent to x.wrappedJSObject.y) will be waived).
(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?
Flags: needinfo?(bobbyholley)
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?
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.
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
Flags: needinfo?(bobbyholley)
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.
Thanks Bobby! I will then wait for bug 1491488 being fixed first before continuing on that patch.
Depends on: 1491488
It might also be worth waiting for bug 1491490 to be fixed.
Depends on: 1491490
You need to log in before you can comment on or make changes to this bug.