1394846 - After downloading a video, then you disable/enable the extension, the EventEmitter.decorate error is displayed

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P1)

Description

[CosminB]

Attachment: EventEmitter.decorate+Logs.zip

[Affected versions]:
- Firefox 57.0a1 (2017-08-29) 

[Affected platforms]:
- Android 6.0.1

[Prerequisites]
- set xpinstall.signatures.dev-root to true
- set extensions.webapi.testing to true
- set xpinstall.signatures.required to false

[Steps to reproduce]:
1.Install https://addons-dev.allizom.org/en-US/firefox/addon/instagram-image-downloader/?src=search
2.Download a video from Instagram.
3.Go to the detailed page of the extension in about:addons.
4.Disable the extension.
5.Enable it.

[Expected results]:
- The icon is displayed in the URL bar.

[Actual results]:
- The icon from the extension is not visible anymore.
- EventEmitter.decorate error is displayed in the browser console every time you enable/disable the extension.

Comment 1

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

Attachment: Bug 1394846 - Fix EventEmitter.decorate exception on Firefox for Android.

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

Comment 2

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

Thanks a lot for the STR.

Long story short: this is a recent regression most likely related to Bug 1387907, we have two different EventEmitter implementations around (three to be fair, but one is in the devtools and only used there), the attached patch applies the changes needed to use only one implementation in the WebExtensions Android internals (the one provided by ExtensionUtils.jsm and used by "most" of the other WebExtensions internals) and also tweak two test cases that started to fail on the new version, it seems to me that these failure are due to wrong assumptions in the test case itself (e.g. there is no guarantees that a tab is going to be immediatelly selected after a `browser.tabs.update` API call, because the underlying implementation uses BrowserApp.selectTab to update the active tab, and its implementation is clearly asynchronous).

Additional Regression Details: Bug 1387907 introduces some changes to `ext-toolkit.js` that define `global.EventEmitter` at toolkit level as implemented in ExtensionUtils.jsm (this one does not provide `Eventemitter.decorate`):

- http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/toolkit/components/extensions/ext-toolkit.js#20
- http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/toolkit/components/extensions/ExtensionUtils.jsm#171

while in `ext-android.js` the same global is overwritten with a lazy getter which retrieves the EventEmitter implementation from EventEmitter.jsm (this one provides `Eventemitter.decorate`) :

- http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/mobile/android/components/extensions/ext-android.js#3-4
- http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/toolkit/modules/EventEmitter.jsm#17

It seems that when the addon is disabled and then re-enabled, ext-pageAction.js gets the `global.EventEmitter` defined by `ext-toolkit.js` instead of the lazy property defined by `ext-android.js` 

well, that is what happen when you name two different implementation of the same concept with the same name :-)

NOTE: the two `EventEmitter` implementation also differs for the `emit` implementation (spoiler the one from ExtensionUtils.jsm returns a Promise.all call if any of the listener returns a result):

- http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/toolkit/components/extensions/ExtensionUtils.jsm#250-272
- http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/toolkit/modules/EventEmitter.jsm#132-163

This is a difference that could be useful to keep in mind, e.g. it could be the cause if we notice further regressions in the tests that could be related to it.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8902412 [details]
Bug 1394846 - Fix EventEmitter.decorate exception on Firefox for Android.

https://reviewboard.mozilla.org/r/173992/#review180160

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fb1527b1af3
Fix EventEmitter.decorate exception on Firefox for Android. r=mixedpuppy

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7fb1527b1af3

Comment 6

[CosminB]

Attachment: emitterFix.gif

This issue is verified as fixed on Fennec 57.0a1 (2017-09-03) under Android 7.1.2.

The icon from extension is displayed after it was re-enabled.