Last Comment Bug 672619 - TBPL zombie compartment lasts 15+ minutes with It's All Text extension
: TBPL zombie compartment lasts 15+ minutes with It's All Text extension
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Christian Höltje
:
:
Mentors:
Depends on: 669096
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2011-07-19 13:29 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-01-14 00:20 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Justin Lebar (not reading bugmail) 2011-07-19 13:29:03 PDT
See also bug 671053, which fixed something related to TBPL.

After closing the TBPL tab, the compartment lives for 20+ minutes, despite frequent minimize memory usage calls.
Comment 1 Justin Lebar (not reading bugmail) 2011-07-19 13:39:32 PDT
Ah...this doesn't happen in safe mode.
Comment 2 Justin Lebar (not reading bugmail) 2011-07-19 13:43:52 PDT
And the culprit is ItsAllText.

This is a really simple extension; I wonder what it's doing wrong...
Comment 3 Nicholas Nethercote [:njn] 2011-07-19 16:25:05 PDT
(In reply to comment #0)
> 
> After closing the TBPL tab, the compartment lives for 20+ minutes, despite
> frequent minimize memory usage calls.

I assume this means "it didn't go away, and I observed it for more than 20 minutes"?

I saw the same problem with this add-on, I contacted Christian (the author) rather than filing a bug because it's not a widely-used extension.  But now that we have a bug for it, here's what Christian said:

> I'm in the middle of writing IAT version 2.0, which will work very
> differently and will have all new bugs! :-)
>
> Anyway, I suspect that the main leakage is the
> deleteTemporaryFileOnExit() call I use to clean up old files.
>
> I thought I'd fixed most of the other memory leakage.  I'd love any
> help on figuring out what might be going wrong.  To be honest, I'm
> confused how there can be any leakage, since I expect the javascript
> VM to be purged when a page closes.  But I guess Firefox doesn't work
> that way.
 
and later:

> With the new version, all the stateful stuff will either be in the
> page it self or on the editor daemon.  No more "backend" that saves
> any state.
>
> That should make both security and memory easier to figure out.
Comment 4 Justin Lebar (not reading bugmail) 2011-07-19 16:31:52 PDT
> Anyway, I suspect that the main leakage is the
> deleteTemporaryFileOnExit() call I use to clean up old files.

I presume he injects this into the page's onunload handler or something?  So could this be bug 669096?
Comment 5 Christian Höltje 2011-08-11 12:30:10 PDT
(In reply to Nicholas Nethercote [:njn] from comment #3)
> I saw the same problem with this add-on, I contacted Christian (the author)
> rather than filing a bug because it's not a widely-used extension.  But now
> that we have a bug for it, here's what Christian said:

Oh, cool. Someone filed a bug IAT.

The source code is here https://github.com/docwhat/itsalltext

Anytime someone clicks the "edit" button, the deleteTemproraryFileOnExit() is called on the newly created file.  See https://github.com/docwhat/itsalltext/blob/master/src/chrome/content/cacheobj.js#L324

In addition, there is console.log() calls, if someone has firebug installed and debugging turned on (defaults to off).

So both sound like bug 669096 could be causing trouble.  This also explains why the leaks have been hard to track down in the past.
Comment 6 Christian Höltje 2011-08-12 15:30:30 PDT
Can someone point me at some documentation for what a TBPL is and how I can debug this?

Even though I'm working on a new version, if I can fix this now I'd like to.

I do inject XHTML into the document and add some listeners.  Other than that, all that happens is firing off system execute commands, reading/writing files, and of course the deleteTemporaryFileOnExit() call.

If I can reproduce this problem here, then I can see what might be calling it.

Specifically, I wonder if it's a problem only if textareas are edited, or simply displayed.
Comment 7 Andrew McCreight [:mccr8] 2011-08-12 15:31:50 PDT
TBPL is tbpl.mozilla.org
Comment 8 Christian Höltje 2011-08-12 15:37:23 PDT
(In reply to Andrew McCreight [:mccr8] from comment #7)
> TBPL is tbpl.mozilla.org

It's a website with the tinderbox logs on it?  So what would a compartment be (zombie or not)?
Comment 9 Nicholas Nethercote [:njn] 2011-08-12 15:40:18 PDT
AIUI: if you have IAT installed, if you visit tbpl.mozilla.org and then close it, the tbpl.mozilla.org compartment hangs around forever.  You can see it in about:memory.
Comment 10 Christian Höltje 2011-08-13 18:40:48 PDT
W00T!  The new memory compartment stuff is really really helpful!

I've had off-and-on complaints over the years and using the nightlies this is the first time I could *see* the problems.

I've released It's All Text! 1.6.0 which no longer has zombie compartments!  It's still pending approval but can be gotten here:
https://addons.mozilla.org/en-US/firefox/addon/its-all-text/versions/

Loading TBPL and then closing it causes the compartment to shrink and eventually disappear (about 20 seconds or so).

There is still a leak if you enable debugging (about:config -> extensions.itsalltext.debug = true) and have Firebug, but I understand that's a known issue?

Rough summary of what was wrong:
 * To track all the textareas to put text back in them I had a structure in the extension itself called "tracker".
 * This tracker contained "cacheobjs" (badly named) that had variables set to elements of the page (the edit button and the textareas).

After trying several different approaches, I figured out how to fix it.  I switched from a global tracker to per-document trackers by using gBrowser.contentDocument.setUserData('tracker',....) so that it goes away when the document goes away.

Thanks for the help with this!
Comment 11 Nicholas Nethercote [:njn] 2011-08-13 18:49:11 PDT
Great news!

> Rough summary of what was wrong:
>  * To track all the textareas to put text back in them I had a structure in
> the extension itself called "tracker".
>  * This tracker contained "cacheobjs" (badly named) that had variables set
> to elements of the page (the edit button and the textareas).

So bug 669096 was not to blame then?
Comment 12 Christian Höltje 2011-08-13 19:05:17 PDT
(In reply to Nicholas Nethercote [:njn] from comment #11)
> So bug 669096 was not to blame then?

Only with It's All Text! debugging set to 'true'.  With it set to 'false', it was not the fault of bug 669096.

Debugging can only be turned on via about:config in the latest versions (since 1.2-ish, I think). But if anyone turned it on before version 1.2 it may still be 'true'.
Comment 13 Nicholas Nethercote [:njn] 2011-08-13 20:21:50 PDT
I think we can close this.
Comment 14 Jesse Ruderman 2011-08-17 18:12:48 PDT
> After trying several different approaches, I figured out how to fix it.  I
> switched from a global tracker to per-document trackers by using
> gBrowser.contentDocument.setUserData('tracker',....) so that it goes away
> when the document goes away.

This worries me from a security perspective. Web pages can see and modify these user-data objects, possibly exposing sensitive information or APIs, or confusing the extension when it reads the object back.

It's also going to conflict with any web page that happens to have a user data blob with the key "tracker".

Have you tried using a document-keyed WeakMap instead?

> Debugging can only be turned on via about:config in the latest versions 
> (since 1.2-ish, I think). But if anyone turned it on before version 1.2 it 
> may still be 'true'.

If you think a large number of users have the pref set for reasons that are no longer valid, I'd suggest renaming the pref. We've done this in Firefox a few times.
Comment 15 Christian Höltje 2011-08-19 12:36:19 PDT
(In reply to Jesse Ruderman from comment #14)
> > After trying several different approaches, I figured out how to fix it.  I
> > switched from a global tracker to per-document trackers by using
> > gBrowser.contentDocument.setUserData('tracker',....) so that it goes away
> > when the document goes away.
> 
> This worries me from a security perspective. Web pages can see and modify
> these user-data objects, possibly exposing sensitive information or APIs, or
> confusing the extension when it reads the object back.

It worries me too, but I didn't know of any other way or place to store this stuff...

> It's also going to conflict with any web page that happens to have a user
> data blob with the key "tracker".

The key name is actually something like "itsalltext_tracker".... Would it help if I generated UUID and used that for the key? I could store the UUID key in the preferences.

> Have you tried using a document-keyed WeakMap instead?

I'm unfamiliar with WeakMap.... It says that it's a prototype and is only available with FF6 (which just came out this week).

Is it safe to use?

> > Debugging can only be turned on via about:config in the latest versions 
> > (since 1.2-ish, I think). But if anyone turned it on before version 1.2 it 
> > may still be 'true'.
> 
> If you think a large number of users have the pref set for reasons that are
> no longer valid, I'd suggest renaming the pref. We've done this in Firefox a
> few times.

I was thinking of adding a version number  preference and resetting debug to false if the version number increments....

I can't think of any reason a non-developer would want the debug to stay on across a new version that turning it back on would be a big deal.
Comment 16 Jesse Ruderman 2011-08-19 22:45:46 PDT
On IRC, we discussed the use of WeakMap in this extension. We found that using WeakMap (with document keys and primitive values) currently leaks :(

Can this be explained as a combination of known WeakMap bugs (e.g. bug 653248, bug 668855, bug 673468)? Or should we file another bug report?
Comment 17 Nicholas Nethercote [:njn] 2011-08-20 02:45:33 PDT
CC'ing gal and dherman, both of whom (I think?) know about WeakMaps (see comment 16).
Comment 18 Nicholas Nethercote [:njn] 2011-08-20 02:46:10 PDT
[Actually CC'ing them this time!]
Comment 19 Andrew McCreight [:mccr8] 2011-08-20 06:34:21 PDT
(In reply to Jesse Ruderman from comment #16)
> Can this be explained as a combination of known WeakMap bugs (e.g. bug
> 653248, bug 668855, bug 673468)? Or should we file another bug report?

That's probably an instance of bug 668855.  It would be nice to have a little test case, though, either here or in that bug.  Maybe I can put one together myself.

I'm going to focus more on WeakMap fixes, after I get some more done on a few other leak fixes, as WeakMaps seem to be insanely popular.

Note You need to log in before you can comment on or make changes to this bug.