1001833 - "TypeError: can't access dead object" due leaks of event/dom and event/chrome modules

Status: RESOLVED FIXED

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

Description

[Matteo Ferretti [:zer0] [:matteo]]

The ActionButton and ToggleButton seems have a leak somewhere; probably there is a clean up that is not made properly. Just have an add-on with only the following line:

  require("sdk/ui/button/action");

Once disabled, clicks to any button on the toolbar (e.g. Home Button) and the console will log "JavaScript error: , line 0: can't access dead object".

It seems a listener that is still alive, probably related to CustomizableUI. I'm working on it.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

After some investigation, it seems that the `open` function of `event/dom` module is the culprit: it's adding listener to a DOM element – in our case, a window - without remove them once the add-on is unloaded.
Removing those listeners during the unload of the add-on, fixes the issue.

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: pull/1476

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8413326 [details] [review]
pull/1476

Denying the review because it will likely introduce flip side issue where dead object errors will be logged on add-on unload for windows that have being GC-ed.

Comment 5

[Alan]

I'm experiencing this bug on x86 windows as well.

Comment 6

[u279076]

Is there any update on this bug? It's blocking some of our Australis automation work.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8413326 [details] [review]
pull/1476

Assign the new review to someone else 'cause Irakli has days off.

Notice, for the reviewer:

> Denying the review because it will likely introduce flip side issue 
> where dead object errors will be logged on add-on unload for windows
> that have being GC-ed.

That wasn't actually true, the "dead object" exception was generated by the event/chrome module, that the previous patch didn't considered.
However, the previous code didn't raise "dead object" exception in such case only because a memory leak: the `window` reference was kept alive even if the window itself was closed. This patch trying to avoid this memory leaks as well.

Comment 8

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

Added some comments, anyway we can test this? Seems like some of the logic is reversed, too

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)
> Added some comments, anyway we can test this? Seems like some of the logic
> is reversed, too

I fixed the reversed logic, and also added a test, that I checked both with and without the fix. I'm not sure if it's the best way to test it, I feel like the `nuke` part could be maybe dangerous to use.
BTW, ready for the second round of review.

Comment 10

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/21496d5835ec5c1d65bcb24fad57f6c20ea6db40
Bug 1001833 - "TypeError: can't access dead object" due leaks of event/dom and event/chrome modules

- Ensured listeners and observers are removed once the add-on is disabled / uninstalled

https://github.com/mozilla/addon-sdk/commit/461e57c8221aaab907e56c9e9de131629e8ae39c
Merge pull request #1476 from ZER0/dead-object/1001833

fix Bug 1001833 - "TypeError: can't access dead object" due leaks of event/dom module r=@jsantell