Remove unused WebElementEventTarget class
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox85 wontfix, firefox86 wontfix, firefox87 wontfix, firefox88 wontfix, firefox111 fixed)
People
(Reporter: whimboo, Assigned: whimboo)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [marionette-fission-reserve][webdriver:m6])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Within the framescript we register message listeners (domAddEventListener, and domRemoveEventListener) that were handling content events for WebElementEventTarget
.
In our actor implementation we do not have these listeners setup, and as such each usage of WebElementEventTarget
will actually hang or timeout when used via TimedPromise
. This can easily be seen for WebDriver:MinimizeWindow
:
1612866848962 Marionette DEBUG 3 -> [0,2,"WebDriver:MinimizeWindow",{}]
[..]
1612866853968 Marionette WARN TimedPromise timed out after 5000 ms: stacktrace:
TimedPromise/<@chrome://marionette/content/sync.js:231:19
TimedPromise@chrome://marionette/content/sync.js:216:10
GeckoDriver.prototype.minimizeWindow@chrome://marionette/content/driver.js:2541:11
1612866854970 Marionette DEBUG 3 <- [1,2,null,{"x":4,"y":25,"width":1280,"height":892}]
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
The problem here is that WebElementEventTarget
makes use of the message manager. That would mean that a completely new implementation is necessary.
Given that only a single method (WebDriver:MinimizeWindow
) uses it, some additional if conditions will be fine.
Assignee | ||
Comment 2•4 years ago
|
||
The problem here is that WebElementEventTarget
makes use of the message manager. That would mean that a completely new implementation is necessary, or a lots of if conditions. As such I would propose that we get this bug fixed after the patch on bug 1669172 landed.
Assignee | ||
Comment 3•4 years ago
|
||
The problem here is that WebElementEventTarget
makes use of the message manager. That would mean that a completely new implementation is necessary, or a lots of if conditions. As such I would propose that we get this bug fixed after the patch on bug 1669172 landed.
Assignee | ||
Updated•4 years ago
|
Comment 4•4 years ago
|
||
Tracking marionette-fission-reserve bugs for Fission Future (post-MVP).
Updated•4 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
This bug is still valid and I should work on it.
Assignee | ||
Comment 6•2 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)
Given that only a single method (
WebDriver:MinimizeWindow
) uses it, some additional if conditions will be fine.
With the changes on bug 1780212 the usage of WebElementEventTarget
has been removed from WebDriver:MinimizeWindow
. As such there is no longer a consumer of this class and we can remove it completely from our Marionette code base.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 7•2 years ago
|
||
I tried to remove all the traces but sadly there is one test that now starts to perma fail and as it looks like switch to window isn't working as expected because the right browsing context reference isn't set.
I'll have to take a look later this week or next week again.
Assignee | ||
Comment 8•2 years ago
|
||
I've missed to follow-up on this bug. Given that a lot of code has changed since then I had to rebase my changes. I've pushed a new try build for now to see if this particular test I was referring to in my last comment is still perma failing:
https://treeherder.mozilla.org/jobs?repo=try&revision=e4a3c5d43ecd5bbbd5860477fc8f2817e37f23be
Assignee | ||
Comment 9•2 years ago
|
||
The perma failure is still present. And it's actually related to the browsingContext to be null
:
https://treeherder.mozilla.org/logviewer?job_id=403383178&repo=try&lineNumber=74857-74859
After a bit of investigation the problem is actually in the following line:
https://searchfox.org/mozilla-central/rev/cf3af6bb6657278880f8baf38435eeb8f2d5d86c/remote/marionette/browser.sys.mjs#317
And surprisingly it's not the creation of the WebElementEventTarget
instance but the access to this.messageManager
. As it looks like we need to access this.contentBrowser.messageManager
to most likely get a new messageManager created for the content browser if none exist yet. This message manager then keeps the window alive:
My proposal would be to continue with the patch and just have a line that accesses the window manager in switchToTab()
to establish the connection and keep the window / browsing context active for browserless tabs. Whenever we have to get rid of the message manager we will have to find another solution.
Assignee | ||
Comment 10•2 years ago
|
||
A try build for the proposed minor update of the patch can be found at:
https://treeherder.mozilla.org/jobs?repo=try&revision=21a88e7a758da0f70c6f308377e6fcb46b00f206
Assignee | ||
Comment 11•2 years ago
|
||
Assignee | ||
Updated•2 years ago
|
Comment 12•2 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16df25c22c56 [marionette] Remove unused WebElementEventTarget class. r=webdriver-reviewers,jdescottes
Comment 13•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Description
•