1114746 - Getting dead object error from tab-firefox.js due to a memory leak when requiring 'sdk/tabs' module

Status: RESOLVED DUPLICATE of bug 1086163

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

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Create an add-on with `let tabs = require("sdk/tabs")` open a few tabs, open about:memory, then close the tabs, and use about:memory to check for leaks.

I noticed that when I did this with cfx 1.17 on nightly there were leaks twice out of a few tries.

Comment 2

[Gabriel Ivanica [:givanica]]

The problem can be reproduces 100% of the time if for opening windows instead of tabs.

Steps to reproduce:
  1. create an add-on with 'let tabs = require('sdk/tabs')' (doesn't matter if you use let/var or const)
  2. open around 10 new Firefox windows and then close them (except one)
  3. windows not tabs
  4. check in about:memory for --top(none)/detached
  5. garbage collection or cycle collection or minimize memory usage does nothing

tested with a clean profile on Firefox 34, and Nightly

  6. disable the add-on
  7. performing garbage collection now will free the memory used by those 10 windows

Comment 3

[Gabriel Ivanica [:givanica]]

Attachment: sdk-tabs-memory-leak.png

Memory leak for closed windows

Comment 4

[Alex Verstak]

Simpler instructions to reproduce:

1. cfx init
2. Put this in lib/main.js:

require("sdk/tabs");
require("sdk/panel").Panel({contentURL:"about:blank"});

3. cfx run
4. Open about:addons, disable the addon, then re-enable it.

The console says:

JavaScript error: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///var/folders/00/012br000h01000cxqpysvccm00049g/T/tmpiZbjbV.mozrunner/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/tabs/tab-firefox.js, line 98: TypeError: can't access dead object

It fails in _onContentEvent in tab-firefox.js - probably need to unregister the listener, though destroy() above appears to be trying to do that already.

Comment 5

[Alex Verstak]

Attachment: WIP

The fix works for me in manual testing.  But no love from the test - it passes both with and without the fix.  Posting it anyway, in case it helps someone.

Comment 6

[Alex Verstak]

Attachment: remove event handlers in tab-firefox.js on unload

Ready for review.  The test works now - fails w/o the fix, passes with the fix.  (The trick was to use sdk/system/events instead of LoaderWithHookedConsole.)

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8571800 [details] [diff] [review]
remove event handlers in tab-firefox.js on unload

Passing this to erik for review since I haven't been working on the SDK for a bit

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8571800 [details] [diff] [review]
remove event handlers in tab-firefox.js on unload

This looks alright, the test should not use setTimeout though it appears to me.  Using setTimeout usually causes intermittent failures due to race conditions, and there is almost always another more reliable way to write tests.  In this case I believe the loader.unload() causes a `sdk:loader:destroy` system event to be fired which we should be listening for https://github.com/mozilla/addon-sdk/blob/master/lib/toolkit/loader.js#L748

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

By the way thanks for identifying the issue Alex!

Comment 10

[Alex Verstak]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #8)
> This looks alright, the test should not use setTimeout though it appears to
> me.  Using setTimeout usually causes intermittent failures due to race
> conditions, and there is almost always another more reliable way to write
> tests.  In this case I believe the loader.unload() causes a
> `sdk:loader:destroy` system event to be fired which we should be listening
> for
> https://github.com/mozilla/addon-sdk/blob/master/lib/toolkit/loader.js#L748

I'll give it a spin, though is that actually any better?  The problem here is that the SDK modules are also unregistering themselves on sdk:loader:destroy.  I want my handler to run after everyone else's, so that their unregistration is complete before I start nuking the sandboxes from under them.  setTimeout(..., 0) puts my function at the end of the current event queue, which seems like a reasonable way to do this - there's no actual timing component to it, second argument is zero.  Or I could add a listener on sdk:loader:destroy right before the unload call, and assume they're run FIFO.  Same thing, it seems?  Anyhow, I can change it if you prefer.

BTW, this is only half of the fix - it only cleans stuff up on unload.  There's also the memory leak as you open & close the windows, as described in comment 2.  This patch can't fix that because there's no unload involved.  I was tracking that down too but didn't get very far as of yet...

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alex Verstak from comment #6)
> Created attachment 8571800 [details] [diff] [review]
> remove event handlers in tab-firefox.js on unload
> 
> Ready for review.  The test works now - fails w/o the fix, passes with the
> fix.  (The trick was to use sdk/system/events instead of
> LoaderWithHookedConsole.)

So I'm running this test and I can't seem to be able to get the test to fail by removing the changes to sdk/tabs.

Comment 13

[Alex Verstak]

Attachment: remove event handlers in tab-firefox.js on unload

Added sdk:loader:destroy as requested.  Works / fails reliably for me...  But the previous version did too?  In fact, my onLoad() appears to be called from inside loader.unload(), so I had to stick setTimeout(0) in there anyway, to avoid nuking the sandboxes from under the still-running cleanup code.  I.e., it looks like essentially the same callback sequence as in the previous version.

Perhaps you could submit a try run with the test but without the tab-firefox.js change?  I can't reproduce failure to fail in this case on linux debug; logs might help.  (Sorry, I don't have committer access, so can't do that myself.)

Comment 14

[Alex Verstak]

@Eric - gentle reminder to review the patch in comment 13.  Thanks.

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8577115 [details] [diff] [review]
remove event handlers in tab-firefox.js on unload

This patch appears to be corrupt, and I can't apply it to m-c.

If you could make a pull request here https://github.com/mozilla/addon-sdk/ that would make it much easier to review the patch.

As it stands it appears I would need to manually apply each line of the provided patch because it appears to be corrupt at the top of the file.

This line: "@@ -16,16 +16,17 @@ const { activateTab, getOwnerWindow, get"

Comment 16

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/7d834e869683dbe93ab10f6df3722248f118be99
Bug 1114746 - Getting dead object error from tab-firefox.js

https://github.com/mozilla/addon-sdk/commit/ac087e8c1a59b8688fa38f783ae7b943c5de4612
Bug 1114746 Part 2 - Fix for dead object error from tab-firefox.js r=erikvold

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alex Verstak from comment #14)
> @Eric - gentle reminder to review the patch in comment 13.  Thanks.

I updated the test and landed these changes in https://github.com/mozilla/addon-sdk/pull/1919