Closed
Bug 1387678
Opened 7 years ago
Closed 6 years ago
Intermittent test_shadow_dom.py TestShadowDom.test_chrome_error | NoSuchElementException: Unable to locate shadow root: 0724bef1-f58e-4f8a-9fce-41673c2f5fd2
Categories
(Testing :: Marionette Client and Harness, defect, P1)
Tracking
(firefox56 disabled, firefox57 disabled, firefox62 fixed)
RESOLVED
FIXED
mozilla62
People
(Reporter: intermittent-bug-filer, Assigned: automatedtester)
References
Details
(Keywords: intermittent-failure, Whiteboard: [stockwell unknown])
Attachments
(9 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
jmaher
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
Filed by: philringnalda [at] gmail.com https://treeherder.mozilla.org/logviewer.html#?job_id=121103309&repo=mozilla-central https://queue.taskcluster.net/v1/task/CDbYCkVGSWe0FsWesG6udA/runs/0/artifacts/public/logs/live_backing.log
Comment hidden (Intermittent Failures Robot) |
Comment 2•7 years ago
|
||
[task 2017-08-04T21:02:36.735712Z] 21:02:36 INFO - 1501880556725 Marionette DEBUG Received DOM event "pageshow" for "http://127.0.0.1:53964/test_shadow_dom.html" [task 2017-08-04T21:02:36.743310Z] 21:02:36 INFO - 1501880556737 Marionette TRACE 137 <- [1,10,null,{}] [task 2017-08-04T21:02:36.744450Z] 21:02:36 INFO - 1501880556741 Marionette TRACE 137 -> [0,11,"findElement",{"using":"id","value":"host"}] [task 2017-08-04T21:02:36.763839Z] 21:02:36 INFO - 1501880556751 Marionette TRACE 137 <- [1,11,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"0724bef1-f58e-4f8a-9fce-41673c2f5fd2","ELEMENT":"0724bef1-f58e-4f8a-9fce-41673c2f5fd2"}}] [task 2017-08-04T21:02:36.763960Z] 21:02:36 INFO - 1501880556753 Marionette TRACE 137 -> [0,12,"switchToShadowRoot",{"id":"0724bef1-f58e-4f8a-9fce-41673c2f5fd2"}] [task 2017-08-04T21:02:36.800170Z] 21:02:36 INFO - 1501880556788 Marionette TRACE 137 <- [1,12,{"error":"no such element","message":"Unable to locate shadow root: 0724bef1-f58e-4f8a-9fce-41673c2f5fd2","stacktrace":"WebDriverError@chrome://marionette/content/error.js:227:5\nNoSuchElementError@chrome://marionette/content/error.js:455:5\nswitchToShadowRoot@chrome://marionette/content/listener.js:1568:11\ndispatch/</req<@chrome://marionette/content/listener.js:513:22\nTaskImpl_run@resource://gre/modules/Task.jsm:331:42\nTaskImpl@resource://gre/modules/Task.jsm:280:3\nasyncFunction@resource://gre/modules/Task.jsm:252:14\nTask_spawn@resource://gre/modules/Task.jsm:166:12\ndispatch/<@chrome://marionette/content/listener.js:511:15\n"},null]
Comment hidden (Intermittent Failures Robot) |
Comment 4•7 years ago
|
||
This test was recently disabled on stylo - bug 1389153. All failures are on nightly...that seems odd.
See Also: → 1389153
Whiteboard: [stockwell needswork]
Comment 5•7 years ago
|
||
Due to most sheriffs being out for a while we haven't had great starring of failures for a while. I check OF results and went back on TH to check what's the earliest nightly build with this failure is and I got this one: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&tochange=87824406b9fe&filter-searchStr=mn%20nightly&fromchange=556f19ef392ac2d9aac579864e2179d6c1d464e8&selectedJob=119682655 From the changes it's not clear right now what could have caused this. So I triggered a couple of further Mn-e10s jobs for Linux64 nightly which seem to be affected most around this time.
Comment 6•7 years ago
|
||
Looks like low intermittent and it might be better to check what's wrong here. So having a look at the latest failures I see the following: >[task 2017-08-17T23:19:24.144991Z] 23:19:24 INFO - 1503011964141 Marionette DEBUG Received DOM event "beforeunload" for "http://127.0.0.1:33894/file_upload" >[task 2017-08-17T23:19:24.162395Z] 23:19:24 INFO - 1503011964157 Marionette DEBUG Received DOM event "pagehide" for "http://127.0.0.1:33894/file_upload" >[task 2017-08-17T23:19:24.163796Z] 23:19:24 INFO - 1503011964157 Marionette DEBUG Received DOM event "unload" for "http://127.0.0.1:33894/file_upload" >[task 2017-08-17T23:19:24.180538Z] 23:19:24 INFO - JavaScript error: http://127.0.0.1:33894/test_shadow_dom.html, line 18: TypeError: host.createShadowRoot is not a function >[task 2017-08-17T23:19:24.181958Z] 23:19:24 INFO - 1503011964178 Marionette DEBUG Received DOM event "DOMContentLoaded" for "http://127.0.0.1:33894/test_shadow_dom.html" >[task 2017-08-17T23:19:24.191659Z] 23:19:24 INFO - 1503011964185 Marionette DEBUG Received DOM event "pageshow" for "http://127.0.0.1:33894/test_shadow_dom.html" So `createShadowRoot` is not a function here. As MDN states this method is deprecated and should be replaced with `attachShadow`: https://developer.mozilla.org/en-US/docs/Web/API/Element/createShadowRoot It doesn't explain why it's only failing intermittently. But maybe by moving to `attachShadow` we could get rid of this problem? Andreas, what do you think?
Flags: needinfo?(ato)
Comment 7•7 years ago
|
||
Btw. when running this test against a nightly build this seems to always fail now.
Comment 8•7 years ago
|
||
I had a quick look and it is not related to `createShadowRoot` alone. It's also happening for `attachShadow`. It's just that the `div` doesn't have such a method. Does anyone know who is working on shadow DOM and could help out here?
Comment hidden (Intermittent Failures Robot) |
Comment 10•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #8) > I had a quick look and it is not related to `createShadowRoot` alone. It's > also happening for `attachShadow`. It's just that the `div` doesn't have > such a method. > > Does anyone know who is working on shadow DOM and could help out here? Blake, maybe you could help out? This intermittent raised last week to #27 for OF. Thanks.
Flags: needinfo?(mrbkap)
Comment 11•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #6) > So `createShadowRoot` is not a function here. As MDN states this > method is deprecated and should be replaced with `attachShadow`: > > https://developer.mozilla.org/en-US/docs/Web/API/Element/createShadowRoot > > It doesn't explain why it's only failing intermittently. But maybe > by moving to `attachShadow` we could get rid of this problem? It would be plausible to assume that maybe createShadowRoot at some point is turned off only on Nightly in preparation for the move to attachShadow, but this should have triggered a permanent failure with this test. I don’t think moving to attachShadow in and by itself will fix this problem, but I somewhat suspect the real problem here is that the shadow root element has not properly been loaded before the document tries calling it: > error: http://127.0.0.1:33894/test_shadow_dom.html, line 18: TypeError: host.createShadowRoot is not a function > 1503011964178 Marionette DEBUG Received DOM event "DOMContentLoaded" for "http://127.0.0.1:33894/test_shadow_dom.html"
Flags: needinfo?(ato)
Comment 12•7 years ago
|
||
I tried with an onload listener locally but that didn't help neither. So it doesn't seem to be related to the loading state of the document.
Comment 13•7 years ago
|
||
We should just disable these tests until we get back to working on Shadow DOM (bug 1205323).
Flags: needinfo?(mrbkap)
Comment 14•7 years ago
|
||
Ok, I think someone from us can do that at latest on Monday.
Blocks: shadowdom-initial-release
Comment hidden (Intermittent Failures Robot) |
Comment 16•7 years ago
|
||
:whimboo, are you going to handle disabling these tests?
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Flags: needinfo?(hskupin)
Keywords: leave-open
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8901849 [details] Bug 1387678 - Skip all tests in test_shadow_dom.py due to intermittent failures. https://reviewboard.mozilla.org/r/173284/#review178654 thanks
Attachment #8901849 -
Flags: review?(jmaher) → review+
Comment 19•7 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f30ab4ac7635 Skip all tests in test_shadow_dom.py due to intermittent failures. r=jmaher
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f30ab4ac7635
Updated•7 years ago
|
status-firefox57:
--- → disabled
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 23•7 years ago
|
||
Please land the skip patch for the test also on beta. Thanks.
status-firefox56:
--- → affected
Whiteboard: [stockwell unknown] → [stockwell unknown][checkin-needed-beta]
Comment 24•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/569c9d764b27
Whiteboard: [stockwell unknown][checkin-needed-beta] → [stockwell unknown]
Comment 25•6 years ago
|
||
Do we need to do anything here to ship Shadow DOM v1?
Flags: needinfo?(dburns)
Assignee | ||
Comment 26•6 years ago
|
||
I have seen and will get to it today
Comment hidden (mozreview-request) |
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8964694 [details] Bug 1387678 - Reenable Shadow DOM tests https://reviewboard.mozilla.org/r/233390/#review238972 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: testing/marionette/element.js:1218 (Diff revision 1) > let centre = element.getInViewCentrePoint(rects[0], win); > > // step 5 > + if (rootNode == doc) { > - return doc.elementsFromPoint(centre.x, centre.y); > + return doc.elementsFromPoint(centre.x, centre.y); > + } else { Error: Unnecessary 'else' after 'return'. [eslint: no-else-return] ::: testing/marionette/element.js:1219 (Diff revision 1) > > // step 5 > + if (rootNode == doc) { > - return doc.elementsFromPoint(centre.x, centre.y); > + return doc.elementsFromPoint(centre.x, centre.y); > + } else { > + return rootNode.elementsFromPoint(centre.x, centre.y) Error: Missing semicolon. [eslint: semi]
Comment hidden (mozreview-request) |
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8964694 [details] Bug 1387678 - Reenable Shadow DOM tests https://reviewboard.mozilla.org/r/233390/#review238980 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: testing/marionette/element.js:1192 (Diff revision 2) > * Sequence of elements in paint order. > */ > element.getPointerInteractablePaintTree = function(el) { > const doc = el.ownerDocument; > const win = doc.defaultView; > const container = {frame: win}; Error: 'container' is assigned a value but never used. [eslint: no-unused-vars]
Comment 31•6 years ago
|
||
Is this just waiting for code style updates for the patch?
Comment 32•6 years ago
|
||
ok, the test was changed when createShadowRoot was removed. I'll push the changes to try.
Comment 33•6 years ago
|
||
I tried the patch (without that pdb stuff) and got https://pastebin.mozilla.org/9084876 I know nothing about marionette, so I'd prefer someone else to take a look.
Comment 34•6 years ago
|
||
The problem here is in the test which calls `self.marionette.switch_to_shadow_root()` without specifying the id of the web element, which is required. It also doesn't fail accurately because the assert has been removed. David, can you please fix that?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8964694 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 ::: testing/marionette/driver.js:2581 (Diff revision 2) > * @param {string} id > * Reference ID to the element. Update API docs with "string=" to indicate that the input value can be null. ::: testing/marionette/driver.js:2593 (Diff revision 2) > - let id = assert.string(cmd.parameters.id); > - let webEl = WebElement.fromUUID(id, this.context); > + let id = cmd.parameters.id; > + let webEl = id ? WebElement.fromUUID(id, this.context) : null; > await this.listener.switchToShadowRoot(webEl); Maybe id can also be removed now that we’re not manipulating checking the input value.
Attachment #8974329 -
Flags: review?(ato) → review+
Comment 52•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > Maybe id can also be removed now that we’re not manipulating checking > the input value. If we keep it why don't we assert anymore for a valid value? Please note that passing in null into switchToShadowRoot will cause an exception as mentioned in an earlier comment on this bug.
Comment 53•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > If we keep it why don't we assert anymore for a valid value? Please note that passing in null into switchToShadowRoot will cause an exception as mentioned in an earlier comment on this bug. You’re right, we still need to check that the value is either explicitly null, undefined, or a string. I’ve raised a separate issue for this.
Comment 54•6 years ago
|
||
mozreview-review |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248956 ::: testing/marionette/driver.js:2593 (Diff revision 2) > - let id = assert.string(cmd.parameters.id); > - let webEl = WebElement.fromUUID(id, this.context); > + let id = cmd.parameters.id; > + let webEl = id ? WebElement.fromUUID(id, this.context) : null; Permitted types for cmd.parameters.id is null, undefined, and string. We should still assert that we get the correct input type.
Attachment #8974329 -
Flags: review+ → review-
Comment 55•6 years ago
|
||
mozreview-review |
Comment on attachment 8974334 [details] Bug 1387678 - Correct import of pprint in Marionette listener; https://reviewboard.mozilla.org/r/242672/#review248958 Well spotted.
Attachment #8974334 -
Flags: review?(ato) → review+
Comment 56•6 years ago
|
||
mozreview-review |
Comment on attachment 8974335 [details] Bug 1387678 - Check if element is passed in is actually an element; https://reviewboard.mozilla.org/r/242674/#review248960
Attachment #8974335 -
Flags: review?(ato) → review+
Comment 57•6 years ago
|
||
mozreview-review |
Comment on attachment 8974330 [details] Bug 1387678 - Remove unused reference to container variable; https://reviewboard.mozilla.org/r/242664/#review248884 ::: commit-message-289ea:1 (Diff revision 2) > +Bug 1387678 - Remove code incorrectly trying to set ShadowDOM root; r?ato s/ShadowDOM/shadow DOM/ ::: testing/marionette/element.js:1189 (Diff revision 2) > element.getPointerInteractablePaintTree = function(el) { > const doc = el.ownerDocument; > const win = doc.defaultView; > const container = {frame: win}; > const rootNode = el.getRootNode(); > > - // Include shadow DOM host only if the element's root node is not the > - // owner document. > - if (rootNode !== doc) { > - container.shadowRoot = rootNode; > - } You’re right, but in fact I can’t see that container is used at all in this function, so it can probably be removed altogether.
Attachment #8974330 -
Flags: review?(ato) → review-
Comment 58•6 years ago
|
||
mozreview-review |
Comment on attachment 8974331 [details] Bug 1387678 - Call elementsFromPoint on the correct document node; https://reviewboard.mozilla.org/r/242666/#review248970 ::: testing/marionette/element.js:1192 (Diff revision 2) > */ > element.getPointerInteractablePaintTree = function(el) { > const doc = el.ownerDocument; > const win = doc.defaultView; > - const container = {frame: win}; > const rootNode = el.getRootNode(); Wouldn’t it be correct to _always_ use rootnOde for elementsFromPoint? ::: testing/marionette/element.js:1209 (Diff revision 2) > + if (rootNode == doc) { > - return doc.elementsFromPoint(centre.x, centre.y); > + return doc.elementsFromPoint(centre.x, centre.y); > + } > + return rootNode.elementsFromPoint(centre.x, centre.y); Instead of using an if-condition here, set the value of rootNode to the appropriate element/document element at the start of the function.
Attachment #8974331 -
Flags: review?(ato) → review-
Comment 59•6 years ago
|
||
mozreview-review |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review248972 ::: testing/marionette/driver.js:327 (Diff revision 2) > + if (!payload) { > + payload = {}; > + } We already support null as return value, but not if you need to set it explicitly on resp.body.value.
Attachment #8974332 -
Flags: review?(ato) → review-
Comment 60•6 years ago
|
||
mozreview-review |
Comment on attachment 8974333 [details] Bug 1387678 - Update tests to not use shadow DOM like a frame; https://reviewboard.mozilla.org/r/242670/#review249128 ::: commit-message-fd1f8:3 (Diff revision 2) > +The current element finding and interaction code expects the shadow DOM > +to act like a frame when it comes to keeping references to elements found > +or not found on the document. This expectation is not correct for the way > +shadow DOM works in reality. Why is this not correct?
Attachment #8974333 -
Flags: review?(ato) → review-
Comment 61•6 years ago
|
||
mozreview-review |
Comment on attachment 8974328 [details] Bug 1387678 - Reenable Shadow DOM tests; https://reviewboard.mozilla.org/r/242660/#review249130
Attachment #8974328 -
Flags: review?(ato) → review+
Assignee | ||
Comment 62•6 years ago
|
||
mozreview-review |
Comment on attachment 8974333 [details] Bug 1387678 - Update tests to not use shadow DOM like a frame; https://reviewboard.mozilla.org/r/242670/#review249182 ::: commit-message-fd1f8:3 (Diff revision 2) > +The current element finding and interaction code expects the shadow DOM > +to act like a frame when it comes to keeping references to elements found > +or not found on the document. This expectation is not correct for the way > +shadow DOM works in reality. Since the way we store elements is stored to frame and not the document, perhaps this will change when we move to the new window tracking code?
Assignee | ||
Comment 63•6 years ago
|
||
mozreview-review |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review249184 ::: testing/marionette/driver.js:327 (Diff revision 2) > + if (!payload) { > + payload = {}; > + } How do you suggest I fix this? If I leave it as the current code then we get a situation where it fails to send to the listener because the payload is null.
Assignee | ||
Comment 64•6 years ago
|
||
mozreview-review |
Comment on attachment 8974331 [details] Bug 1387678 - Call elementsFromPoint on the correct document node; https://reviewboard.mozilla.org/r/242666/#review249186 ::: testing/marionette/element.js:1192 (Diff revision 2) > */ > element.getPointerInteractablePaintTree = function(el) { > const doc = el.ownerDocument; > const win = doc.defaultView; > - const container = {frame: win}; > const rootNode = el.getRootNode(); pretty much :)
Assignee | ||
Comment 65•6 years ago
|
||
mozreview-review |
Comment on attachment 8974330 [details] Bug 1387678 - Remove unused reference to container variable; https://reviewboard.mozilla.org/r/242664/#review249188 ::: testing/marionette/element.js:1189 (Diff revision 2) > element.getPointerInteractablePaintTree = function(el) { > const doc = el.ownerDocument; > const win = doc.defaultView; > const container = {frame: win}; > const rootNode = el.getRootNode(); > > - // Include shadow DOM host only if the element's root node is not the > - // owner document. > - if (rootNode !== doc) { > - container.shadowRoot = rootNode; > - } I need to move that from another commit... leave it with me :)
Assignee | ||
Comment 66•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > You’re right, we still need to check that the value is either > explicitly null, undefined, or a string. I’ve raised a separate > issue for this. The issue in comment 2 has been fixed in this patch series. Part of the problem is asserting on string is that it null is not a string. In `switchToFrame` we don't do any asserting of what is coming in, we then do it in a conditional later and do the relevant thing when we hit a `true`
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dburns
Flags: needinfo?(dburns)
Assignee | ||
Comment 75•6 years ago
|
||
In the comments above there are some questions (which are in mozreview), could you answer and I will finalise this patch series
Flags: needinfo?(ato)
Comment 76•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > Update API docs with "string=" to indicate that the input value can > be null. Dropping this issue since you continued the discussion on the other issue, which was actually about whether the id variable could be removed. > The issue in comment 2 has been fixed in this patch series. Part of the problem is asserting on string is that it null is not a string. > > In `switchToFrame` we don't do any asserting of what is coming in, we then do it in a conditional later and do the relevant thing when we hit a `true` It’s not harder than this: > if (typeof id != "undefined" || id !== null) { > assert.string(id); > } You could even rely on double-equals which matches both null and undefined: > if (id == null) { > assert.string(id); > } However, I looked at WebElement.fromUUID and it does an assert.string internally. However, id could still be a boolean in which case the ternary would evaluate to null. That’s the case you need to fix here.
Comment 77•6 years ago
|
||
mozreview-review |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review250308
Attachment #8974329 -
Flags: review?(ato) → review-
Comment 78•6 years ago
|
||
mozreview-review |
Comment on attachment 8974330 [details] Bug 1387678 - Remove unused reference to container variable; https://reviewboard.mozilla.org/r/242664/#review250310 ::: commit-message-289ea:1 (Diff revision 3) > +Bug 1387678 - Remove code incorrectly trying to set shadow DOM root; r?ato > + > +The shadowRoot property is a getter and not a setter so this code is > +essentially doing nothing and can be removed. This commit message seems wrong. What you want to say here is that container isn’t used so it can be removed without consequence.
Attachment #8974330 -
Flags: review?(ato) → review-
Comment 79•6 years ago
|
||
mozreview-review |
Comment on attachment 8974331 [details] Bug 1387678 - Call elementsFromPoint on the correct document node; https://reviewboard.mozilla.org/r/242666/#review250312
Attachment #8974331 -
Flags: review?(ato) → review+
Comment 80•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review249184 > How do you suggest I fix this? If I leave it as the current code then we get a situation where it fails to send to the listener because the payload is null. I suppose you are correct to coerce this into a dictionary here.
Comment 81•6 years ago
|
||
mozreview-review |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review250318 ::: commit-message-f3019:1 (Diff revision 3) > +Bug 1387678: Handle null being returned as a payload; r?ato Your other commits uses dashes in the commit message, this uses a colon. ::: commit-message-f3019:1 (Diff revision 3) > +Bug 1387678: Handle null being returned as a payload; r?ato This patch doesn’t deal with the return value, but with the parameters sent alongside the message to the frame script? ::: testing/marionette/driver.js:322 (Diff revision 3) > * @throws {JavaScriptError} > * If <var>data</var> could not be marshaled. > * @throws {NoSuchWindowError} > * If there is no current target frame. > */ > GeckoDriver.prototype.sendAsync = function(name, data, commandID) { Set default value of commandID to null. ::: testing/marionette/driver.js:325 (Diff revision 3) > + if (!payload) { > + payload = {}; > + } Set this as a default value on data in the arguments list.
Attachment #8974332 -
Flags: review?(ato) → review-
Comment 82•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974333 [details] Bug 1387678 - Update tests to not use shadow DOM like a frame; https://reviewboard.mozilla.org/r/242670/#review249182 > Since the way we store elements is stored to frame and not the document, perhaps this will change when we move to the new window tracking code? I may have expressed myself poorly in my question. I have limited knowledge of shadow DOM. Are element references located inside the shadow DOM visible to outside document’s DOM? How we choose to handle this in Marionette is purley fiction, as there is no specification to follow in this case. WebDriver does not talk about how to handle shadow DOM, and generally I would be more comfortable reviewing this patch if it did. But considering we already have code to handle shadow DOM in Marionette, unbreaking that is the priority. How Marionette handles shadow DOM is an approximation to the idea that an element inside a shadow DOM is not exposed to the ouside DOM. Should WebDriver not care for this?
Comment 83•6 years ago
|
||
mozreview-review |
Comment on attachment 8974333 [details] Bug 1387678 - Update tests to not use shadow DOM like a frame; https://reviewboard.mozilla.org/r/242670/#review250328
Attachment #8974333 -
Flags: review?(ato) → review-
Updated•6 years ago
|
Flags: needinfo?(ato)
Comment 84•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > It’s not harder than this: > > > if (typeof id != "undefined" || id !== null) { > > assert.string(id); > > } > > You could even rely on double-equals which matches both null and > undefined: > > > if (id == null) { > > assert.string(id); > > } > > However, I looked at WebElement.fromUUID and it does an assert.string > internally. However, id could still be a boolean in which case the > ternary would evaluate to null. That’s the case you need to fix > here. But it's not allowed to pass `null` as element to `switchToShadowRoot()` right? So why would we accept `null` as element reference?
Assignee | ||
Comment 85•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > But it's not allowed to pass `null` as element to `switchToShadowRoot()` right? So why would we accept `null` as element reference? Passing in `null` is used to make sure we move to the correct document. `null` makes sure we use the shadow root's host's root node instead of the shadow dom. ``` let elementWithShadowRoot = document.querySelector(...); let shadowRootOfElement = elementWithShadowRoot.shadowRoot; /* To search for elements in the shadow DOM we need to substitute * document with shadowRootOfElement */ let someElement = shadowRootOfElement.querySelector(...); elementWithShadowRoot == shadowRootOfElement.host; //or elementWithShadowRoot == someElement.getRootNode(); elementWithShadowRoot.getRootNode() == document; ```
Comment 86•6 years ago
|
||
mozreview-review |
Comment on attachment 8974328 [details] Bug 1387678 - Reenable Shadow DOM tests; https://reviewboard.mozilla.org/r/242660/#review250700 ::: testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini (Diff revision 3) > [test_prefs.py] > [test_prefs_enforce.py] > skip-if = manage_instance == false || appname == 'fennec' # Bug 1298921 > > [test_shadow_dom.py] > -skip-if = true # Bug 1293844, bug 1387678 Shouldn't this be the last commit in this series to not produce broken commits because tests are failing?
Comment 87•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > Passing in `null` is used to make sure we move to the correct document. `null` makes sure we use the shadow root's host's root node instead of the shadow dom. > > ``` > let elementWithShadowRoot = document.querySelector(...); > let shadowRootOfElement = elementWithShadowRoot.shadowRoot; > /* To search for elements in the shadow DOM we need to substitute > * document with shadowRootOfElement > */ > let someElement = shadowRootOfElement.querySelector(...); > > elementWithShadowRoot == shadowRootOfElement.host; > //or > elementWithShadowRoot == someElement.getRootNode(); > > elementWithShadowRoot.getRootNode() == document; > ``` Hm, I remembered a try push from Olli which failed because `null` was being passed-in. Maybe the relevant code has been changed. > if (id == null) { > assert.string(id); > } How can `null` be asserted for a string? This should always fail, right? An `if (id)` should work here.
Assignee | ||
Comment 88•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > Hm, I remembered a try push from Olli which failed because `null` was being passed-in. Maybe the relevant code has been changed. > > > if (id == null) { > > assert.string(id); > > } > > How can `null` be asserted for a string? This should always fail, right? An `if (id)` should work here. I had assumed it was a typo when comparing to the part of the comment above. As :ato mentions `if (id)` doesnt protect us from `id` being a boolean.
Comment 89•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > I had assumed it was a typo when comparing to the part of the comment above. As :ato mentions `if (id)` doesnt protect us from `id` being a boolean. Oh, right. So `if (id != null)` then, which matches `null` and `undefined`.
Comment 90•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review248882 > Oh, right. So `if (id != null)` then, which matches `null` and `undefined`. I blame JavaScript for the confusion here. Henrik is correct in his last comment. The edge case we’re trying to protect ourselves from is if id is a false boolean or some other value that evaluates to false.
Assignee | ||
Comment 91•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review250318 > Set this as a default value on data in the arguments list. This doesn't work since the issue is we can't seem to proxy a null across the wire. It appears when we hit the promise in https://searchfox.org/mozilla-central/source/testing/marionette/proxy.js#119-140 it breaks. Looking at https://searchfox.org/mozilla-central/source/testing/marionette/evaluate.js#178 we don't handle `null` but not sure if we want to handle `null` there. What is the preference?
Assignee | ||
Comment 92•6 years ago
|
||
(In reply to Andreas Tolfsen 「:ato」 from comment #82) > Comment on attachment 8974333 [details] > Bug 1387678 - Update tests to not use shadow DOM like a frame; > > https://reviewboard.mozilla.org/r/242670/#review249182 > > > Since the way we store elements is stored to frame and not the document, perhaps this will change when we move to the new window tracking code? > > I may have expressed myself poorly in my question. I have limited > knowledge of shadow DOM. Are element references located inside the > shadow DOM visible to outside document’s DOM? > > How we choose to handle this in Marionette is purley fiction, as > there is no specification to follow in this case. WebDriver does > not talk about how to handle shadow DOM, and generally I would be > more comfortable reviewing this patch if it did. But considering > we already have code to handle shadow DOM in Marionette, unbreaking > that is the priority. > > How Marionette handles shadow DOM is an approximation to the idea > that an element inside a shadow DOM is not exposed to the ouside > DOM. Should WebDriver not care for this? I think at least for this iteration it should not care but I will take a stab at writing this part of the spec up and then see what people think and we can then correct from there. I don't think, at least for now, it should block this review.
Assignee | ||
Comment 93•6 years ago
|
||
mozreview-review |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review252114 ::: commit-message-f3019:1 (Diff revision 3) > +Bug 1387678: Handle null being returned as a payload; r?ato Reply to this is in the other issue so dropping this issue for now and will correct the commit based on what we decide.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 102•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review250318 > This doesn't work since the issue is we can't seem to proxy a null across the wire. It appears when we hit the promise in https://searchfox.org/mozilla-central/source/testing/marionette/proxy.js#119-140 it breaks. Looking at https://searchfox.org/mozilla-central/source/testing/marionette/evaluate.js#178 we don't handle `null` but not sure if we want to handle `null` there. What is the preference? It is true that we can’t send null as the payload of an IPC message, but what does this got to do with null? The default value is {}, which should be possible to set as a default argument and which is possible to send in a message.
Comment 103•6 years ago
|
||
mozreview-review |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review252856 ::: testing/marionette/driver.js:2576 (Diff revision 4) > /** > * Switch to shadow root of the given host element. > * > * @param {string} id > * Reference ID to the element. > * > * @throws {InvalidArgumentError} > * If <var>id</var> is not a string. > * @throws {NoSuchElementError} > * If element represented by reference <var>id</var> is unknown. > */ The API documentation is now not correct.
Attachment #8974329 -
Flags: review?(ato) → review-
Comment 104•6 years ago
|
||
mozreview-review |
Comment on attachment 8974330 [details] Bug 1387678 - Remove unused reference to container variable; https://reviewboard.mozilla.org/r/242664/#review252858
Attachment #8974330 -
Flags: review?(ato) → review+
Comment 105•6 years ago
|
||
mozreview-review |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review252860
Attachment #8974332 -
Flags: review?(ato) → review-
Comment 106•6 years ago
|
||
(In reply to David Burns :automatedtester from comment #92) > I think at least for this iteration it should not care but I will take a > stab at writing this part of the spec up and then see what people think and > we can then correct from there. I don't think, at least for now, it should > block this review. So my problem here is that the patch only removes two tests, which means one of your other patches in this changeset that unbreaks the shadow DOM support doesn’t take into account checking if the web element is stale or not. Presumably we had code in the past to check this. The mitigating reasoning is of course that the tests have been disabled for quite a while and that shadow DOM support in Marionette has been broken. I guess one can make a reasonable argument that this change then does not cause backwards incompatible behaviour, but it would if the shadow DOM support was not broken from earlier.
Comment 107•6 years ago
|
||
mozreview-review |
Comment on attachment 8974333 [details] Bug 1387678 - Update tests to not use shadow DOM like a frame; https://reviewboard.mozilla.org/r/242670/#review252862
Attachment #8974333 -
Flags: review?(ato) → review+
Assignee | ||
Comment 108•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review250318 > It is true that we can’t send null as the payload of an IPC message, > but what does this got to do with null? The default value is {}, > which should be possible to set as a default argument and which is > possible to send in a message. Thats not true that it returns `{}`. In https://searchfox.org/mozilla-central/source/testing/marionette/evaluate.js#187-188 if it's given `null` it returns `null` which is the reason for this change. We are in a place with a `commandID` AND we have `null` which is why setting a default value is not going to helpful
Comment 109•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review250318 > Thats not true that it returns `{}`. In https://searchfox.org/mozilla-central/source/testing/marionette/evaluate.js#187-188 if it's given `null` it returns `null` which is the reason for this change. We are in a place with a `commandID` AND we have `null` which is why setting a default value is not going to helpful I wasn’t saying that evaluate.toJSON returns {} when given null. That wouldn’t make any sense. At this point it is not clear to me what you are fixing. The commit message “Handle null being returned as a payload” doesn’t give any clues.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 118•6 years ago
|
||
mozreview-review |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review253550 ::: commit-message-f3019:1 (Diff revision 5) > +Bug 1387678 - Handle null being returned from evaluating an object; r?ato If I understand this correctly, what you’re trying to fix here is to allow sendAsync to take null and undefined values as input. evaluate.toJSON only happens to be used internally to ensure the payload is serialisable so I feel the commit message should be updated to say that this allows sendAsync to take null and undefined. ::: commit-message-f3019:3 (Diff revision 5) > +When evaluating if an object it can be null which would mean that we would > +not be able to pass through IPC as the commandId could not be added to null. > +This patch makes sure that we can still commands via IPC if the object is > +evaluated to null. A couple of grammer mistakes here. ::: testing/marionette/driver.js:325 (Diff revision 5) > + if (!payload) { > + payload = {}; > + } This will also match any value that evaluates to false, for example falsey values such as undefined (which I don’t think evaluate.toJSON will return) and false. Wouldn’t "if (payload === null)" be safer?
Attachment #8974332 -
Flags: review?(ato) → review+
Comment 119•6 years ago
|
||
mozreview-review |
Comment on attachment 8974329 [details] Bug 1387678 - Allow traversing to the parent if switchToShadowDom is given null; https://reviewboard.mozilla.org/r/242662/#review253560
Attachment #8974329 -
Flags: review?(ato) → review+
Assignee | ||
Comment 120•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review253550 > This will also match any value that evaluates to false, for example > falsey values such as undefined (which I don’t think evaluate.toJSON > will return) and false. > > Wouldn’t "if (payload === null)" be safer? If undefined does make it through we are going to have the same issue. The point is to make the code more defensive than it already is and only going for null seems suboptimal.
Comment 121•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974332 [details] Bug 1387678 - Allow Marionette sendAsync command to handle null or undefinedr?ato https://reviewboard.mozilla.org/r/242668/#review253550 > If undefined does make it through we are going to have the same issue. The point is to make the code more defensive than it already is and only going for null seems suboptimal. The purpose of evaluate.toJSON is to convert JS types into safe JSON types. If you look at the implementation of evaluate.toJSON, undefined gets converted to null. I don’t find it more defensive to be _less specific_ about typing in this case.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 130•6 years ago
|
||
Pushed by dburns@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2f8be50be8e1 Allow traversing to the parent if switchToShadowDom is given null; r=ato https://hg.mozilla.org/integration/autoland/rev/34634db1a311 Remove unused reference to container variable; r=ato https://hg.mozilla.org/integration/autoland/rev/022c470e1af0 Call elementsFromPoint on the correct document node; r=ato https://hg.mozilla.org/integration/autoland/rev/767a56006317 Allow Marionette sendAsync command to handle null or undefinedr?ato r=ato https://hg.mozilla.org/integration/autoland/rev/3c9c76ca14ad Update tests to not use shadow DOM like a frame; r=ato https://hg.mozilla.org/integration/autoland/rev/20cdbb614683 Correct import of pprint in Marionette listener; r=ato https://hg.mozilla.org/integration/autoland/rev/23af3771178d Check if element is passed in is actually an element; r=ato https://hg.mozilla.org/integration/autoland/rev/938be426f7d9 Reenable Shadow DOM tests; r=ato
Comment 131•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2f8be50be8e1 https://hg.mozilla.org/mozilla-central/rev/34634db1a311 https://hg.mozilla.org/mozilla-central/rev/022c470e1af0 https://hg.mozilla.org/mozilla-central/rev/767a56006317 https://hg.mozilla.org/mozilla-central/rev/3c9c76ca14ad https://hg.mozilla.org/mozilla-central/rev/20cdbb614683 https://hg.mozilla.org/mozilla-central/rev/23af3771178d https://hg.mozilla.org/mozilla-central/rev/938be426f7d9
Comment 132•6 years ago
|
||
leave-open has been set a long time ago to keep this bug open due to the bulk closure for incomplete. Now this is fixed and no longer needed.
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Keywords: leave-open
Priority: P5 → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 133•1 year ago
|
||
Moving bug to Testing::Marionette Client and Harness component per bug 1815831.
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•