Current browsing context not considered when checking element staleness

RESOLVED FIXED in Firefox 57

Status

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks: 1 bug, {regression})

Version 3
mozilla58
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
As can be seen from the discussion in
https://github.com/mozilla/geckodriver/issues/934, the
element.isStale function does not take into account the
WebDriver-selected current browsing context when checking an
element’s staleness.  This means, for example, that an element in
an <iframe> that gets retrieved, will still be considered valid for
as long as its associated document lives.

In WebDriver the expected behaviour is for the element reference to
only be valid for the current browsing context, meaning retrieving
the element reference when another browsing context is chosen should
return an ‘stale element’ error.
(Assignee)

Updated

2 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
This is actually a regression from bug 1394881, and needs to be fixed for both central, and beta.
Blocks: 1394881
status-firefox57: --- → affected
status-firefox58: --- → affected
Keywords: regression
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191386

I don't know what I'm reviewing here because I have contributed nearly nothing yet to wpt-tests. It's better to ask Maja for review here. Beside that I added some notes.

::: testing/web-platform/meta/MANIFEST.json:409310
(Diff revision 2)
>       {
>        "timeout": "long"
>       }
>      ]
>     ],
> +   "webdriver/tests/get_active_element.py": [

Wouldn't an `elements` subfolder makes sense here, so it is better grouped with other tests? The window tests should have added to a subfolder like `contexts` too. I take the actions tests as reference here.

::: testing/web-platform/meta/webdriver/tests/get_active_element.py.ini:13
(Diff revision 2)
> +
> +  [get_active_element.py::test_handle_prompt_missing_value]
> +    expected: FAIL
> +
> +  [get_active_element.py::test_sucess_without_body]
> +    expected: FAIL

Why do we expect all to fail now given that you only moved the tests?
Attachment #8914438 - Flags: review?(hskupin)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191386

I don't know what I'm reviewing here because I have contributed nearly nothing yet to wpt-tests. It's better to ask Maja for review here. Beside that I added some notes.

::: testing/web-platform/meta/MANIFEST.json:409310
(Diff revision 2)
>       {
>        "timeout": "long"
>       }
>      ]
>     ],
> +   "webdriver/tests/get_active_element.py": [

Wouldn't an `elements` subfolder makes sense here, so it is better grouped with other tests? The window tests should have added to a subfolder like `contexts` too. I take the actions tests as reference here.

::: testing/web-platform/meta/webdriver/tests/get_active_element.py.ini:13
(Diff revision 2)
> +
> +  [get_active_element.py::test_handle_prompt_missing_value]
> +    expected: FAIL
> +
> +  [get_active_element.py::test_sucess_without_body]
> +    expected: FAIL

Why do we expect all to fail now given that you only moved the tests?

Comment 14

2 years ago
mozreview-review
Comment on attachment 8914439 [details]
Bug 1405018 - Add support helper for generating inline <iframe> elements.

https://reviewboard.mozilla.org/r/185748/#review191388

::: testing/web-platform/tests/webdriver/tests/support/inline.py:33
(Diff revision 2)
>      return build_url("/webdriver/tests/support/inline.py",
>                       query=urllib.urlencode(query),
>                       protocol=protocol)
>  
>  
> +def iframe(doc):

`src` requires just a URL. Please update the parameters name for it.
Attachment #8914439 - Flags: review?(hskupin) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8914440 [details]
Bug 1405018 - Consider current browsing context on staleness check.

https://reviewboard.mozilla.org/r/185750/#review191392

::: testing/marionette/element.js:166
(Diff revision 3)
> +   * Upon retrieving the element, an element staleness check is
> +   * performed.
> +   *
>     * @param {string} uuid
>     *     Web element reference, or UUID.
> + * @param {WindowProxy} window

Please fix the indentation.

::: testing/marionette/element.js:181
(Diff revision 3)
>     * @throws {StaleElementReferenceError}
>     *     If the element has gone stale, indicating it is no longer
>     *     attached to the DOM, or its node document is no longer the
>     *     active document.
>     */
> -  get(uuid) {
> +  get(uuid, window) {

Once you are using `win` and other times `window`. Please choose one of them.

::: testing/marionette/element.js:649
(Diff revision 3)
>   *
> - * @param {Element} el
> - *     DOM element to check for staleness.
> + * The currently selected browsing context, specified through
> + * <var>window<var>, is a WebDriver concept defining the target
> + * against which commands will run.  As the current browsing context
> + * may differ from <var>el</var>'s associated context, an element is
> + * considered stale even if it is connected to a living (not discarded)

s/considered stale/considered valid/

::: testing/marionette/element.js:659
(Diff revision 3)
> + *     the case if the element has been unwrapped from a weak
> + *     reference, it is always considered stale.
> + * @param {WindowProxy=} window
> + *     Current browsing context, which may differ from the associate
> + *     browsing context of <var>el</var>.  When retrieving XUL
> + *     elements, this is optional.

I don't see that you special-case chrome context (XUL) below.

::: testing/marionette/evaluate.js:203
(Diff revision 3)
>   *
>   * @return {Object}
>   *     Same object as provided by <var>obj</var> with the web elements
>   *     replaced by DOM elements.
>   */
> -evaluate.fromJSON = function(obj, seenEls, win, shadowRoot = undefined) {
> +evaluate.fromJSON = function(obj, seenEls, window) {

Removing `shadowRoot` is not really tight to this bug. It should have been made in a different patch.
Attachment #8914440 - Flags: review?(hskupin) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8914441 [details]
Bug 1405018 - Add test for stale frame element.

https://reviewboard.mozilla.org/r/185752/#review191404

Maja will be a better reviewer here.

::: testing/web-platform/tests/webdriver/tests/switch_to_parent_frame.py:7
(Diff revision 3)
> +from webdriver import StaleElementReferenceException
> +
> +from tests.support.inline import inline, iframe
> +
> +
> +# 10.6 Switch To Parent Frame

What's up with that line?
Attachment #8914441 - Flags: review?(hskupin)

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191386

> Wouldn't an `elements` subfolder makes sense here, so it is better grouped with other tests? The window tests should have added to a subfolder like `contexts` too. I take the actions tests as reference here.

Yeah, if we expect more element-related tests when they should all go in an element dir. (At least that's how the other tests got reorganized recently.)
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191386

> Yeah, if we expect more element-related tests when they should all go in an element dir. (At least that's how the other tests got reorganized recently.)

I guess this is not the right place for a naming debat, but I
disagree somewhat with this naming scheme because it makes it much
harder to find tests for a particular command without knowing which
chapter in the specification it is defined in.  When more files for
a single command is added, it is possible to have a directory with
the command’s name.

I have therefore moved it to testing/web-platform/tests/webdriver/tests/element_retrieval/get_active_element.py.

> Why do we expect all to fail now given that you only moved the tests?

Yes, the test was not run before.
(Assignee)

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8914439 [details]
Bug 1405018 - Add support helper for generating inline <iframe> elements.

https://reviewboard.mozilla.org/r/185748/#review191388

> `src` requires just a URL. Please update the parameters name for it.

inline(doc) produces a URL.  You pass the document into the function.
(Assignee)

Comment 21

2 years ago
mozreview-review-reply
Comment on attachment 8914440 [details]
Bug 1405018 - Consider current browsing context on staleness check.

https://reviewboard.mozilla.org/r/185750/#review191392

> Once you are using `win` and other times `window`. Please choose one of them.

I try to be consistent in using longer-form variable names such
as "window" for input or output variables, and "win" sometimes as
intermediary internal variables.

> s/considered stale/considered valid/

I mean “valid” here.

> I don't see that you special-case chrome context (XUL) below.

window is populated with el.ownerGlobal if window is not chosen.
This is only optional in XUL, because the window global is always
the same.

> Removing `shadowRoot` is not really tight to this bug. It should have been made in a different patch.

I disagree. shadowRoot is not being picked up by this function or
its dependents, so it can be safely removed.
(Assignee)

Comment 22

2 years ago
mozreview-review-reply
Comment on attachment 8914441 [details]
Bug 1405018 - Add test for stale frame element.

https://reviewboard.mozilla.org/r/185752/#review191404

> What's up with that line?

I don’t understand the question?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8914441 [details]
Bug 1405018 - Add test for stale frame element.

https://reviewboard.mozilla.org/r/185752/#review191404

> I don’t understand the question?

So after thinking more about that, it is the chapter of the webdriver specification. That's not really clear for someone who hasn't written a test yet. Feel free to drop if that style is used in other tests too.

Comment 28

2 years ago
mozreview-review-reply
Comment on attachment 8914440 [details]
Bug 1405018 - Consider current browsing context on staleness check.

https://reviewboard.mozilla.org/r/185750/#review191392

> I try to be consistent in using longer-form variable names such
> as "window" for input or output variables, and "win" sometimes as
> intermediary internal variables.

Which you are not doing for eg. getElementCenter.

> I mean “valid” here.

If you mean `valid` why do we still have `stale` here, and the issue got dropped?
(Assignee)

Comment 29

2 years ago
mozreview-review-reply
Comment on attachment 8914441 [details]
Bug 1405018 - Add test for stale frame element.

https://reviewboard.mozilla.org/r/185752/#review191404

> So after thinking more about that, it is the chapter of the webdriver specification. That's not really clear for someone who hasn't written a test yet. Feel free to drop if that style is used in other tests too.

I don’t think this comment is super-important, so happy to remove
it.  I just put it there because I copied the user prompt tests from
another file.
(Assignee)

Comment 30

2 years ago
mozreview-review-reply
Comment on attachment 8914440 [details]
Bug 1405018 - Consider current browsing context on staleness check.

https://reviewboard.mozilla.org/r/185750/#review191392

> Which you are not doing for eg. getElementCenter.

… which I did not make a change to in this patch.

I’m dropping this issue because I don’t think there’s a
deficiency here that needs to be fixed before the patch is merged.

> If you mean `valid` why do we still have `stale` here, and the issue got dropped?

Sigh.  Sorry, I meant “stale” in my reply.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8914440 [details]
Bug 1405018 - Consider current browsing context on staleness check.

https://reviewboard.mozilla.org/r/185750/#review191392

> … which I did not make a change to in this patch.
> 
> I’m dropping this issue because I don’t think there’s a
> deficiency here that needs to be fixed before the patch is merged.

You do by adding the `win` parameter. See line 1433 of elements.js in your patch.
(Assignee)

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8914440 [details]
Bug 1405018 - Consider current browsing context on staleness check.

https://reviewboard.mozilla.org/r/185750/#review191392

> You do by adding the `win` parameter. See line 1433 of elements.js in your patch.

OK, I’ve updated that to "window".
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

2 years ago
mozreview-review
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191908

My understanding is that the tests should be in subdirectories: https://github.com/w3c/web-platform-tests/pull/6809
Attachment #8914438 - Flags: review?(mjzffr) → review-
(Assignee)

Comment 42

2 years ago
mozreview-review-reply
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191908

I don’t think you checked the review carefully enough.  The tests
are living in the a subdirectory.
(Assignee)

Updated

2 years ago
Attachment #8914438 - Flags: review- → review?
(Assignee)

Updated

2 years ago
Attachment #8914438 - Flags: review? → review?(mjzffr)

Updated

2 years ago
status-firefox57: affected → ---
status-firefox58: affected → ---

Updated

2 years ago
status-firefox56: --- → affected
status-firefox57: --- → affected

Comment 43

2 years ago
mozreview-review-reply
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191908

Sorry! that comment applied to an older interdiff.

Comment 44

2 years ago
mozreview-review
Comment on attachment 8914438 [details]
Bug 1405018 - Move Get Active Element test to the tests directory.

https://reviewboard.mozilla.org/r/185746/#review191984
Attachment #8914438 - Flags: review?(mjzffr) → review+
Looks like this may affect 57 and 58, but not 56, since bug 1394881 landed in 57.
status-firefox56: affected → unaffected
status-firefox58: --- → affected
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 50

2 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c188cf79dc
Move Get Active Element test to the tests directory. r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/f24efcd4d6d8
Add support helper for generating inline <iframe> elements. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b443c0de681
Consider current browsing context on staleness check. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d6602e602c
Add test for stale frame element. r=maja_zf
status-firefox-esr52: --- → unaffected
Andreas, any objections in directly requesting uplift to beta? As it looks like it works fine so far.
Flags: needinfo?(ato)
(Assignee)

Comment 53

2 years ago
(In reply to Henrik Skupin (:whimboo) from comment #52)
> Andreas, any objections in directly requesting uplift to beta? As it looks
> like it works fine so far.

Yes!  Thanks for reminding me.

Sheriffs: Please uplift this to beta, if possible.
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.