Remove support for CHROME_ELEMENT_KEY element references
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox103 fixed)
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.
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.
Assignee | ||
Comment 1•3 years ago
•
|
||
As agreed on in the meeting the client should serialize to a WebElement or ChromeElement but not both.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 4•3 years ago
|
||
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 | ||
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D149955
Assignee | ||
Comment 7•3 years ago
|
||
This renames the base class so that WebElement can actually
be used for real elements.
Depends on D149956
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D149957
Assignee | ||
Comment 9•3 years ago
|
||
Depends on D149958
Assignee | ||
Comment 10•3 years ago
|
||
Depends on D149959
Assignee | ||
Comment 11•3 years ago
|
||
Depends on D149960
Updated•3 years ago
|
Assignee | ||
Comment 12•3 years ago
|
||
Depends on D149960
Assignee | ||
Comment 13•3 years ago
|
||
Depends on D149955
Updated•3 years ago
|
Comment 14•3 years ago
|
||
Assignee | ||
Comment 15•3 years ago
|
||
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.
Comment 16•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1c5d4c86b55
https://hg.mozilla.org/mozilla-central/rev/57c730ee3404
https://hg.mozilla.org/mozilla-central/rev/e150c059b43c
https://hg.mozilla.org/mozilla-central/rev/e8c2e3aada2a
https://hg.mozilla.org/mozilla-central/rev/b4eb03a7bd5f
https://hg.mozilla.org/mozilla-central/rev/10004ffd56ce
https://hg.mozilla.org/mozilla-central/rev/965dc4228cd4
https://hg.mozilla.org/mozilla-central/rev/43c4718b8847
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•2 years ago
|
Description
•