Make `list of known elements` per Window, not per browsing context

[whimboo]

As discussed with @jgraham the support for stale element reference in WebDriver HTTP should be considered to be removed. There is basically no strong reason why no such element cannot handle such situations eg. after a navigation. For WebDriver BiDi it looks like not be used at all.

We are aware that the removal of this error type is backward incompatible. As such we would like to get a discussion started to know:

• if the removal as suggested is acceptable for everyone
• how often this error type might actually be used in the wild
• The strategy to get it removed

Maybe it can be discussed in the next monthly meeting.

CC'ing @AutomatedTester, @shs96c, @JohnChen0, @burg, @christian-bromann

[jgraham]

Some background: We current have to produce the Stale Element Reference error if an element is in a different document, or if it's not connected. But that's annoying to implement. It means that you can't just have a per-window weak map between element ids and elements, because the spec requires you to figure out that the reference is valid for a DOM node that's in another document, or for a DOM node that's been removed from the DOM (and might only be kept alive by the List of Known Elements in WebDriver).

Particuarly with heavily multiprocess architectures in modern browsers, the stale element reference distinction alone requires an implementation to keep at least a list of all known element references in the main process. Without this implementations could be significantly simplified. It seems unlikely that users are strongly depending on the distinction between no such element and stale element reference, so it seems reasonable to just always return no such element. If we can't change HTTP WebDriver here, I at least plan to make BiDi not have this distinction and force HTTP-based WebDriver to do the bookkeeping on which element references have ben used.

[AutomatedTester]

We don't need to keep a map of all element references, just ones that have been previously searched for in the past. This is why it is described as get known elements. Historically, Marionette used to have this at a frame level which should work with multiprocess architectures, assuming that each frame gets a new process.

How are we we planning on handling the situation where someone has a reference to a known element and it is no longer attached to the DOM? Are we going to say the element you found a while back is now NoSuchElement?

[jgraham]

Hmm, so I re-read the relevant spec bits, and I think maybe we'd misinterpreted the scope of the staleness checks. As long as is stale is always called after you've already failed with no such element for elements from a different browsing context then it's probably true that the cache can be per-process. But the spec here is a real mess; as far as I can tell it's always wrong to check for staleness except in "get a known element", and I'm not seeing an observable difference between "get a known element" and "get a known connected element".

In any case the staleness check at least requires that you keep a record of all the element ids you ever handed out for a specific browsing context (assuming the intent is that you get stale element reference for an element that has been gc'd; the list of known elements construct at least implies that).

Maybe it would be enough to change the spec to not hold a strong reference to elements, so that if the element is gc'd you get no such element and stale element reference is only for the case that the element is disconnected or the page navigated. Even better it could just be for the disconnected case, so after navigation you always get no such element, even if the browsing context hasn't been discarded. Then the implementation could just be a weakmap of element id to element, and the staleness check would just be checking the document of the element. That seems a lot closer to the way that BiDi is likely to work (given we will have all sorts of remote objects there, not just elements, and we don't want to extend the lifetime of the object ids past the lifetime of the global).

[christian-bromann]

From the framework perspective it really depends if the client keeps information about whether it has fetched an element id before. For example WebdriverIO has an element instance for users to call element commands on, e.g.:

const elem = $('#someId') // fetch element
elem.click()
// arbitrary re-render happened
elem.click() // could be stale element now, but would be refetched if so

This element (elem) instance keeps various information like the original selector to ensure it can be refetched. There is also an implicit check for stale element errors to refetch the element with the same selector.

For implementations that don't have such safe guard it could be more confusing for users who interact with an element first and then suddenly receive a no such element error, particularly given the fact that staleness can sometimes be triggered based of arbitrary re-render events of the underlying frontend framework. So e.g. if we take the webdriver package that WebdriverIO uses for making low level WebDriver requests, it would look different given no safe guards are in place:

const searchInput = await client.findElement('css selector', '#lst-ib')
await client.elementClick(searchInput['element-6066-11e4-a52e-4f735466cecf'])
// arbitrary re-render happened
await client.elementClick(searchInput['element-6066-11e4-a52e-4f735466cecf']) // suddenly "no such element"

Overall I believe the biggest advantage to have a stale element error was to give the devil a name. Such errors are still happening and are reasons for non deterministic test results. For implementations that work with safe guards it probably won't make a difference but for the ones that don't have such functionality such non deterministic behavior will be even harder to explain.

[AutomatedTester]

Hmm, so I re-read the relevant spec bits, and I think maybe we'd misinterpreted the scope of the staleness checks. As long as is stale is always called after you've already failed with no such element for elements from a different browsing context then it's probably true that the cache can be per-process. But the spec here is a real mess; as far as I can tell it's always wrong to check for staleness except in "get a known element", and I'm not seeing an observable difference between "get a known element" and "get a known connected element".

There probably isn't an observable difference.

In any case the staleness check at least requires that you keep a record of all the element ids you ever handed out for a specific browsing context (assuming the intent is that you get stale element reference for an element that has been gc'd; the list of known elements construct at least implies that).

In theory we don't need to have a map in the browser for this. If a user passes us an element ID and we are not aware of it or can't find it on the page we are always going to pass back a StaleElementReferenceException

Maybe it would be enough to change the spec to not hold a strong reference to elements, so that if the element is gc'd you get no such element and stale element reference is only for the case that the element is disconnected or the page navigated. Even better it could just be for the disconnected case, so after navigation you always get no such element, even if the browsing context hasn't been discarded. Then the implementation could just be a weakmap of element id to element, and the staleness check would just be checking the document of the element. That seems a lot closer to the way that BiDi is likely to work (given we will have all sorts of remote objects there, not just elements, and we don't want to extend the lifetime of the object ids past the lifetime of the global).

The thing that I don't want, from my first reply and echo'ed by @christian-bromann, is that we don't have a situation where we pass in an element reference and get "NoSuchElementException" as there is already "reflexive" knowledge here.

[jgraham]

Let's consider some examples. What do people expect in the following scenarios, assuming a page like <div id=test></div>? In the navigation cases, do your answers depend on whether the page is in the bfcache?

elem = webdriver.findElement("#test")
webdriver.executeScript("""
  let elem = document.querySelector("#test");
  window.strongReference = elem;
  elem.parentNode.removeChild(elem);
""");
webdriver.getElementProperty(elem, "id");

elem = webdriver.findElement("#test")
webdriver.executeScript("""
  let elem = document.querySelector("#test");
  elem.parentNode.removeChild(elem);
""");
// Assume a gc has run in the browser
webdriver.getElementProperty(elem, "id");

elem = webdriver.findElement("#test")
webdriver.go("about:blank")
webdriver.back()
webdriver.getElementProperty(elem, "id");

elem = webdriver.findElement("#test")
webdriver.executeScript("""
  let elem = document.querySelector("#test");
  window.strongReference = elem;
  elem.parentNode.removeChild(elem);
""");
webdriver.go("about:blank")
webdriver.back()
webdriver.getElementProperty(elem, "id");

elem = webdriver.findElement("#test")
webdriver.executeScript("""
  let elem = document.querySelector("#test");
  elem.parentNode.removeChild(elem);
""");
// Assume a gc has run in the browser
webdriver.go("about:blank")
webdriver.back()
webdriver.getElementProperty(elem, "id");

elem = webdriver.findElement("#test")
webdriver.executeScript("""
  let elem = document.querySelector("#test");
  elem.parentNode.removeChild(elem);
""");
// Assume a gc has run in the browser
webdriver.go("about:blank")
webdriver.back()
webdriver.setWindow(otherValidWindowHandle)
webdriver.getElementProperty(elem, "id");

[shs96c]

Local ends use this exception to allow them to tell the difference between "the user has just searched for something that doesn't exist" and "this element used to exist, and now it is gone". The two cases are different, and being able to differentiate them is useful.

The thing that I don't want, from my first reply and echo'ed by @christian-bromann, is that we don't have a situation where we pass in an element reference and get "NoSuchElementException" as there is already "reflexive" knowledge here.

+1 to this.

[jgraham]

Sure, although in theory at least that's something that local ends can know themselves (they would keep a set of all the element references that have previously been returned and check if the element reference is a member on failure). Of course there can be advantages in putting this in the driver, but I'm loathe to reintroduce the complexity into BiDi; it might be a thing that's (spec-wise) handled in the HTTP frontend. But, to understand what the complexity is supposed to be it would really help to get answers to the lifetime questions above.

[css-meeting-bot]

The Browser Testing and Tools Working Group just discussed Stale Element Reference.

The full IRC log of that discussion

<AutomatedTester> topic: Stale Element Reference
<AutomatedTester> github: https://github.com//issues/1594
<AutomatedTester> jgraham (IRC): this is a question that is for all webdriver
<AutomatedTester> ... I understand there was a distinction in the past
<simonstewart> q+
<AutomatedTester> ... the question is what is the expected lifetime...
<AutomatedTester> ack simonstewart
<AutomatedTester> simonstewart (IRC): The original plan was we kept a cache in the window global.
<AutomatedTester> ... if you navigate away and come back it would nuke our cache
<AutomatedTester> ... in watir and that they will do a lookup again but selenium doesnt do that and assumes the user knows what is the best wayt to handle this
<AutomatedTester> jgraham (IRC): i think that is good
<AutomatedTester> ... I think we need to read the webdriver spec and make sure it does what you have just said
<AutomatedTester> ... my inclination is to not have something similar in bidi
<simonstewart> q+
<AutomatedTester> ... and that in bidi we have more complex objects that are returned
<AutomatedTester> ... and they can have large amounts of data that may get stored in a cache
<AutomatedTester> ack simonstewart
<AutomatedTester> simonstewart (IRC): I think it gets very interested... in webdriver http we required that they keep it instead of the client
<AutomatedTester> ... as the browser would know the lifecycles better than a client
<AutomatedTester> jgraham (IRC): my concern is if we return a handle and then it is gc'ed and someone tries to use it and it could return a differrent error
<AutomatedTester> simonstewart (IRC): is this due to JS serialisation
<AutomatedTester> jgraham (IRC): maybe... [discusses]
<AutomatedTester> simonstewart (IRC): in webdriver http we essentially wanted something that could be returned that passed going through `json.stringify`
<AutomatedTester> jgraham (IRC): [discusses JS execution and handles]
<AutomatedTester> simonstewart (IRC): the original plan was due to things that lives in the DOM
<AutomatedTester> jgraham (IRC): that leads to the question about what type of references we need to keep
<AutomatedTester> ... one thing that I think is true in http spec but not bidi is...
<AutomatedTester> ... if you disconnect an element from the DOM but it's still alive then we would return staleelement reference
<AutomatedTester> ... but in JS the handle would still be held
<AutomatedTester> ... [missed a few items around usage]
<AutomatedTester> action: Verify Webdriver HTTP spec matches the requirements and clarify where necessary

[titusfortner]

At first I was annoyed by the change in implementation for returning "Not Found" instead of "Stale" when working inside a different frame, because Watir removes the need to explicitly switch between frames. Except it ended up being really easy to manage.

From an implementation standpoint there isn't any difference as to what the error is.

If a user sends a command along with an element reference and it comes back "Not Found" instead of "Stale," we can just rescue it and throw a Stale error to stay backward compatible.

I'm not sure I understand what the reflexive knowledge concern is.

[jgraham]

The general tl;dr of the telecon discussion is that the element reference cache only needs to live as long as the page (from the point of view of modern browser architecture, this means it needs to live in the content/render process, not the UI/parent process). I think this removes the main technical concern from our side, but we need to audit the spec to ensure that it does in fact have the behaviour we want.

[jgraham]

Based on my understanding of where we're at with this issue, I've updated the issue summary.

If we make list of known elements per Window rather than per browsing context, it means that once you navigate away from a page trying to use an element reference you got from that page will give no such element rather than stale element reference. This allows implementations to tie the element id cache to the Window lifetime, and avoids having to store all previous element ids indefinitely, including after navigation. If you go back() to the original page, and it was in the bfcache, the original element references will still work (as before), and any element that's been removed will give stale element reference (as before). If you back() and the page is reloaded (i.e it wasn't in bfcache) you'd now get no such element if you try to reuse an existing reference, whereas per the current spec you'd get stale element reference.

In the case that the navigation creates a new browsing context group (i.e. there's a COOP header set) the old and new behaviours will be the same.

This never affects whether a reference is valid, it only affects the specific exception you get in cases involving navigation, and ensures that the COOP and non-COOP cases always behave identically.