MarionetteCommands actor leaks observers of dialog observer instance
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(Fission Milestone:M7, firefox-esr78 unaffected, firefox83 wontfix, firefox84 fixed, firefox85 fixed)
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox83 | --- | wontfix |
firefox84 | --- | fixed |
firefox85 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Regression)
Details
(Keywords: memory-leak, regression, Whiteboard: [marionette-fission-mvp][simple])
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
Just noticed that the MarionetteCommand actor creates an instance of the DialogObserver
within actorCreated
, and registers for all the following observer notifications:
Services.obs.addObserver(this, "common-dialog-loaded");
Services.obs.addObserver(this, "tabmodal-dialog-loaded");
Services.obs.addObserver(this, "toplevel-window-ready");
Sadly we do not remove those listeners in didDestroy
, and as such these are going to leak.
As result I see tons of toplevel-window-ready
notifications for TestClickCloseContext.test_click_close_window
when running all tests in that file.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Marking as being soft-blocked by bug 1678708 given that I want to add some tests and the other bug lays the basics for it.
Assignee | ||
Comment 2•4 years ago
|
||
Actually I cannot find a way with xpcshell tests to get this tested. We would need browser chrome tests but those aren't available for Marionette.
Assignee | ||
Comment 3•4 years ago
|
||
Assignee | ||
Comment 4•4 years ago
|
||
Updated•4 years ago
|
Comment 5•4 years ago
|
||
Tracking marionette-fission-mvp bugs for Fission M7 Beta milestone.
Comment 7•4 years ago
|
||
bugherder |
Comment 8•4 years ago
|
||
Does this need a Beta uplift?
Assignee | ||
Comment 9•4 years ago
|
||
Comment on attachment 9189882 [details]
Bug 1679414 - [marionette] Don't leak registered dialogObserver listeners in MarionetteCommands actor.
Beta/Release Uplift Approval Request
- User impact if declined: When Firefox is under remote control eg by running Selenium tests the new actor implementation is leaking memory by not unregistering listeners and observer notifications.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- 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): Ensures to unregister all listeners and observers when the JSWindowActor gets destroyed.
- String changes made/needed:
Comment 10•4 years ago
|
||
Comment on attachment 9189882 [details]
Bug 1679414 - [marionette] Don't leak registered dialogObserver listeners in MarionetteCommands actor.
Approved for 84.0b7.
Comment 11•4 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Updated•2 years ago
|
Description
•