1135256 - navigator.mozL10n.ready can cause memory leaks

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::L10n, defect, P2)

Description

[David Flanagan [:djf]]

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.

Comment 1

[David Flanagan [:djf]]

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.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 3

[Staś Małolepszy :stas]

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?

Comment 4

[Eitan Isaacson [:eeejay]]

(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'?

Comment 5

[Eitan Isaacson [:eeejay]]

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!

Comment 6

[Eitan Isaacson [:eeejay]]

.. 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();
  }
});

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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?

Comment 8

[Staś Małolepszy :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 :)

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 10

[Eitan Isaacson [:eeejay]]

https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#mozL10n.ready_will_cause_memory_leaks

Comment 11

[Eric Rahm [:erahm]]

Closing old b2g "leak" bugs.