Open Bug 1274251 Opened 9 years ago Updated 1 year ago

Provide JSON serialisation and deserialization of Window and Frame

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Points:
3

Tracking

(Not tracked)

People

(Reporter: ato, Assigned: whimboo)

References

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

Details

(Whiteboard: [webdriver:m8])

Attachments

(2 files, 2 obsolete files)

[marionette] Serialize Web Frame and Web Window objects
(deleted), patch
Details | Diff | Splinter Review
[wdspec] Add tests for execute (async) script to return element, window, and frame identifiers
(deleted), patch
Details | Diff | Splinter Review
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
Blocks: webdriver
Keywords: ateam-marionette-server
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
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

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

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3

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

Keywords: pi-marionette-server
Whiteboard: [webdriver:backlog]
Summary: Provide JSON serialisation of Window object → Provide JSON serialisation of Window (Frame) object

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

Summary: Provide JSON serialisation of Window (Frame) object → Provide JSON serialisation of Window, Frame and plain objects

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

See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1794078
Summary: Provide JSON serialisation of Window, Frame and plain objects → Provide JSON serialisation of Window and Frame

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"}}]
Depends on: 1692468
No longer depends on: 1491490
Severity: normal → S3
Product: Testing → Remote Protocol
Points: --- → 3
Priority: P3 → P1
Whiteboard: [webdriver:backlog] → [webdriver:m7]
Priority: P1 → P2

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.

Depends on: 1822466

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

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
See Also: → https://github.com/w3c/webdriver/pull/1738
Summary: Provide JSON serialisation of Window and Frame → Provide JSON serialisation and deserialization of Window and Frame
Depends on: 1837031
Whiteboard: [webdriver:m7] → [webdriver:m8]

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

Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: