Closed Bug 1691683 Opened 3 years ago Closed 1 year ago

Remove unused WebElementEventTarget class

Categories

(Remote Protocol :: Marionette, defect, P1)

Firefox 84
defect
Points:
1

Tracking

(firefox85 wontfix, firefox86 wontfix, firefox87 wontfix, firefox88 wontfix, firefox111 fixed)

RESOLVED FIXED
111 Branch
Tracking Status
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)

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: nobody → hskupin
Status: NEW → ASSIGNED

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.

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.

No longer blocks: 1669172
Depends on: 1669172

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.

Keywords: regression
Regressed by: 1669169
Version: Default → Firefox 84

Tracking marionette-fission-reserve bugs for Fission Future (post-MVP).

Fission Milestone: --- → Future

This bug is still valid and I should work on it.

Fission Milestone: Future → ---

(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.

Depends on: 1780212
Summary: WebElementEventTarget broken for JSWindowActor implementation → Remove unused WebElementEventTarget class

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.

https://treeherder.mozilla.org/jobs?repo=try&revision=4fd22c26e0f0f8d32a209ba789b2b50603dfa456&selectedTaskRun=Basuvu9rTda2mZJ_CRQz_A.0

I'll have to take a look later this week or next week again.

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

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:

https://searchfox.org/mozilla-central/rev/cf3af6bb6657278880f8baf38435eeb8f2d5d86c/remote/marionette/browser.sys.mjs#111-117

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.

A try build for the proposed minor update of the patch can be found at:
https://treeherder.mozilla.org/jobs?repo=try&revision=21a88e7a758da0f70c6f308377e6fcb46b00f206

Points: --- → 1
Priority: P2 → P1
Whiteboard: [marionette-fission-reserve] → [marionette-fission-reserve][webdriver:m6]
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16df25c22c56
[marionette] Remove unused WebElementEventTarget class. r=webdriver-reviewers,jdescottes
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
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: