Closed Bug 672619 Opened 13 years ago Closed 13 years ago

TBPL zombie compartment lasts 15+ minutes with It's All Text extension

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: docwhat)

References

Details

(Whiteboard: [MemShrink:P3]) 

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.
Whiteboard: [MemShrink]
Ah...this doesn't happen in safe mode.
And the culprit is ItsAllText.

This is a really simple extension; I wonder what it's doing wrong...
Summary: TBPL zombie compartment lasts 15+ minutes (maybe never goes away) → TBPL zombie compartment lasts 15+ minutes with ItsAllText extension
Whiteboard: [MemShrink] → [MemShrink:P3]
(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.
> 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?
Summary: TBPL zombie compartment lasts 15+ minutes with ItsAllText extension → TBPL zombie compartment lasts 15+ minutes with It's All Text extension
(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.
Depends on: 669096
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.
TBPL is tbpl.mozilla.org
(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)?
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.
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!
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?
(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'.
I think we can close this.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
> 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.
(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.
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?
CC'ing gal and dherman, both of whom (I think?) know about WeakMaps (see comment 16).
[Actually CC'ing them this time!]
(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.
Assignee: nobody → docwhat
You need to log in before you can comment on or make changes to this bug.