1086163 - Disabling an add-on (e.g. Gecko Profiler) causes "can't access dead object" failures

Status: RESOLVED INCOMPLETE

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

Description

[Tim Taubert [:ttaubert] (inactive)]

Installing the Gecko Profiler, restarting, and then disabling it causes a few error messages until shutdown. It seems like tab-firefox.js is holding onto |_browser|.

JavaScript error: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/tim/.mozilla/firefox/0at5j95f.trunk/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

This hints to the |TabTrait| still being alive and not getting destroyed properly.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Indeed, none of the TabTraits is being destroyed when the add-on is disabled. So it's not the |_browser| being held onto but that the whole code is still alive at all. We should of course destroy all TabTraits when the add-on goes away.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I've been forgetting to log this bug for over a week. Annoying, yes.

Comment 7

[Alex Verstak]

Bug 1114746 has a working patch for this, I think.  (I posted it there before realizing it was marked as a dup, which is probably only half-true anyway...)

Comment 8

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/9340d4bf7ffcb5633c50347c4c4095ea970ca55b
Bug 1086163 Using a different method to check for the leak, and moving the test to a Fx specific test file a=me

https://github.com/mozilla/addon-sdk/commit/b5367d91431cd498707fbf2121586723e9778199
Merge pull request #1919 from erikvold/1086163p3

Bug 1086163 tab-firefox.js was leaking tab objects when the add-on unloads r=erikvold

Comment 9

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

(In reply to [github robot] from comment #8)
> Commits pushed to master at https://github.com/mozilla/addon-sdk
> 
> https://github.com/mozilla/addon-sdk/commit/
> 9340d4bf7ffcb5633c50347c4c4095ea970ca55b
> Bug 1086163 Using a different method to check for the leak, and moving the
> test to a Fx specific test file a=me
> 
> https://github.com/mozilla/addon-sdk/commit/
> b5367d91431cd498707fbf2121586723e9778199
> Merge pull request #1919 from erikvold/1086163p3
> 
> Bug 1086163 tab-firefox.js was leaking tab objects when the add-on unloads
> r=erikvold

I think this patch, originally from bug Bug 1114746, should fix the issue here, once this is uplifted we should verify that is the case.

Comment 10

[jacek]

Is there any workaround to avoid this error on current stable firefox version? (without having the newest release with given pull requrest merged in)

Comment 11

[Alex Verstak]

(In reply to jacek from comment #10)
> Is there any workaround to avoid this error on current stable firefox version?

You could bundle the SDK with your addon?  I don't know if cfx supports this, but it should be possible in principle.

BTW, bug 1114746 also reports a memory leak that the unload patch doesn't fix.  This seems like a more serious issue than an error message in the console.  The only workaround I can think of is to avoid sdk/tabs and use the lower-level APIs directly.

Neither of these options seem particularly satisfying.  Option 3 is to ignore the problems and ship it with the leak and the error message. ;-)

Comment 12

[jacek]

(In reply to Alex Verstak from comment #11)
> (In reply to jacek from comment #10)
> > Is there any workaround to avoid this error on current stable firefox version?
> 
> You could bundle the SDK with your addon?  I don't know if cfx supports
> this, but it should be possible in principle.

Seems like it's not possible - from what I can read on https://developer.mozilla.org/en-US/Add-ons/SDK/Tutorials/Installation: "If you use the latest development version, you must use it with the Nightly version of Firefox, and you won't be able to submit any add-ons you develop to addons.mozilla.org (AMO), because AMO requires that you use an official release.".
 
> BTW, bug 1114746 also reports a memory leak that the unload patch doesn't
> fix.  This seems like a more serious issue than an error message in the
> console.  The only workaround I can think of is to avoid sdk/tabs and use
> the lower-level APIs directly.

What lower-level APIs do you mean? You mean the XUL / XPCOM APIs? 
 
> Neither of these options seem particularly satisfying.  Option 3 is to
> ignore the problems and ship it with the leak and the error message. ;-)

I don't think it's an option, as the memory leak (it keep reference to all the tabs) is pretty serious.

Comment 13

[zepeuj]

(In reply to jacek from comment #12)
> (In reply to Alex Verstak from comment #11)
> > (In reply to jacek from comment #10)
> What lower-level APIs do you mean? You mean the XUL / XPCOM APIs? 

He means this: https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/tabs_utils

But for me it's not a solution anyway.

Comment 14

[Alex Verstak]

(In reply to jacek from comment #12)
> > Option 3 is to
> > ignore the problems and ship it with the leak and the error message. ;-)
> 
> I don't think it's an option, as the memory leak (it keep reference to all
> the tabs) is pretty serious.

If this bug is a blocker for new add-ons, then one would think it should be fixed by now?  It seems like the sort of thing that's likely to affect lots of existing add-ons, including more widely used ones than yours or mine.

Anyhow, I submitted my add-on for AMO review and included a pointer to this bug.  There're four possible outcomes:

1. My add-on gets approved as-is, which would mean that AMO doesn't consider this a blocker.  If that's their judgement call, I'll probably just go ahead and release it, and then look into fixing the leak in the SDK as my time and skill level allow.

2. AMO finds that this bug affects some important add-ons, and gets it fixed.  My add-on is not that important, so it can take advantage of the fix whenever it rolls out.

3. AMO finds that this bug doesn't affect any important add-ons.  In that case, it would be fair to require me to switch to some lower-level API.  Or perhaps they would even be kind enough to suggest some work-around that I didn't think of.

4. AMO denies my add-on without investigating the broader impact of this bug, or regardless of the fact that this bug affects more important add-ons than mine.  I would consider this unfair, but switching to a lower-level API would still be an option in this case.

So, FWIW, option 3 doesn't sound all that bad to me.  But I'm just an external developer, not affiliated with Mozilla, so please don't take my opinion very seriously. :-)

Comment 15

[Alex Verstak]

(In reply to zepeuj from comment #13)
> https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/tabs_utils
> 
> But for me it's not a solution anyway.

Or require("chrome") and have fun with XPCOM:

https://developer.mozilla.org/en-US/Add-ons/SDK/Tutorials/Chrome_Authority

Both of these are harder to use and change more frequently than the stable SDK modules, but they should let you do anything you want.  If you're running scripts over content, see:

https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/The_message_manager
https://developer.mozilla.org/en-US/docs/Xray_vision

Comment 16

[christopherfitzpatrick23]

Attachment: tmp_21405-EICCCodeofConduct5_1_English-1631957414.pdf

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8874280 [details] [diff] [review]
tmp_21405-EICCCodeofConduct5_1_English-1631957414.pdf

Please don't set flags if you don't know what you're doing.

Comment 18

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

Comment on attachment 8874280 [details] [diff] [review]
tmp_21405-EICCCodeofConduct5_1_English-1631957414.pdf

I'm not sure what this PDF is

Comment 19

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562