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
Created attachment 434175 [details] Patch
the above patch fixes the issue. inspired by https://github.com/chromium/chromium/blob/d7da0240cae77824d1eda25745c4022757499131/chrome/test/chromedriver/js/call_function.js#L227
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?
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.
<rdar://problem/81371385>
Created attachment 434821 [details] Patch
(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
(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
Created attachment 434840 [details] Patch
Created attachment 434888 [details] Patch
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.
(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
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.
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
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
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?