1793920 - Don't serialize HTMLDocument as a WebElement reference

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Whenever a WebDriver command is executed which should return a reference to the HTMLDocument it is failing with a stale element reference error. The simplest example is:

self.marionette.execute_script("return document")

That happens because our element.isStale() check has a flaw:

https://searchfox.org/mozilla-central/rev/b0e929cfaf3994cad2dd02afdac138083ee3fc84/remote/marionette/element.sys.mjs#742-750

If el is a document then el.ownerDocument is null and we consider the element as stale. Instead of comparing the documents we should check that the element's ownerGlobal is equal to the win reference if given. Then the expected el.connected state will be returned instead.

A change here might affect a couple of currently failing tests and will make them pass.

I would like to see this bug fixed first before continuing my work on bug 1692468.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1793920 - [wdspec] Don't serialize HTMLDocument as a WebElement reference.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As James mentioned to me a HTMLDocument should actually not be a WebElement because the WebDriver spec only allows Node's of type 1 to be handled as such a reference. Means when returning a document it should be handled as a basic object.

I checked the code and there is actually a bug in our isDOMElement() helper:
https://searchfox.org/mozilla-central/rev/80e1bfa13c954e6450a13b3cc802d82773eba2e0/remote/marionette/element.sys.mjs#1312

This code ([ELEMENT_NODE, DOCUMENT_NODE].includes(obj.nodeType)) allows both an Element and a document to be a WebElement.

Removing this we end-up in a cyclic object value and no longer raise a stale element reference when trying to return a document via Execute Script.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that there is the following WebDriver classic spec issue that we are waiting for a consensus first:
https://github.com/w3c/webdriver/issues/1687

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/816cfcb6b81d
[wdspec] Don't serialize HTMLDocument as a WebElement reference. r=webdriver-reviewers,jgraham

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36619 for changes under testing/web-platform/tests

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/816cfcb6b81d

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We should check with consumers of Webdriver if this might cause issues once the change hits beta/release. If that's the case we will have to make modifications to the spec. See the referenced GitHub issue for that.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So far we didn't receive any feedback or regression reports. So I assume that we are fine here.