Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

unspecified
mozilla39
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments)

Requiring sdk/tab/events and then opening a new tab causes a leak for the lifetime of the app.
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)
Posted file pull request
Attachment #8579558 - Flags: review?(evold)
Comment on attachment 8579558 [details] [review]
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] [review]
> 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] [review]
> > 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] [review]
pull request

Added some tests, they fail without the changes and pass with.
Attachment #8579558 - Flags: review- → review?(evold)
Comment on attachment 8579558 [details] [review]
pull request

Awesome!
Attachment #8579558 - Flags: review?(evold) → review+

Comment 8

4 years ago
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.
Assignee

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Closed: 4 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: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.