Closed Bug 1405018 Opened 7 years ago Closed 7 years ago

Current browsing context not considered when checking element staleness

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect

Tracking

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

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: regression)

Attachments

(4 files)

Bug 1405018 - Move Get Active Element test to the tests directory.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1405018 - Add support helper for generating inline <iframe> elements.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1405018 - Consider current browsing context on staleness check.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1405018 - Add test for stale frame element.
59 bytes, text/x-review-board-request
impossibus
: review+
Details 
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: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
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 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 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?(mjzffr)
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 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 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 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.)
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.
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.
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.
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 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 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?
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.
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 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.
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 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-
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.
Attachment #8914438 - Flags: review- → review?
Attachment #8914438 - Flags: review? → review?(mjzffr)
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 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
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
Andreas, any objections in directly requesting uplift to beta? As it looks like it works fine so far.
Flags: needinfo?(ato)
(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]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: