navigator.mozL10n.ready can cause memory leaks

RESOLVED WONTFIX

Status

defect
P2
normal
RESOLVED WONTFIX
4 years ago
2 years ago

People

(Reporter: djf, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
In recent RTL and accessibility bugs affecting the Gallery app, we've needed to re-localize things when the language changes, and have ended up with patches that call navigator.mozL10n.ready() from the constructor of objects like shared/js/media/MediaFrame.js.  (See bug 1068976).  I gave the patch an r+ before I realized that it was a memory leak and changed to an r-.

The problem is that l10n.js has an EventEmitter class that handles event listeners with an ordinary array. Any function passed to ready() will be added to that array and will never be removed.  If the function is bound to an object (as they usually are) then that object can never be garbage collected.

If I'm understanding this right, it means that navigator.mozL10n.ready() should only be called from top-level code in an app, never as part of objects that may be created and destroyed repeatedly.  

If I'm right, then we need, at a minimum, to document that fact clearly.

It would be better if we could actually fix the problem. Maybe using a WeakSet instead of an array? Or maybe by using a dummy DOM element intead of EventEmitter. I'm not actually sure whether either of those fixes would actually solve the problem.
(Reporter)

Comment 1

4 years ago
After further thought, I've convinced myself that this problem can not be solved.  But we should probably document it.

Avoiding memory leaks is presumably a reason why apps might want to use window.addEventListener('localized', f) instead of ready(f) because then they can use window.removeEventListener to remove the function.
We definitely should document this.

I've come accross this while designing the API and solved it by using ready and removeEvenListener together:

Init: function () {
  mozL10n.ready(start);
},
Cleanup: function() {
  mozL10n.ctx.removeEventListener(start);
}

Not the cleanest but not sure what is
An alternative would be to call mozL10n.once() from the constructor to localize the object upon its creation, and keep track of current instances manually.  Then you can do:

  mozL10n.ready(function() {
    App.myObjects.forEach(localize);
  });

Clearly this is also suboptimal, because you need to keep App.myObjects up-to-date, but maybe you're already doing that?
(In reply to David Flanagan [:djf] from comment #0)
> It would be better if we could actually fix the problem. Maybe using a
> WeakSet instead of an array? Or maybe by using a dummy DOM element intead of
> EventEmitter. I'm not actually sure whether either of those fixes would
> actually solve the problem.

Weak{Set,Map} are not iterable, so I don't think they would work for maintaining a weak listener list. But you probably figured that out!

(In reply to Zibi Braniecki [:gandalf] from comment #2)
> We definitely should document this.
> 
> I've come accross this while designing the API and solved it by using ready
> and removeEvenListener together:
> 
> Init: function () {
>   mozL10n.ready(start);
> },
> Cleanup: function() {
>   mozL10n.ctx.removeEventListener(start);
> }
> 
> Not the cleanest but not sure what is

What would make it cleaner is if removeEventListener was made public. Are we allowed to use 'ctx'?
Another thought I had is that the majority of cases you would want to call ready() is for formatting dates and nested translations. If it is a simply l10n-id with some args, you don't need to handle it in js. So if there was a good solution for nesting and passing date objects in arguments, it could be good too!
.. and another solution with WeakMap for a singleton listener:

var instances = new WeakMap();

function MyThing(container) {
  container.classList.add('my-thing');
  instances.set(container, this);
}

MyThing.prototype.localize = function localize() { /* ... */ };

navigator.mozL10n.ready(function () {
  for (var container of document.querySelectorAll('.my-thing')) {
    var obj = instances.get(container);
    if (obj) obj.localize();
  }
});
(In reply to Eitan Isaacson [:eeejay] from comment #4)
> What would make it cleaner is if removeEventListener was made public. Are we
> allowed to use 'ctx'?

That's a great question. We did not expose it on purpose, and the fact that ready() is using ctx.eemitter is an implementation detail, so probably not.

But we probably should expose something that cleans up after mozL10n.ready.

Stas, do you have any opinions on how it should look like?
Flags: needinfo?(stas)
I think I'd prefer to not extend the current API right now, but I'm not strongly opinionated.  I actually like eeejay's solution in comment 6 which is something I tried to describe in my comment 3.  Bonus points for using WeakMap :)
Flags: needinfo?(stas)
Great!

Stas or Eitan - would you be willing to describe it in https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices for people to use properly?

Updated

4 years ago
OS: Mac OS X → Gonk (Firefox OS)
Priority: -- → P2
Hardware: x86 → ARM
Closing old b2g "leak" bugs.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.