1313767 - Deobfuscate (and consequentially optimize) core SDK modules

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Kris Maglione [:kmag]]

The obfuscated, hyper-"functional" style of some core SDK modules causes a measurable performance hit. The simpler versions are much more performant.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1313767: Deobfuscate events/core.js

Review commit: https://reviewboard.mozilla.org/r/89388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89388/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1313767: Deobfuscate core/heritage.js

Review commit: https://reviewboard.mozilla.org/r/89390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89390/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89388/diff/1-2/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8805676 [details]
Bug 1313767: Deobfuscate core/heritage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89390/diff/1-2/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

https://reviewboard.mozilla.org/r/89388/#review91238

The "event/core.js" looks much much better, but before landing this changes I'd like to ask you some additional details and your feelings about some the subtle differences between the two implementations (and I'm a bit concerned about disabling one more test from the SDK test suite, I'm wondering how it behaves for you locally, and in the meantime I'm going to attach a reworked version of that test).

::: addon-sdk/source/lib/sdk/event/core.js:18
(Diff revision 2)
> -// Utility function to access given event `target` object's event listeners for
> -// the specific event `type`. If listeners for this type does not exists they
> +// Count of total listeners ever added.
> +let listenerCount = 0;

it would be nice to add to the inline comment a note about why we are keep this counter (which seems to be related to line 105)

::: addon-sdk/source/lib/sdk/event/core.js:22
(Diff revision 2)
>  
> -// Utility function to access given event `target` object's event listeners for
> -// the specific event `type`. If listeners for this type does not exists they
> -// will be created.
> -const observers = function observers(target, type) {
> -  if (!target) throw TypeError("Event target must be an object");
> +// Count of total listeners ever added.
> +let listenerCount = 0;
> +
> +const observers = new DefaultWeakMap(() => {
> +  return new DefaultMap(() => new Map());

How about moving this validation:

```
if (!target) 
  throw new TypeError("Event target must be an object");
```

into the `on`/`off`/`once` methods?

::: addon-sdk/source/lib/sdk/event/core.js:39
(Diff revision 2)
>   */
>  function on(target, type, listener) {
>    if (typeof(listener) !== 'function')
>      throw new Error(BAD_LISTENER);
>  
> -  let listeners = observers(target, type);
> +  observers.get(target).get(type).set(listener, listenerCount++);

If I'm not reading this wrong, if the same listener is added more then once, it can be detected as a "late added" listener and skipped by line 105.

::: addon-sdk/source/lib/sdk/event/core.js:102
(Diff revision 2)
> -      let listener = listeners[index];
> -      // Dispatch only if listener is still registered.
> -      if (~state.indexOf(listener))
> +      // Since our contract unfortuantely requires that we not dispatch to
> +      // this event to listeners that were either added or removed during this
> +      // dispatch, we need to check when each listener was added.
> +      if (added >= count)
> +        break;

None of the tests seems to have any issue with the following difference in the behavior of the new `emitOnObject`, but if I'm not reading this wrong, the previous version was skipping any listener that:

- was added after after the last element that was there when line 100 retrieved the length of the listeners array
- was removed before line 111 has been executed for it

while the new version is:

- skipping any listener that has been added after line 99 retrieved the value of listenerCount (and as commented above, any listener that has been added again after that listenerCount value)

- not skipping any listner that has been removed before line 105 is executed for it

and so it seems that line 105 could be changed into something like to match the previous contract:

```js
      // Since ...
      // ... was added and if is still in the listener map 
      if (added >= count || !listeners.has(listener)) {
      
      }
```

::: addon-sdk/source/lib/sdk/event/core.js:112
(Diff revision 2)
> -      if (type !== 'error') emit(target, 'error', error);
> -      else console.exception(error);
> -    }
> -    index++;
> +      if (type !== 'error')
> +        emitOnObject(target, 'error', target, error);
> +      else
> +        console.exception(error);
> -  }
> +    }
> +
> -   // Also emit on `"*"` so that one could listen for all events.
> +  // Also emit on `"*"` so that one could listen for all events.
> -  if (type !== '*') emit(target, '*', type, ...args);
> +  if (type !== '*' && allListeners.get('*').size)
> +    emitOnObject(target, '*', target, type, ...args);

This is an issue that the old code already presents, but I want to point it out given that we are looking into this method:

if a listener registered to \`"\*"\` raises an exception, \`emitOnObject\` is going to enter into a recursion loop (I briefly tried locally and it seems that sometimes it achieves to raise a "Too much recursion" exception at some point, and sometimes it seems to remain stuck in the recursion).

::: addon-sdk/source/lib/sdk/tabs/tabs-firefox.js:104
(Diff revision 2)
> -// Export a new object with allTabs as the prototype, otherwise allTabs becomes
> -// frozen and addListItem and removeListItem don't work correctly.
> +module.exports = allTabs;
> +pipe(tabEvents, allTabs);

this is related to the change to the `loader` that is in the same patch, am I right?

is this necessary to the deobfuscate the `"events/core"` module?

::: addon-sdk/source/lib/sdk/util/object.js:44
(Diff revision 2)
> +
> +    return super.get(key);
> +  }
> +}
> +
> +Object.assign(exports, {DefaultMap, DefaultWeakMap});

Why not just: 

```js
...
exports.DefaultMap = DefaultMap;
...
exports.DefaultWeakMap = DefaultWeakMap;
```

like in the rest of this module?

::: addon-sdk/source/lib/toolkit/loader.js:549
(Diff revision 2)
>      if (err) {
>        throw err;
>      }
>    }
>  
> -  if (module.exports && typeof(module.exports) === 'object')
> +  if (module.exports === originalExports)

This seems to be related to the change in `lib/sdk/tabs/tabs-firefox.js`, can we add a comment here about it?

probably it is not something that would affect how addons are using the module.exports object, but in my opinion it is a difference that can be worth an explicit mention during the review of this patch:

it seems that we are not going to freeze the export object anymore if it has been entirely reassigned, the freezed object couldn't be changed, while if we do not freeze it anymore, a change made to the export object will be reflected into the original object (and anyone that have a reference to it, it is going to access the updated object).

::: addon-sdk/source/test/tabs/test-firefox-tabs.js:1221
(Diff revision 2)
>  };
>  
>  // related to bugs 922956 and 989288
>  // https://bugzilla.mozilla.org/show_bug.cgi?id=922956
>  // https://bugzilla.mozilla.org/show_bug.cgi?id=989288
> -exports["test tabs ready and close after window.open"] = function*(assert, done) {
> +if (0) exports["test tabs ready and close after window.open"] = function*(assert, done) {

It is weird, but this test case doesn't pass for me locally even without the other changes in this patch.

After looking into it a bit, I'm now wondering how it ever been able to work.

Anyway I would be much more happy if we can fix the test, and given that I know how frustrating can be handling the SDK test suite, I'm attaching a reworked version of it that pass (like we use to say here "trouble shared is trouble halved" :-)), on both the old and the new version of this modules.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8805676 [details]
Bug 1313767: Deobfuscate core/heritage.js

https://reviewboard.mozilla.org/r/89390/#review90994

Thanks Kris, this module is definitely much more readable now.

I'm wondering: is there any relationship/dependency between these two patches, or this can land separately from the first patch in this mozreview request?

I'm asking because it looks like the new cleaned version of `Class` and its related helpers match the expected behavior of the previous version quite well, and that none of the tests would fail with this refactoring applied.

::: addon-sdk/source/lib/sdk/core/heritage.js:125
(Diff revision 2)
> -    // Create array of property descriptors who's properties will be defined
> +  // Create array of property descriptors who's properties will be defined
> -    // on the resulting prototype. Note: Using reflection `concat` instead of
> -    // method as it may be overridden.
> -    var descriptors = concat(descriptor.implements, options, descriptor, {
> +  // on the resulting prototype.
> +  var descriptors = [].concat(descriptor.implements, options, descriptor,
> +                              { constructor });
> -      constructor: constructor
> -    });
>  
> -    // Note: we use reflection `apply` in the constructor instead of method
> +  // Note: we use reflection `apply` in the constructor instead of method
> -    // call since later may be overridden.
> +  // call since later may be overridden.
> -    function constructor() {
> +  function constructor() {
> -      var instance = create(prototype, attributes);
> -      if (initialize) apply(initialize, instance, arguments);
> +    var instance = Object.create(prototype, attributes);
> +    if (initialize)
> +      Reflect.apply(initialize, instance, arguments);
> -      return instance;
> +    return instance;
> -    }
> +  }
> -    // Create `prototype` that inherits from given ancestor passed as
> +  // Create `prototype` that inherits from given ancestor passed as
> -    // `options.extends`, falling back to `Type.prototype`, implementing all
> +  // `options.extends`, falling back to `Type.prototype`, implementing all
> -    // properties of given `options.implements` and `options` itself.
> +  // properties of given `options.implements` and `options` itself.
> -    var prototype = extend(descriptor.extends, mix.apply(mix, descriptors));
> +  var prototype = Object.create(descriptor.extends,
> +                                getOwnPropertyDescriptors(...descriptors));
> -    var initialize = prototype.initialize;
> +  var initialize = prototype.initialize;
>  

it is totally optional, but given that the "deobfuscate"-mode is on, it would be nice to make this even a bit less magic by moving some of the declared vars where they immediatelly make sense, e.g.:

```js
var prototype;
var initialize;
function constructor() {
...
}
prototype = ...
initialize = ...
```

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Reworked_SDK_test_tabs_ready_and_close_after_window_open.patch

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8805676 [details]
Bug 1313767: Deobfuscate core/heritage.js

https://reviewboard.mozilla.org/r/89390/#review90994

Yes, they're independent, and have independent performance benefits.

> it is totally optional, but given that the "deobfuscate"-mode is on, it would be nice to make this even a bit less magic by moving some of the declared vars where they immediatelly make sense, e.g.:
> 
> ```js
> var prototype;
> var initialize;
> function constructor() {
> ...
> }
> prototype = ...
> initialize = ...
> ```

Yeah, good idea.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

https://reviewboard.mozilla.org/r/89388/#review91238

I actually meant to figure out what was wrong with that test and re-enable it before I pushed.

> it would be nice to add to the inline comment a note about why we are keep this counter (which seems to be related to line 105)

I had a more detailed comment before. I'm not sure why I removed it...

> How about moving this validation:
> 
> ```
> if (!target) 
>   throw new TypeError("Event target must be an object");
> ```
> 
> into the `on`/`off`/`once` methods?

It's not really necessary. The WeakMap set method will already throw if it's not an object.

> If I'm not reading this wrong, if the same listener is added more then once, it can be detected as a "late added" listener and skipped by line 105.

Yeah, that had occurred to me. I honestly don't know which is the correct behavior. I was modeling the second add listener as a removal and a re-add. Either way, I hope there's no code depending on either behavior.

> None of the tests seems to have any issue with the following difference in the behavior of the new `emitOnObject`, but if I'm not reading this wrong, the previous version was skipping any listener that:
> 
> - was added after after the last element that was there when line 100 retrieved the length of the listeners array
> - was removed before line 111 has been executed for it
> 
> while the new version is:
> 
> - skipping any listener that has been added after line 99 retrieved the value of listenerCount (and as commented above, any listener that has been added again after that listenerCount value)
> 
> - not skipping any listner that has been removed before line 105 is executed for it
> 
> and so it seems that line 105 could be changed into something like to match the previous contract:
> 
> ```js
>       // Since ...
>       // ... was added and if is still in the listener map 
>       if (added >= count || !listeners.has(listener)) {
>       
>       }
> ```

The listeners iterator is not a snapshot. It doesn't iterate over listeners that were removed from the map after we start iterating, so there's no need for a second check. I really we could tweak the internals so there wasn't a need for the first check either... but too much work.

Anyway, there are tests for this behavior.

> This is an issue that the old code already presents, but I want to point it out given that we are looking into this method:
> 
> if a listener registered to \`"\*"\` raises an exception, \`emitOnObject\` is going to enter into a recursion loop (I briefly tried locally and it seems that sometimes it achieves to raise a "Too much recursion" exception at some point, and sometimes it seems to remain stuck in the recursion).

Interesting. Can you file a separate bug for that please?

> this is related to the change to the `loader` that is in the same patch, am I right?
> 
> is this necessary to the deobfuscate the `"events/core"` module?

Unfortunately, yes, it is. This code relies on a bug in the old implementation. Since it used `namespace` to track the event listeners for an object, it also created a prototype chain through all of the ancestors of that object, and if a listener for a certain event was added to a prototype of the object before it was added to the object itself, listeners for that object would be added to the prototype instead, and still be dispatched more or less as expected. But everything about that behavior is flaky and pathological. And this hack to avoid the freezing behavior for export objects had its own issues besides. It really needed to go either way.

> Why not just: 
> 
> ```js
> ...
> exports.DefaultMap = DefaultMap;
> ...
> exports.DefaultWeakMap = DefaultWeakMap;
> ```
> 
> like in the rest of this module?

Because it's ugly ;)

> This seems to be related to the change in `lib/sdk/tabs/tabs-firefox.js`, can we add a comment here about it?
> 
> probably it is not something that would affect how addons are using the module.exports object, but in my opinion it is a difference that can be worth an explicit mention during the review of this patch:
> 
> it seems that we are not going to freeze the export object anymore if it has been entirely reassigned, the freezed object couldn't be changed, while if we do not freeze it anymore, a change made to the export object will be reflected into the original object (and anyone that have a reference to it, it is going to access the updated object).

I can add a comment about the rationale. In any case, modules that want to completely replace the `exports` object and still want it frozen can still freeze it themselves. There were already enough hacky ways around the freezing that are in common use, such as the `Object.create()` hack in the tabs module, and getters and setters elsewhere.

> It is weird, but this test case doesn't pass for me locally even without the other changes in this patch.
> 
> After looking into it a bit, I'm now wondering how it ever been able to work.
> 
> Anyway I would be much more happy if we can fix the test, and given that I know how frustrating can be handling the SDK test suite, I'm attaching a reworked version of it that pass (like we use to say here "trouble shared is trouble halved" :-)), on both the old and the new version of this modules.

Yeah, I wound up disabling it while I was working on the patch because it seemed to be broken either way, and I couldn't figure out why. I think it may be timing dependent, but the underlying code is a mess, and I couldn't figure out why it ever worked... Either way, I didn't mean to disable it in this patch.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

https://reviewboard.mozilla.org/r/89388/#review91238

> It's not really necessary. The WeakMap set method will already throw if it's not an object.

Yeah, I know that the WeakMap will raise an exception, but it will happen in the `sdk/util/object` module and it will be something like "TypeError: key is not a non-null object" vs. the previous "TypeError: Event target must be an object".

It is not a big deal, the stacktrace would make visible that the issue is related to the `on` or `once` methods, but it is a small difference that can be easily removed, and so I though to point it out.

> Yeah, that had occurred to me. I honestly don't know which is the correct behavior. I was modeling the second add listener as a removal and a re-add. Either way, I hope there's no code depending on either behavior.

Theoretically, given that the listener was already there, registering it more than once shouldn't change its behavior, on the contrary if we remove the listener and we add it again, it makes sense that it will be considered as a "late added" listener and be skipped by emitOnObject.

> Interesting. Can you file a separate bug for that please?

Sure, filed as Bug 1316323

> Unfortunately, yes, it is. This code relies on a bug in the old implementation. Since it used `namespace` to track the event listeners for an object, it also created a prototype chain through all of the ancestors of that object, and if a listener for a certain event was added to a prototype of the object before it was added to the object itself, listeners for that object would be added to the prototype instead, and still be dispatched more or less as expected. But everything about that behavior is flaky and pathological. And this hack to avoid the freezing behavior for export objects had its own issues besides. It really needed to go either way.

Yeah, I guessed that it was related to that part.
Thanks for the additional details, they make the picture much more clear.

> Because it's ugly ;)

Definitely, I asked mainly because a lot of SDK module are often already harder to read than they should be (especially if someone is looking at their sources for the first time), and introducing different "patterns" to achieve the same result in the same SDK module over the time doesn't make this easier :-)

> I can add a comment about the rationale. In any case, modules that want to completely replace the `exports` object and still want it frozen can still freeze it themselves. There were already enough hacky ways around the freezing that are in common use, such as the `Object.create()` hack in the tabs module, and getters and setters elsewhere.

+1

Thanks for the additional details.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

https://reviewboard.mozilla.org/r/89388/#review91238

> Theoretically, given that the listener was already there, registering it more than once shouldn't change its behavior, on the contrary if we remove the listener and we add it again, it makes sense that it will be considered as a "late added" listener and be skipped by emitOnObject.

Either way, it's undefined behavior, and I don't think implementations should be relying on it either way. If a listener adds a new listener for the event it's receiving on every call, the first call will never see the event. I don't think it's at all clear what the behavior for subsequent calls should be, and I personally find it easier to reason about as the second .on() call removing the previous registration of the listener and adding a new one.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

https://reviewboard.mozilla.org/r/89388/#review101904

This patch is mainly missing two things that has not been yet solved from the previous review comments:

- an inline comment (that, as stated in the comments, has been probably removed by mistake)
- fixed test case (e.g. as in the proposed fix attached as a patch in the bugzilla issue) instead of blacklisting it (related to the `if (0) exports["test tabs ready and close after window.open"] = ...`)

I'm clearing the current review request in the meantime.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89388/diff/2-3/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8805676 [details]
Bug 1313767: Deobfuscate core/heritage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89390/diff/2-3/

Comment 15

[Kris Maglione [:kmag]]

I added comments where we discussed, but I didn't bother fixing the testcase. This code is on its way out, and I don't think it's worth anyone's time to continue maintaining those tests. If you want to submit your test updates in a separate bug, though, I'll review them.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8805675 [details]
Bug 1313767: Deobfuscate events/core.js

https://reviewboard.mozilla.org/r/89388/#review126142

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6423194b241e3d5dd1470f5bd40135826cf6828
Bug 1313767: Deobfuscate events/core.js r=rpl

https://hg.mozilla.org/integration/mozilla-inbound/rev/d60cdc45458895430a3f1dcef45a3d23987961bf
Bug 1313767: Deobfuscate core/heritage.js r=rpl

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6423194b241
https://hg.mozilla.org/mozilla-central/rev/d60cdc454588