Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Associate known elements with a window rather than browsing context #1667

Closed
wants to merge 1 commit into from

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented Jun 14, 2022

This makes the lifecycle of this element list clearer; instead of
having to keep element references alive as long as the browsing
context, you only need to keep it alive as long as the underlying
window. This also makes the behaviour more consistent; before a
navigation might create a new browsing context depending on e.g. COOP
headers, or might not. But it will always create a new Window object.

Practically the difference to users is only in the kind of error you
get in various situations. In particular if you get an element
reference E from page A then navigate A's browsing context to B, and
then try to use E, you will now get "no such element", rather than
"stale element reference". If you traverse history back to A, and it
was in the bfcache, E will be usable again. If it was not in the
bfcache, A is reloaded, and so again using E will give "no such
element". This is exactly consistent with the behaviour if you switch
to another frame or window and try to use E.

You still get a "stale element reference" error in the case that no
navigation happened but the element referenced by E was removed from
the DOM.


Preview | Diff

This makes the lifecycle of this element list clearer; instead of
having to keep element references alive as long as the browsing
context, you only need to keep it alive as long as the underlying
window. This also makes the behaviour more consistent; before a
navigation might create a new browsing context depending on e.g. COOP
headers, or might not. But it will always create a new Window object.

Practically the difference to users is only in the kind of error you
get in various situations. In particular if you get an element
reference E from page A then navigate A's browsing context to B, and
then try to use E, you will now get "no such element", rather than
"stale element reference". If you traverse history back to A, and it
was in the bfcache, E will be usable again. If it was not in the
bfcache, A is reloaded, and so again using E will give "no such
element". This is exactly consistent with the behaviour if you switch
to another frame or window and try to use E.

You still get a "stale element reference" error in the case that no
navigation happened but the element referenced by E was removed from
the DOM.
@whimboo
Copy link
Contributor

whimboo commented Jun 14, 2022

This PR is going to fix issue #1594.

@whimboo
Copy link
Contributor

whimboo commented Jun 16, 2022

FYI I'm going to add no such element tests for all the WebDriver classic commands over on https://bugzilla.mozilla.org/show_bug.cgi?id=1774182. Right now we basically have only a few tests checking that step.

Also with https://bugzilla.mozilla.org/show_bug.cgi?id=1772484 I've already added stale element reference error tests for all the classic commands which should be merged soon (see this PR on the wpt repository).

index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
@whimboo
Copy link
Contributor

whimboo commented Jul 5, 2022

@AutomatedTester, @shs96c, @titusfortner, @christian-bromann - given that you all commented on issue #1594 maybe you could have a look at this pull request and let us know if you would agree with those changes?

We would like to finally discuss this in the next monthly WebDriver meeting to unblock the work for BiDi. Thanks.

CC @sadym-chromium and @foolip to get a position from Google as well.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Associate known elements with a window rather than browsing context.

The full IRC log of that discussion <jgraham> topic: Associate known elements with a window rather than browsing context
<jgraham> github: https://github.com//pull/1667
<simonstewart> jgraham: this applies to both webdriver classic and bidi
<jgraham> https://github.com//issues/1594
<simonstewart> jgraham: original question was whether we had a per-window or per-browsing context list of elements
<simonstewart> jgraham: conclusion was that it was probably okay to have a per-window list, which is easier to implement
<simonstewart> jgraham: result would be that stale element exceptions are no longer returned
<simonstewart> jgraham: there is a bigger question here: what happens if you shuffle elements between windows (eg. return an iframe's parent's body from two different iframes with the same parent)
<simonstewart> jgraham: unfortunately, the answer to this is that none of this should work, because the elements were found from different browsing contexts
<simonstewart> jgraham: probably not what users expect or implementations do
<simonstewart> jgraham: firefox always gives the same element ID. chromedriver sometimes gives the same element ID.
<jgraham> q?
<whimboo> q+
<jgraham> ack whimboo
<simonstewart> q+
<sadym> q+
<simonstewart> whimboo: if we have this per-window, it would be consistent whether the frames were in the same process or not.
<jgraham> https://github.com/web-platform-tests/wpt/pull/34834 is a draft PR with some relevant tests.
<jgraham> ack simonstewart
<jgraham> simonstewart: The question is what are people going to do with the element ids once they've got them? In Selenium, people just do a find element or similar. They don't compare element ids directly. We don't have issues reported on this. I'm not sure that use case is important. From a practical point of view this might not matter much.
<jgraham> ack sadym
<simonstewart> sadym: I remember using Selenium and always having problems with stale element exceptions
<simonstewart> sadym: so we just used to get elements every time we wanted to use them
<jgraham> q+
<jgraham> simonstewart: That seems a bit more like a selenium-specific problem.
<jgraham> ack jgraham
<sadym> q+
<simonstewart> jgraham: I think having a list per-window and having a different ID for the same thing from different windows is mostly okay. Would be weird if the same shared ID (between classic and bidi) is returned based on which context you access the element through
<jgraham> ack sadym
<simonstewart> sadym: talking about sharing ids between bidi and classic. There's a limitation in the CDP, because in classic it must be a GUID, but in CDP we can't do that.
<simonstewart> q+
<jgraham> ack simonstewart
<jgraham> simonstewart: We could loosen the classic spec to not require a UUID, if that helps.
<whimboo> q+
<jgraham> simonstewart: UUIDs are a should-level requirement
<simonstewart> various: discussion about internals of CDP
<jgraham> ack whimboo
<simonstewart> whimboo: for firefox, we have a js module that generates unique ids for elements. These are floating numbers
<jgraham> q?
<jdescottes> q+
<jgraham> ack jdescottes
<simonstewart> What's Julian's IRC nick?
<jdescottes> jdescottes
<simonstewart> jdescottes: if we have two windows connected to each other (win1, win2), and you're retreieving an element from win2 via win1, and win1 navigates away. Win1 will have lost the element reference. What should we do in that case? Throw a "no such element"?
<simonstewart> jgraham: At the moment, "yes".
<jgraham> q?

@whimboo
Copy link
Contributor

whimboo commented Aug 23, 2022

@jgraham there is another problem. As the WebDriver Bidi spec is written for object serialization and deserialization the object cache to be used is unique per realm. This means when objects of type Node are handled we wouldn't be even able to share the same element between different realms of a single browsing context.

I feel that we might need some other strategy here.

@whimboo
Copy link
Contributor

whimboo commented Aug 23, 2022

To add there is also w3c/webdriver-bidi#180 which introduces a new notion of shared references which I missed.

@jgraham
Copy link
Member Author

jgraham commented Dec 14, 2022

Closing this in favour of #1705

@jgraham jgraham closed this Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants