1086613 - RegisteredActorFactory uses wrong constructor name

Status: RESOLVED WONTFIX

(DevTools :: General, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(follow up for bug 977443)

The current actor registration uses actor constructor name to identify the actor. The name is used later when the actor is unregistered (by removeTabActor or removeGlobalActor).

The problem is that actors are created using Class object (coming from sdk/core/heritage module) that uses implicit constructor function. The name of this constructor is always "constructor"
https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/core/heritage.js#L143

The ctor name is used by RegisteredActorFactory (in common.js)
https://github.com/mozilla/gecko-dev/blob/fx-team/toolkit/devtools/server/actors/common.js#L60

(the options argument passed in the RegisteredActorFactory function is the actor constructor created by Class() object e.g. MyActor)

https://github.com/mozilla/gecko-dev/blob/fx-team/toolkit/devtools/server/main.js#L913

An example:

// ActorClass uses Class (see protocol.js)
let MyActor = protocol.ActorClass({
  typeName: "myactor",
});


// Registration: factory is e.g. MyActor 
DebuggerServer.addGlobalActor(factory, name);

https://github.com/mozilla/gecko-dev/blob/fx-team/toolkit/devtools/server/main.js#L133

Later it's used to unregister the actor:

// factory is e.g. MyActor 
DebuggerServer.removeTabActor(factory);

https://github.com/mozilla/gecko-dev/blob/fx-team/toolkit/devtools/server/main.js#L138


Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Any tips how to solve this?

Should we fix protocol.js or heritage.js?

Honza

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

I don't know `protocol.js` neither the `actors` well, probably Irakli knows better to reply properly, but to me the approach used by protocol.js is too fragile. It shouldn't rely on function's name.
Plus, I'm not sure how to "fix" it on heritage side, anything I can thinking is a sort of hack. We could either:

- Use a property to identify the constructor's name
  
  let Dog = Class({
    constructorName: 'Dog',
    initialize: function initialize(name) {
      this.name = name;
    },
    type: 'dog',
    bark: function bark() {
      return 'Ruff! Ruff!'
    }
  });

- Reuse the `initialize` name: 

  let Dog = Class({
    initialize: function Dog(name) {
      this.name = name;
    },
    type: 'dog',
    bark: function bark() {
      return 'Ruff! Ruff!'
    }
  });

(but then I'm not sure about class like `Disposable`, that use internally `initialize` and exposes `setup`, for example)

In my humble opinion, we should fix protocol.js, or avoid to use heritage's classes in this scenario, and just use the plain constructor's syntax for such actors. But I don't know in first place why it was decided to use the function's name to register that stuff, instead of a Map or a Set, for instance; maybe there is a reason that makes the function's name mandatory to be used.

Comment 3

[Alexandre Poirot [:ochameau]]

I'm not sure protocol.js really needed to use heritage as I don't think it rely on multiple inheritance,
but now it is quite late in process to refactor all the codebase just for that.

Using initialize function name as constructor name doesn't look that bad.
But as Honza said, we can workaround that in devtools codebase if you think it's not worth fixing anything in heritage.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I don't think there is much point in fixing heritage now that we're close to having native class syntax in JS, which I believe will have the same issue as pointed out here. So I think it would be best to use different field field for identifying than constructor.name

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Actor (factory) registration has been refactored and this bug is not longer needed.

Honza