Closed Bug 1775036 Opened 2 years ago Closed 2 years ago

Remove support for CHROME_ELEMENT_KEY element references

Categories

(Remote Protocol :: Marionette, defect, P1)

Default
defect
Points:
3

Tracking

(firefox103 fixed)

RESOLVED FIXED
103 Branch
Tracking Status
firefox103 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webdriver:m4])

Attachments

(8 files, 1 obsolete file)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

I have seen that while working on bug 1692468. When interacting with chrome scope any chrome element instance in the Marionette client inappropriately gets serialized to a dict containing both a HTML WebElement and a chrome WebElement:

1655716626064	Marionette	DEBUG	2 -> [0,51,"WebDriver:FindElement",{"value":"textInput3","using":"id"}]
1655716626065	Marionette	TRACE	[40] MarionetteCommands actor created for window id 25
1655716626066	Marionette	DEBUG	2 <- [1,51,null,{"value":{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"25bdb6a7-beaf-4e55-9df7-c7e1b73914dc"}}]
UNSUPPORTED (log once): POSSIBLE ISSUE: unit 1 GLD_TEXTURE_INDEX_2D is unloadable and bound to sampler type (Float) - using zero texture because texture unloadable
1655716626067	Marionette	DEBUG	2 -> [0,52,"WebDriver:ExecuteScript",{"script":"return arguments[0].value;","args":[{"element-6066-11e4-a52e-4f735466cecf":"25bdb6a7-beaf-4e55-9df7-c7e1b73914dc","chromeelement-9fc5-4b51-a3c8-01716eedeb04":"25bdb6a7-beaf-4e55-9df7-c7e1b73914dc"}],"newSandbox":true,"sandbox":"default","line":32,"filename":"testing/marionette/harness/marionette_harness/tests/unit/test_text_chrome.py"}]

Here is the related code in the Marionette client and it goes all back to bug 1400233 comment 22.

https://searchfox.org/mozilla-central/rev/cc98a15c7327d742d283cddddde712a8a3165006/testing/marionette/client/marionette_driver/marionette.py#1597-1599

So yes, someone could have a script running in chrome scope and trying to return an element from a content browser. But at the same time also other elements from within the chrome window. As such chrome elements also got the web element id assigned to it so that the same id could be re-used independently in which context the script currently is running in.

But based on the upcoming WebDriver spec changes element references would have to be stored with the scope of the element's active document's window. And this would make handling that way more complex when we move our element store from the parent process to the specific owner global the JSWindowActor pair is attached to. In such a case the serialization code would have to handle different window globals.

As such I wonder if we really want to continue in having this behavior for such a very rare case and allow to use the same content element reference id in both the content and chrome scope.

Adding to the triage list for today's meeting.

See Also: → 1775064

As agreed on in the meeting the client should serialize to a WebElement or ChromeElement but not both.

Points: --- → 1
Priority: -- → P1
Whiteboard: [webdriver:triage] → [webdriver:m4]

We had another discussion on Element and discussed how and if chrome elements should be specifically handled at all:
https://matrix.to/#/!PbfmTydoOIZebaNppn:mozilla.org/$Kd9JCMW6c3YSiHc7K2h28JO5p7btZj_7iBLRcFyYwZc?via=mozilla.org&via=matrix.org&via=archlinux.org

We came to the conclusion that we actually can remove this specific type and only support WebElements. For clients it wouldn't matter anyway and the relationship to a specific browsing context is always there so that any deserialization logic in Marionette will find and can handle the element. The only question that came up was it if it would be helpful for clients to know if it's a XUL or a normal HTML element. But here we never had a requirement for that in all the last years, given that geckodriver is even not able to handle ChromeWebElement references at all.

As such we are going to fix bug 1775064 first and land it on central, 102 beta if still possible, and 102 ESR. That way we do not introduce a breaking change on the next ESR. Then the patch for this bug will only land on mozilla-central for the 103 cycle.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: [client] Chrome elements serialize to both HTML and Chrome Element references → Remove support for CHROME_ELEMENT_KEY element references

This renames the base class so that WebElement can actually
be used for real elements.

Depends on D149956

Attachment #9282317 - Attachment description: Bug 1775036 - [marionette] Rename WebElement to Element. → Bug 1775036 - [marionette] Rename WebElement to WebReference.
Attachment #9282321 - Attachment is obsolete: true
Blocks: 1776355
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1c5d4c86b55
[marionette] Rename WebElement to WebReference. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/57c730ee3404
[marionette] Rename ContentWebElement to WebElement. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/e150c059b43c
[marionette] Rename ContentWebFrame to WebFrame. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/e8c2e3aada2a
[marionette] Rename ContentWebWindow to WebWindow. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/b4eb03a7bd5f
[marionette] Rename ContentShadowRoot to ShadowRoot. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/10004ffd56ce
[marionette] Fix JSDoc comments in element.js. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/965dc4228cd4
[marionette] Remove support for ChromeWebElements. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/43c4718b8847
[geckodriver] Remove unused code for chrome element references. r=webdriver-reviewers,jgraham

Compared to our initial prediction the scope of this bug has kinda exceeded. As discussed with the team we are going to increase the points to 3 for this time.

Points: 1 → 3
Blocks: 1743541
Blocks: 1770420
No longer blocks: webdriver:m4
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: