1274251 - Provide JSON serialisation and deserialization of Window and Frame

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Andreas Tolfsen ❲:ato❳]

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

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It's actually kinda easy to get done now.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Serialize Web Frame and Web Window objects

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [wdspec] Add tests for execute (async) script to return element, window, and frame identifiers

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Serialize Web Frame and Web Window objects

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [wdspec] Add tests for execute (async) script to return element, window, and frame identifiers

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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!

Comment 10

[Bobby Holley (:bholley)]

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.

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Bobby Holley (:bholley)]

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.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Currently I'm not working on this bug, also it's blocked by some other code, which has to land first.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This bug hasn't been updated for a while but could become important again for our WebDriver BiDi implementation.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually we also fail to serialize plain objects like when returning a document correctly.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

General JS objects are actually different and is handled on bug 1794078.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While it seems to be much easier to do this nowadays I'll wait with the implementation until bug 1692468 has been done. This is mainly because it involves quite a bit of refactoring of the toJSON and fromJSON commands where by parts of that is done on the other bug.

Also it doesn't look like that we need bug 1491490. Without it we are able to serialize a window:

1665417978741	Marionette	DEBUG	3 -> [0,2,"WebDriver:ExecuteScript",{"script":"return window","args":[],"newSandbox":true,"sandbox":"default","line":106,"filename":"_a/test_minimized.py"}]
1665417978745	Marionette	TRACE	[10] MarionetteCommands actor created for window id 4294967297
1665417982274	Marionette	DEBUG	3 <- [1,2,null,{"value":{"window-fcc6-11e5-b4f8-330a88ab9d7f":"10"}}]

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

On bug 1822466 I'll add some intercepting code to the JSON cloning and deserialization algorithms that will help us with getting this feature added to Marionette.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm working on this bug and have a nearly complete implementation. Whereby while trying to add the deserialization steps I noticed that the current WebDriver classic specification doesn't have any definitions. So I've created a PR which is going to add them:

https://github.com/w3c/webdriver/pull/1738

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #25)

https://github.com/w3c/webdriver/pull/1738

Note that this is still blocked on this PR. Until it hasn't been merged I'm going to put the bug back to new.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #26)

https://github.com/w3c/webdriver/pull/1738

Note that this is still blocked on this PR. Until it hasn't been merged I'm going to put the bug back to new.

It's merged now. So I'll work on this next.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1274251 - [wdspec] Add proper support for tests to use WebFrame and WebWindow.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1274251 - [marionette client] Add proper support for tests to use WebFrame and WebWindow.

Depends on D191434

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1274251 - [remote] Add support for serializing and deserializing of window objects.

Depends on D191435

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92a611eda6b1
[wdspec] Add proper support for tests to use WebFrame and WebWindow. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/7c8760c20a62
[marionette client] Add proper support for tests to use WebFrame and WebWindow. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/bcdbc81d4adf
[remote] Add support for serializing and deserializing of window objects. r=webdriver-reviewers,jdescottes

Comment 32

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 33

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/42708 for changes under testing/web-platform/tests

Comment 34

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/92a611eda6b1
https://hg.mozilla.org/mozilla-central/rev/7c8760c20a62
https://hg.mozilla.org/mozilla-central/rev/bcdbc81d4adf

Comment 35

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot