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)

Version 3
defect

Tracking

(firefox56 disabled, firefox57 disabled, firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox56 --- disabled
firefox57 --- disabled
firefox62 --- fixed

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
[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]
This test was recently disabled on stylo - bug 1389153.

All failures are on nightly...that seems odd.
See Also: → 1389153
Whiteboard: [stockwell needswork]
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.
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)
Btw. when running this test against a nightly build this seems to always fail now.
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?
(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)
(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)
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.
We should just disable these tests until we get back to working on Shadow DOM (bug 1205323).
Flags: needinfo?(mrbkap)
Ok, I think someone from us can do that at latest on Monday.
:whimboo, are you going to handle disabling these tests?
Flags: needinfo?(hskupin)
Flags: needinfo?(hskupin)
Keywords: leave-open
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+
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
Please land the skip patch for the test also on beta. Thanks.
Whiteboard: [stockwell unknown] → [stockwell unknown][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/569c9d764b27
Whiteboard: [stockwell unknown][checkin-needed-beta] → [stockwell unknown]
Do we need to do anything here to ship Shadow DOM v1?
Flags: needinfo?(dburns)
I have seen and will get to it today
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 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]
Is this just waiting for code style updates for the patch?
ok, the test was changed when createShadowRoot was removed. I'll push the changes to try.
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.
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?
Attachment #8964694 - Attachment is obsolete: true
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 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 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 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 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 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 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 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 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 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 on attachment 8974328 [details]
Bug 1387678 - Reenable Shadow DOM tests;

https://reviewboard.mozilla.org/r/242660/#review249130
Attachment #8974328 - Flags: review?(ato) → 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?
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.
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 :)
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 :)
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`
Assignee: nobody → dburns
Flags: needinfo?(dburns)
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 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 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 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 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 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 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 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 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-
Flags: needinfo?(ato)
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?
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 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 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.
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 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 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.
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?
(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.
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 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 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 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 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-
(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 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+
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 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 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 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+
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 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.
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
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
Keywords: leave-open
Priority: P5 → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Product: Testing → Remote Protocol
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.