Bug 228270 – Elements cloned into an iframe cannot be accessed by webdriver

• Component: WebDriver
• Status: NEW

[MoLow]

when cloning/creating elements inside an iframe, trying to access the element using Webdriver results in a `cannot serialize cyclic structures` Error.
this is since iframe.contentWindow.Node !== Node and the checkCyclic method fails to determine the cloned element is an element.
a minimal reproduction for creating such an element can be found here http://jsbin.testim.io/duq/3

[MoLow]

Created attachment 434175 [details]
Patch

[MoLow]

the above patch fixes the issue.
inspired by https://github.com/chromium/chromium/blob/d7da0240cae77824d1eda25745c4022757499131/chrome/test/chromedriver/js/call_function.js#L227

[Sam Sneddon [:gsnedders]]

Comment on attachment 434175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434175&action=review

(Non-reviewer r-)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:213
> +          (value.nodeType == NodeType.ELEMENT || value.nodeType == NodeType.DOCUMENT || value instanceof ShadowRoot);

The "value instanceof ShadowRoot" check still suffers from the exact same problem as described above.

My suggestion here would be something like:

let constructor = value.constructor;
do {
    if (constructor.name === "Element") return true;
} while(constructor = Object.getPrototypeOf(constructor));
return false;

(With whatever else is needed to make that safe, not thought too much about this! Still need to check that value is a non-null object, at least.)

Also: per spec this is just used by https://w3c.github.io/webdriver/#dfn-internal-json-clone-algorithm, and in that case per spec we simply want to check if its an instanceof Element; so do we even want to allow other Nodes?

[James Graham]

It would be useful to get a web platform test covering this case if there isn't already such a test. I don't know if/how one can write such a test in the WebKit tree, but I can help with submitting a test directly to https://github.com/web-platform-tests/wpt if required.

[Radar WebKit Bug Importer]

<rdar://problem/81371385>

[MoLow]

Created attachment 434821 [details]
Patch

[MoLow]

(In reply to Sam Sneddon [:gsnedders] from comment #3)
> Comment on attachment 434175 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434175&action=review
> 
> (Non-reviewer r-)
> 
> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:213
> > +          (value.nodeType == NodeType.ELEMENT || value.nodeType == NodeType.DOCUMENT || value instanceof ShadowRoot);
> 
> The "value instanceof ShadowRoot" check still suffers from the exact same
> problem as described above.
> 
> My suggestion here would be something like:
> 
> let constructor = value.constructor;
> do {
>     if (constructor.name === "Element") return true;
> } while(constructor = Object.getPrototypeOf(constructor));
> return false;
> 
> (With whatever else is needed to make that safe, not thought too much about
> this! Still need to check that value is a non-null object, at least.)
> 
> Also: per spec this is just used by
> https://w3c.github.io/webdriver/#dfn-internal-json-clone-algorithm, and in
> that case per spec we simply want to check if its an instanceof Element; so
> do we even want to allow other Nodes?

I have added a fix for shadow root - in a somwhat more performant way

[MoLow]

(In reply to James Graham from comment #4)
> It would be useful to get a web platform test covering this case if there
> isn't already such a test. I don't know if/how one can write such a test in
> the WebKit tree, but I can help with submitting a test directly to
> https://github.com/web-platform-tests/wpt if required.

Thanks for that! I have submitted a PR with a test for this: https://github.com/web-platform-tests/wpt/pull/29877

[MoLow]

Created attachment 434840 [details]
Patch

[MoLow]

Created attachment 434888 [details]
Patch

[youenn fablet]

Comment on attachment 434888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434888&action=review

> Source/WebKit/ChangeLog:8
> +        Added a new test at https://github.com/web-platform-tests/wpt/pull/29877

We probably want a non-regression test, either the WPT one or a WebKit specific one in this patch.

[MoLow]

(In reply to youenn fablet from comment #11)
> Comment on attachment 434888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434888&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Added a new test at https://github.com/web-platform-tests/wpt/pull/29877
> 
> We probably want a non-regression test, either the WPT one or a WebKit
> specific one in this patch.

I am not sure I understand. can you describe what such a test will perform? thanks

[youenn fablet]

You added a test in WPT (webdriver/tests/switch_to_frame/switch_webelement.py).
I guess we would want to run this test as part of WebKit CI, or run an equivalent but WebKit-specific test if we do not have ways to run WPT WebDriver tests yet.
I am not familiar with how WebDriver is being tested in WebKit.

[Tyler Wilcock]

You can import WPTs with the `Tools/Scripts/import-w3c-tests` script. For example:

Tools/Scripts/import-w3c-tests --tip-of-tree web-platform-tests/webdriver/tests/switch_to_frame

Assuming WebKit can run WebDriver WPTs (I'm also not sure), you may want to make the import a separate bug and patch, since it will pull in a few other tests too. You'd need to create test expectations for the newly imported tests — the new one you added would presumably be marked as fail. Then this patch would mark the test as passing.

Here's an example of a bug / patch that imports WPTs and sets expectations: https://bugs.webkit.org/show_bug.cgi?id=225369

[Tyler Wilcock]

Setting expectations is done by running the tests. The test runner will notice the newly imported tests are missing them and write new ones.

Tools/Scripts/run-webkit-tests --debug LayoutTests/imported/w3c/web-platform-tests/webdriver/tests/switch_to_frame

[Devin Rousso]

Comment on attachment 434888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434888&action=review

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:33
> +var NodeType = {

Style: `const`?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:34
> +    ELEMENT: 1,

Style: Is there a reason to uppercase these?  We normally CamelCase enums.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:36
> +    DOCUMENT_FRAGMENT_NODE: 11

Style: missing trailing comma

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:37
> +  };

Style: odd indentation

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:207
> +    _isElement(value) {

Style: `{` on it's own line

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:212
> +        return ((typeof value == 'object' && value != null) ||

Style: please use `===` and `!==` unless you have a really good reason not to
Style: we prefer `"`
Style: can we split this into separate `if` or something?  Maybe use some early-returns and separate each line out so it's easier to read?