Closed Bug 1775064 Opened 3 years ago Closed 3 years ago

DOM nodes from browser windows are no longer returned as chrome element references

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Default
Type:
defect
Points:
2

Tracking

(firefox-esr91 wontfix, firefox-esr102102+ fixed, firefox101 wontfix, firefox102 fixed, firefox103 fixed)

Status:
RESOLVED FIXED
Milestone:
103 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 102+ fixed
firefox101 --- wontfix
firefox102 --- fixed
firefox103 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [webdriver:m4][webdriver-external])

Attachments

(1 file)

Bug 1775064 - [marionette] Return chrome element references for elements within any privileged document.
48 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-release+
pascalc
: approval-mozilla-esr102+
Details | Review

Not all the elements, which live in the XUL namespace, are actually returned as chrome elements but HTML elements:

1655724675574	Marionette	DEBUG	3 -> [0,2,"Marionette:SetContext",{"value":"chrome"}]
1655724675574	Marionette	DEBUG	3 <- [1,2,null,{"value":null}]
1655724675575	Marionette	DEBUG	3 -> [0,3,"WebDriver:FindElement",{"value":"urlbar","using":"id"}]
1655724675577	Marionette	TRACE	[9] MarionetteCommands actor created for window id 4
1655724675577	Marionette	DEBUG	3 <- [1,3,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"e8ba9ddf-46d0-458e-b90c-b4d9911530bf"}}]

And here for another element that actually lives in a non browser window:

1655724491116	Marionette	DEBUG	2 -> [0,50,"Marionette:SetContext",{"value":"chrome"}]
1655724491116	Marionette	DEBUG	2 <- [1,50,null,{"value":null}]
1655724491117	Marionette	DEBUG	2 -> [0,51,"WebDriver:FindElement",{"value":"textInput3","using":"id"}]
1655724491117	Marionette	TRACE	[40] MarionetteCommands actor created for window id 25
1655724491118	Marionette	DEBUG	2 <- [1,51,null,{"value":{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"d772c16b-21f7-4cb4-9691-ca98a61c65db"}}]

If that is a regression we should get it investigated and fixed, but we should also clarify what we actually expect. IMHO any element that is not part of a tab's content browser should be a chrome element and as such returned.

Ok, so this is indeed related to browser windows only. Now that we do no longer use XUL documents for them but XHTML documents the isInXULDocument check returns false due to the http://www.w3.org/1999/xhtml namespace.

https://searchfox.org/mozilla-central/rev/cc98a15c7327d742d283cddddde712a8a3165006/remote/marionette/element.js#1371-1378

Most likely we should change the check by actually checking for a chrome:// URI instead, which would also include chrome://browser/content/browser.xhtml.

Summary: Not all he DOM nodes in XUL scope get a chrome element reference → DOM nodes from browser windows are no longer returned as chrome element references

Actually the window within the browser.xhtml file has different namespaces:
https://searchfox.org/mozilla-central/source/browser/base/content/browser.xhtml#38-40

<html id="main-window"
        xmlns:html="http://www.w3.org/1999/xhtml"
        xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        xmlns="http://www.w3.org/1999/xhtml"

As such it might work if we could also check for xmlns:xul? I would have to check which property to use given that namespaceURI only returns the default one.

As discussed in the meeting we should find a way to determine the XUL/Chrome status based on something else than just the namespace of the element / document.

It's blocking a M4 bug, so moving this to M4 as well. We should create new tests as well to ensure that we do not regress that behavior again.

Points: --- → 2
Priority: -- → P1
Whiteboard: [webdriver:triage] → [webdriver:m4]
Assignee: nobody → hskupin
Status: NEW → ASSIGNED

This is actually a regression from bug 1492582 which landed in Firefox 72!

status-firefox101: --- → wontfix
status-firefox102: --- → affected
status-firefox103: --- → affected
status-firefox-esr102: --- → affected
status-firefox-esr91: --- → wontfix
Keywords: regression
Regressed by: 1492582
Whiteboard: [webdriver:m4] → [webdriver:m4][webdriver-external]
Blocks: 1764594
Attachment #9282161 - Attachment description: WIP: Bug 1775064 - [marionette] Return chrome element references for elements within any privileged document. → Bug 1775064 - [marionette] Return chrome element references for elements within any privileged document.
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d24535fc8fc5 [marionette] Return chrome element references for elements within any privileged document. r=webdriver-reviewers,jdescottes

Backed out for causing mochitest failures on browser_all_files_referenced.js

Backout link
Push with failures
Link to failure log
Failure line :
TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected +0

Flags: needinfo?(hskupin)

This fails because the newly added test_no_xul.html file is used similar to the other files in this folder for testing purposes. Until bug 1344267 is fixed we have to exclude it from the checks in browser_all_files_referenced.js.

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/76df4ebf0bea [marionette] Return chrome element references for elements within any privileged document. r=webdriver-reviewers,jdescottes

https://hg.mozilla.org/mozilla-central/rev/76df4ebf0bea

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox103: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

Comment on attachment 9282161 [details]
Bug 1775064 - [marionette] Return chrome element references for elements within any privileged document.

Beta/Release Uplift Approval Request

  • User impact if declined: The bug will only impact a limited amount of users that are actually testing the chrome scope of Gecko applications with Marionette.

Since Firefox switched the XUL-based browser window to XHTML the wrong type of element is returned to clients. For consistency we would like to get this fixed on 102 and 102 ESR before we are starting a major refactoring in Marionette.

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Only affects testing with Marionette and doesn't change any behavior with the Marionette client.
  • String changes made/needed:
  • Is Android affected?: Yes

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Since Firefox switched the XUL-based browser window to XHTML the wrong type of element is returned to clients. For consistency we would like to get this fixed on 102 and 102 ESR before we are starting a major refactoring in Marionette.
  • User impact if declined: The bug will only impact a limited amount of users that are actually testing the chrome scope of Gecko applications with Marionette.
  • Fix Landed on Version: 103
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Only affects testing with Marionette and doesn't change any behavior with the Marionette client.
Attachment #9282161 - Flags: approval-mozilla-esr102?
Attachment #9282161 - Flags: approval-mozilla-beta?

Hi Ryan, I'm not sure if a beta (102) uplift is still possible and if the request should have been for release given that the merge did already happen. If it's too late we can leave 102 alone but would still really like to see it on the 102esr branch. Thanks.

Flags: needinfo?(ryanvm)

Comment on attachment 9282161 [details]
Bug 1775064 - [marionette] Return chrome element references for elements within any privileged document.

We already shipped our RC build and have no driver for another RC or another ESR 102 build so far. I am keeping the request open for a potential dot release. We can uplift that patch in the next ESR 102 minor release (103 timeframe).

Attachment #9282161 - Flags: approval-mozilla-beta? → approval-mozilla-release?

That's fine. Thanks.

Flags: needinfo?(ryanvm)

The patch landed in nightly and beta is affected.
:whimboo, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox102 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(hskupin)
Flags: needinfo?(hskupin)

Comment on attachment 9282161 [details]
Bug 1775064 - [marionette] Return chrome element references for elements within any privileged document.

Approved for 102.0.1, thanks.

Attachment #9282161 - Flags: approval-mozilla-release? → approval-mozilla-release+
Blocks: 1770420
No longer blocks: webdriver:m4

Comment on attachment 9282161 [details]
Bug 1775064 - [marionette] Return chrome element references for elements within any privileged document.

Approved for ESR102.0.1, thanks.

Attachment #9282161 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
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: