Closed Bug 1144742 Opened 10 years ago Closed 10 years ago

sdk/tab/events leaks

Categories

(Add-on SDK Graveyard :: General, defect)

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(2 files)

test harness patch
(deleted), patch
evold
: review+
Details | Diff | Splinter Review
pull request
(deleted), text/x-github-pull-request
evold
: review+
evold
: feedback+
Details
Requiring sdk/tab/events and then opening a new tab causes a leak for the lifetime of the app.
Attached patch test harness patch (deleted) — — Splinter Review
Not strictly necessary but unloading the test harness loader after tests are complete helps a lot with tracking real leaks with heapgraph.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #8579555 - Flags: review?(evold)
Attached file pull request (deleted) —
Attachment #8579558 - Flags: review?(evold)
Comment on attachment 8579558 [details] pull request Looks good, it needs a test though.
Attachment #8579558 - Flags: review?(evold)
Attachment #8579558 - Flags: review-
Attachment #8579558 - Flags: feedback+
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #3) > Comment on attachment 8579558 [details] > pull request > > Looks good, it needs a test though. I'm not sure what test I could write here, what did you have in mind?
Flags: needinfo?(evold)
(In reply to Dave Townsend [:mossop] from comment #4) > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #3) > > Comment on attachment 8579558 [details] > > pull request > > > > Looks good, it needs a test though. > > I'm not sure what test I could write here, what did you have in mind? We could access the sandbox scope and check that `loader.sandbox("sdk/event/dom").listeners.keys().length` is 0. Or include an "end" event. If there is a way to check how many listeners a window has that would be better. I'm open to other options.
Flags: needinfo?(evold)
Comment on attachment 8579558 [details] pull request Added some tests, they fail without the changes and pass with.
Attachment #8579558 - Flags: review- → review?(evold)
Comment on attachment 8579558 [details] pull request Awesome!
Attachment #8579558 - Flags: review?(evold) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/6b627247edf7340843a455bbb319ca73461c05e2 Bug 1144742: Clear promise debugging observer after tests and clear all added DOM event listeners on unload.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #8579555 - Flags: review?(evold) → review+
Oh right, I need to land the harness patch too
Status: RESOLVED → REOPENED
Flags: needinfo?(dtownsend)
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/a6c42747e5bc
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: