waitForNavigationCompleted doesn't clean-up listeners when running into a page timeout
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(Fission Milestone:M7, firefox-esr78 unaffected, firefox82 wontfix, firefox83 fixed, firefox84 fixed)
| Tracking | Status | |
|---|---|---|
| firefox-esr78 | --- | unaffected |
| firefox82 | --- | wontfix |
| firefox83 | --- | fixed |
| firefox84 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Regression)
Details
(Keywords: memory-leak, regression, Whiteboard: [marionette-fission-mvp][simple])
Attachments
(1 file)
|
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
Within waitForNavigationCompleted we register a couple of listeners (events, observer, message) that usually get all cleaned-up within checkDone.
Sadly all those registered listeners aren't cleaned-up in case of a page load timeout. The reason for that is the usage of the TimedPromise and that it internally rejects the promise, and the code in waitForNavigationCompleted doesn't use the catch or finally methods.
That's a regression from moving the navigation code into the parent process (bug 1612831).
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 1•4 years ago
|
||
I actually noticed that while working on bug 1673823 where the MarionetteEvents actor could not be registered again after a call to a timing out WebDriver:NavigateTo command.
|
|
Assignee | |
Comment 2•4 years ago
|
||
This is actually a memory leak now given that this code is in the parent process that lives all the time. When it was in the framescript it got cleaned-up after a remoteness change.
|
|
Assignee | |
Comment 3•4 years ago
|
||
|
||
Comment 4•4 years ago
|
||
Tracking marionette-fission-mvp bugs for Fission Beta milestone (M7).
|
|
Assignee | |
Updated•4 years ago
|
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8dac5d18ef27 [marionette] waitForNavigationCompleted has to clean-up registered listeners. r=marionette-reviewers,maja_zf
|
||
Comment 6•4 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 7•4 years ago
|
||
Comment on attachment 9184567 [details]
Bug 1674158 - [marionette] waitForNavigationCompleted has to clean-up registered listeners.
Beta/Release Uplift Approval Request
- User impact if declined: Registered listeners for events and observer notifications are not removed and given that all the navigation code lives in the parent process now each navigation via Marionette adds another set of listeners. So this can leak quite a good amount of memory.
- Is this code covered by automated tests?: No
- 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): Moves unregistering of listeners into the finally block of the navigation Promise, so it to always gets called whether if the Promise gets resolved or rejected.
- String changes made/needed: No
|
||
Updated•4 years ago
|
|
||
Comment 8•4 years ago
|
||
Comment on attachment 9184567 [details]
Bug 1674158 - [marionette] waitForNavigationCompleted has to clean-up registered listeners.
Fix for an important memory leak for Marionette uses, approved for 83 beta 8, thanks.
|
||
Comment 9•4 years ago
|
||
| bugherder uplift | ||
|
||
Updated•2 years ago
|
Description
•