1400233 - Drop ContentWebElement.LegacyIdentifier from Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[Andreas Tolfsen ❲:ato❳]

When Marionette creates web element references, it uses the web
element identifier specified by the WebDriver specification
(element.Key) but also a element.LegacyKey.

This will require changes to element.makeWebElement in
testing/marionette/element.js as well as the Marionette
Python client in testing/marionette/client and geckodriver in
testing/geckodriver.

Comment 1

[Andreas Tolfsen ❲:ato❳]

I believe it is now safe to make this change.

Comment 2

[Wambui (:wambui)]

Can I try fixing this bug?

Comment 3

[Andreas Tolfsen ❲:ato❳]

Sure.  The key is now referred to as
ContentWebElement.LegacyIdentifier in Marionette.  Please upload
your patches and flag me for review.

Comment 4

[Wambui (:wambui)]

Is the key also present in the Python client?  
I have only been able to find it in the element.js file.

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Wambui (:wambui) from comment #4)
> Is the key also present in the Python client?  
> I have only been able to find it in the element.js file.

Yes: https://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#28

Comment 6

[Wambui (:wambui)]

Attachment: Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

Marionette currently uses the ContentWebElement.Identifier Key but also has the Legacy Key.
The key is removed by making changes to testing/marionette/element.js and
testing/marionette/client/marionette_driver/marionette.py.

Review commit: https://reviewboard.mozilla.org/r/227322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227322/

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review233210

Thanks for the patch!  This looks good, but you need to make some
minor changes.

You also need to update
https://searchfox.org/mozilla-central/source/testing/marionette/test_element.js.
You can run this test with "./mach xpcshell-test --sequential testing/marionette/test_element.js".

::: commit-message-6ff60:1
(Diff revision 1)
> +Bug 1400233 - Remove ContentWebElement.LegacyIdentifier Key from Marionette; r?ato
> +Marionette currently uses the ContentWebElement.Identifier Key but also has the Legacy Key.
> +The key is removed by making changes to testing/marionette/element.js and
> +testing/marionette/client/marionette_driver/marionette.py.

There should be a blank line between the short commit message and
the long-form message.

::: commit-message-6ff60:2
(Diff revision 1)
> +Bug 1400233 - Remove ContentWebElement.LegacyIdentifier Key from Marionette; r?ato
> +Marionette currently uses the ContentWebElement.Identifier Key but also has the Legacy Key.

s/Key/key/
s/Legacy Key/a legacy key, LegacyIdentifier/

::: moz.configure:549
(Diff revision 1)
> +# Automatically download and use compiled C++ components:
> +ac_add_options --enable-artifact-builds

This shouldn’t be checked in.  You will want to add this to your
local $topsrcdir/mozconfig file, not to the build system defaults.

::: testing/marionette/client/marionette_driver/marionette.py:29
(Diff revision 1)
>  from .geckoinstance import GeckoInstance
>  from .keys import Keys
>  from .timeout import Timeouts
>  
> -WEBELEMENT_KEY = "ELEMENT"
> +
>  W3C_WEBELEMENT_KEY = "element-6066-11e4-a52e-4f735466cecf"

Let’s rename this to WEB_ELEMENT_KEY now.

Comment 8

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f47bd5ebba2c38a32c394ae992328df6eae35df5

Comment 9

[Andreas Tolfsen ❲:ato❳]

There are also some linting problems that you can see locally if
you run "./mach lint testing/marionette".

Comment 10

[Wambui (:wambui)]

Attachment: Bug 1400233 - Remove ContentWebElement.LegacyIdentifier key from Marionette;

Marionette currently uses the ContentWebElement.Identifier key but also has the legacy key.
The key is removed by making changes to testing/marionette/element.js, testing/marionette/test_element.js
 and testing/marionette/client/marionette_driver/marionette.py.

Review commit: https://reviewboard.mozilla.org/r/229626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229626/

Comment 11

[Code Review Bot [:reviewbot]]

Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review235362


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/element.js:1553
(Diff revision 1)
>   * transported over the wire protocol.
>   */
>  class ContentWebElement extends WebElement {
>    toJSON() {
>      return {
> -      [ContentWebElement.Identifier]: this.uuid,
> +      [ContentWebElement.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]

::: testing/marionette/element.js:1580
(Diff revision 1)
>   * over the wire protocol.
>   */
>  class ContentWebWindow extends WebElement {
>    toJSON() {
>      return {
> -      [ContentWebWindow.Identifier]: this.uuid,
> +      [ContentWebWindow.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]

::: testing/marionette/element.js:1604
(Diff revision 1)
>   * are represented as web frames over the wire protocol.
>   */
>  class ContentWebFrame extends WebElement {
>    toJSON() {
>      return {
> -      [ContentWebFrame.Identifier]: this.uuid,
> +      [ContentWebFrame.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]

::: testing/marionette/element.js:1627
(Diff revision 1)
>   * over the wire protocol.
>   */
>  class ChromeWebElement extends WebElement {
>    toJSON() {
>      return {
> -      [ChromeWebElement.Identifier]: this.uuid,
> +      [ChromeWebElement.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8960862 [details]
Bug 1400233 - Remove ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/229626/#review235414

Your update to this changeset introduced a second commit.  The correct
way to address issues is to _modify_ the original commit instead.
Can you squash these two commits together?

::: commit-message-1b080:3
(Diff revision 1)
> +Marionette currently uses the ContentWebElement.Identifier key but also has the legacy key.
> +The key is removed by making changes to testing/marionette/element.js, testing/marionette/test_element.js
> + and testing/marionette/client/marionette_driver/marionette.py.

Format this at ~72 characters.

Comment 13

[Wambui (:wambui)]

Can you help me on how to squash the two commits together?
I've tried to do it, but I still end up with two commits showing.

Comment 14

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

You are using mercurial? Then run `hg histedit` for it. Select `r` for the second commit and finish.

Comment 15

[Wambui (:wambui)]

Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227322/diff/1-2/

Comment 16

[Andreas Tolfsen ❲:ato❳]

I have triggered a new try run, but I’m not actually sure how to
close the outstanding lint issues.  Maybe the linter will close
them?  It seems like a pretty big flaw to the review system that the
reviewer can’t dismiss them.

Comment 17

[Andreas Tolfsen ❲:ato❳]

wambui, you appear to have some test failures in
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d01f198301a7e53d0065c351ee22933abbf9a44c.
Will you please have a look and fix up the patch?

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review236810

Comment 19

[Wambui (:wambui)]

Sure, let me have a look at them.

Comment 20

[Wambui (:wambui)]

After looking at some of the failed tests, there are seem to be three keys being used "ELEMENT", "element-6066-11e4-a52e-4f735466cecf" and "chromeelement-9fc5-4b51-a3c8-01716eedeb04". The key objects created seem to be made up of the "ELEMENT" key and either the "element-6066-11e4-a52e-4f735466cecf" key or the "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key. Without the "ELEMENT" key, the other two are the only ones used. If the "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key is used, the tests fail as the `if` statements that use the "element-6066-11e4-a52e-4f735466cecf" key fail.

Here's an example from `testing/marionette/harness/marionette_harness/tests/unit/test_click.py TestClickNavigation.test_click_link_install_addon` that fails with the changes.

https://pastebin.mozilla.org/9081315


Could this be why the tests are failing?


(In reply to Andreas Tolfsen ‹:ato› from comment #17)
> wambui, you appear to have some test failures in
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d01f198301a7e53d0065c351ee22933abbf9a44c.
> Will you please have a look and fix up the patch?

Comment 21

[Wambui (:wambui)]

The objects with the "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key seem to not get some attributes because of this, which causes the tests failures.

(In reply to Wambui (:wambui) from comment #20)
> After looking at some of the failed tests, there are seem to be three keys
> being used "ELEMENT", "element-6066-11e4-a52e-4f735466cecf" and
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04". The key objects created seem to
> be made up of the "ELEMENT" key and either the
> "element-6066-11e4-a52e-4f735466cecf" key or the
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key. Without the "ELEMENT" key,
> the other two are the only ones used. If the
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key is used, the tests fail as
> the `if` statements that use the "element-6066-11e4-a52e-4f735466cecf" key
> fail.
> 
> Here's an example from
> `testing/marionette/harness/marionette_harness/tests/unit/test_click.py
> TestClickNavigation.test_click_link_install_addon` that fails with the
> changes.
> 
> https://pastebin.mozilla.org/9081315
> 
> 
> Could this be why the tests are failing?
> 
> 
> (In reply to Andreas Tolfsen ‹:ato› from comment #17)
> > wambui, you appear to have some test failures in
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d01f198301a7e53d0065c351ee22933abbf9a44c.
> > Will you please have a look and fix up the patch?

Comment 22

[Andreas Tolfsen ❲:ato❳]

(In reply to Wambui (:wambui) from comment #20)

> The key objects created seem to be made up of the "ELEMENT" key
> and either the "element-6066-11e4-a52e-4f735466cecf" key or the
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key.

That’s correct.  Marionette used to send a dict such as
{"ELEMENT": <uuid>}, then moved to {"ELEMENT": <uuid>,
"element-6066-11e4-a52e-4f735466cecf": <uuid>}, and most recently
your patch here attempts to drop the ELEMENT part of that.  This is
part of a migration effort.

Marionette has a further specialisation in that it supports
automation of chrome context, where web elements are identified by
chromeelement-9fc5-4b51-a3c8-01716eedeb04.  You will need to change
your patch to also map this to the HTMLElement class in the Python
client.

Comment 23

[Wambui (:wambui)]

Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227322/diff/2-3/

Comment 24

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=916d7229e46aa086608b15f585fa61f2a9ee97b9

Comment 25

[Andreas Tolfsen ❲:ato❳]

Apparently I’m not allowed to dismiss the issues raised by the code
review bot.  Can you mark them as fixed?

Comment 26

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review239228

Comment 27

[Wambui (:wambui)]

I've marked them as fixed on mozreview.
https://reviewboard.mozilla.org/r/227322/#review235362

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fd96d40bdb3
Drop ContentWebElement.LegacyIdentifier key from Marionette; r=ato

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6fd96d40bdb3

Comment 30

[Barry Chen]

This change is causing all the Firefox Screenshots Selenium tests to fail.  I believe it's because geckodriver hasn't been updated to use the new key: https://hg.mozilla.org/mozilla-central/file/tip/testing/geckodriver/src/marionette.rs#l655

Here's a trace log snippet:

1522961405067   webdriver::server       DEBUG   -> POST /session/9f2df171-edef-4c6f-8dce-4165742571b2/elements {"using":"css selector","value":"*[id=\"pageActionButton\"]"}
1522961405067   geckodriver::marionette TRACE   -> 82:[0,5,"findElements",{"using":"css selector","value":"*[id=\"pageActionButton\"]"}]
1522961405097   Marionette      TRACE   0 -> [0,5,"findElements",{"using":"css selector","value":"*[id=\"pageActionButton\"]"}]
1522961405100   Marionette      TRACE   0 <- [1,5,null,[{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"8aedd9a2-a7f8-4ac7-9560-b4d5ac8e7217"}]]
1522961405106   geckodriver::marionette TRACE   <- [1,5,null,[{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"8aedd9a2-a7f8-4ac7-9560-b4d5ac8e7217"}]]
1522961405106   webdriver::server       DEBUG   <- 500 Internal Server Error {"value":{"error":"unknown error","message":"Failed to extract Web Element from response","stacktrace":""
}}

Comment 31

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

(In reply to Barry Chen from comment #30)
> This change is causing all the Firefox Screenshots Selenium tests to fail. 
> I believe it's because geckodriver hasn't been updated to use the new key:
> https://hg.mozilla.org/mozilla-central/file/tip/testing/geckodriver/src/
> marionette.rs#l655

Yes, that is something we figured out yesterday too. See bug 1451916. Maybe we have to wait here until the other bug has been fixed.

Comment 32

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

Well, I just saw that this patch already landed, and caused a major regression. I would suggest that we get it backed-out, so we can fix bug 1451916 first.

(In reply to Pulsebot from comment #28)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/6fd96d40bdb3
> Drop ContentWebElement.LegacyIdentifier key from Marionette; r=ato

Sheriffs, please backout this change. Thanks.

Comment 33

[Andreea Pavel [:apavel]]

(In reply to Henrik Skupin (:whimboo) from comment #32)
> Well, I just saw that this patch already landed, and caused a major
> regression. I would suggest that we get it backed-out, so we can fix bug
> 1451916 first.
> 
> (In reply to Pulsebot from comment #28)
> > Pushed by [e-mail]:
> > https://hg.mozilla.org/integration/autoland/rev/6fd96d40bdb3
> > Drop ContentWebElement.LegacyIdentifier key from Marionette; r=ato
> 
> Sheriffs, please backout this change. Thanks.

Backout done, merged around trees: https://hg.mozilla.org/mozilla-central/rev/62c2508f27a865dd0ac5ca3f0485cb20afafbbd0

Comment 34

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

We have to wait for bug 1451916 being fixed and a new geckodriver release, before we can re-land this patch.

Comment 35

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

Andreas, wouldn't we have to add the chrome element identifier to marionette client too? I don't see it defined in marionette.py:

https://dxr.mozilla.org/mozilla-central/rev/62c2508f27a865dd0ac5ca3f0485cb20afafbbd0/testing/marionette/client/marionette_driver/marionette.py#28-29

Comment 36

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

Actually that is part of the attached patch. So please drop that comment.

Comment 37

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

geckodriver 0.21.0 has been released yesterday. So we can finally land this patch again.

Comment 38

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

Andreas, I don't have permissions to reopen the mozreview patch for landing. Can you please have a look? Maybe you can as reviewer?

Otherwise if Wambui is faster, I would appreciate if you could do that. Just click the `review` link next to the patch at the top of this bug, and then `Reopen`. Thanks

Comment 39

[Andreas Tolfsen ❲:ato❳]

Apparently I don’t have permission to do this either.  My tolerance
for mozreview is diminishing rapidly.

But fear not!  I’ll re-submit the patches for landing.

Comment 40

[Andreas Tolfsen ❲:ato❳]

Attachment: Drop ContentWebElement.LegacyIdentifier key from Marionette. r=ato

Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8

Comment 41

[Andreas Tolfsen ❲:ato❳]

Attachment: Drop ContentWebElement.LegacyIdentifier key from Marionette. r?ato,whimboo

Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8

Comment 42

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8985904 [details] [diff] [review]
Drop ContentWebElement.LegacyIdentifier key from Marionette. r?ato,whimboo

Review of attachment 8985904 [details] [diff] [review]:
-----------------------------------------------------------------

whimboo: I’d like you to have a second look at this before we re-land
it.  Perhaps we need to communicate to the Selenium project that
they need to upgrade to 0.21.0.

Comment 43

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

Comment on attachment 8985904 [details] [diff] [review]
Drop ContentWebElement.LegacyIdentifier key from Marionette. r?ato,whimboo

Review of attachment 8985904 [details] [diff] [review]:
-----------------------------------------------------------------

I miss tests for the chrome element identifier in test_element.js. Mind adding at least a basic one?

Comment 44

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

Alexei, we are trying to land this patch that requires geckodriver 0.21.0 which has been released on Friday last week. Please note that failing Nightly tests would mean you haven't updated yet. 

Otherwise I don't think that we have to worry given that we land on Nightly only. People might notice that they have to upgrade.

Comment 45

[Alexei Barantsev]

Selenium CI uses the latest geckodriver version published to GitHub and Nightly broswer version.

Comment 46

[Andreas Tolfsen ❲:ato❳]

Attachment: Drop ContentWebElement.LegacyIdentifier key from Marionette. r?ato,whimboo

Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8

Comment 47

[Andreas Tolfsen ❲:ato❳]

Attachment: Drop ContentWebElement.LegacyIdentifier key from Marionette. r=ato,whimboo

Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8

Comment 48

[Andreas Tolfsen ❲:ato❳]

Rebased patch, new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe5c2723d1d0f0eafce5cf30ed9ffd4846dfe71

Comment 49

[Andreas Tolfsen ❲:ato❳]

Sorry I dropped the ball on this one.  The last try run looks good,
but I suspect another rebase is needed by now.

Comment 50

[Andreas Tolfsen ❲:ato❳]

Attachment: Drop ContentWebElement.LegacyIdentifier key from Marionette.

Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/036eaf559f72
Drop ContentWebElement.LegacyIdentifier key from Marionette. r=ato,whimboo

Comment 52

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/036eaf559f72