Closed Bug 1144742 Opened 10 years ago Closed 9 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
819 bytes, patch
evold
: review+
Details | Diff | Splinter Review
pull request
46 bytes, text/x-github-pull-request
evold
: review+
evold
: feedback+
Details | Review 
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)
Attached 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+

    
    

    
  
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: 9 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: 9 years ago → 9 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: