SyntaxError in firefox after adblockplusui 7be8b3f3c5fb

[wspee]

Environment

Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0

How to reproduce

1. Update adblockplusui dependency in adblockplus to ​7be8b3f3c5fb and adblockpluscore dependency to ​e78c74e66066.
2. Build and install extension
3. Observe console output

Observed behaviour

The following error is raised:

1488996136267   addons.xpi      WARN    Exception running bootstrap method startup on {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}: SyntaxError: in strict mode code, functions may be declared only at top level or immediately within another function (resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/wspee/.mozilla/firefox/x0e4pn2q.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///home/wspee/.mozilla/firefox/x0e4pn2q.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/messageResponder.js:44:11) JS Stack trace: require@bootstrap.js:134:7 < @main.js:32:1 < require@bootstrap.js:134:7 < startup@bootstrap.js:26:3 < this.XPIProvider.callBootstrapMethod@XPIProvider.jsm:4656:9 < AddonInstall.prototype.startInstall/<@XPIProvider.jsm:6081:13 < TaskImpl_run@Task.jsm:315:40 < Handler.prototype.process@Promise-backend.js:933:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:812:7 < this.PromiseWalker.scheduleWalkerLoop/<@Promise-backend.js:746:1

Expected behaviour

No SyntaxError should be raised.

[kzar]

Which revision of adblockplus are you using? Are you using the Browser Console (Control Shift J)? What do you do to trigger the error/warning, or does it appears immediately after adblockplus is installed?

I had trouble reproducing this as described (tried Firefox 51 and 52 after it happened to update), but I did see different exceptions relating to the elemHide / elemHideEmulation code:

TypeError: can't convert undefined to object elemHide.js:331:1
TypeError: patterns is undefined elemHideEmulation.js:129:23

[kzar]

(If we can track down what's going wrong I'd be happy to fix this as part of the ESLint related changes, since I'm changing both adblockplusui and adblockpluscore there anyway.)

[trev]

It's quite obvious what is wrong. Your change added "use strict" to this file, so Firefox now enforces functions to be at the top level - currently there is a block wrapping all the contents for some reason and functions are declared inside this block which makes fairly little sense (and prohibited in strict mode).

[wspee]

Replying to kzar:

Which revision of adblockplus are you using? Are you using the Browser Console (Control Shift J)? What do you do to trigger the error/warning, or does it appears immediately after adblockplus is installed?

I had trouble reproducing this as described (tried Firefox 51 and 52 after it happened to update), but I did see different exceptions relating to the elemHide / elemHideEmulation code:

TypeError: can't convert undefined to object elemHide.js:331:1
TypeError: patterns is undefined elemHideEmulation.js:129:23

The error is printed on stdout/stderr (if you start firefox from a console) not in the developer console.

I was also able to isolate it down to ​https://hg.adblockplus.org/adblockplusui/rev/7be8b3f3c5fb in ​https://hg.adblockplus.org/adblockplus/rev/cdf92a8ed531 (then the TypeErrors also disappear).

[trev]

For some reason, ESLint isn't catching this issue. In my understanding, this should be covered by no-inner-declarations.

[trev]

For reference, for #4783 changes to work in Firefox another change is necessary - messageResponder now expects that messages send from content via ext.backgroundPage.sendMessage() can be received via port, with a sender object indicating page and frame. The latter makes it particularly problematic because the sender semantics are usually different on Firefox. So I think that we should go with the following hack:

diff --git a/ext/common.js b/ext/common.js
--- a/ext/common.js
+++ b/ext/common.js
@@ -41,16 +41,25 @@
 
     return frames;
   }
 
   var EventTarget = global.ext._EventTarget = function(port, windowID)
   {
     this._port = port;
     this._windowID = windowID;
+    this.addListener((payload, sender, resolve) =>
+    {
+      if (payload.type)
+      {
+        let result = this._port._dispatch(payload.type, payload, sender);
+        if (typeof result != "undefined")
+          resolve(result);
+      }
+    });
   };
   EventTarget.prototype = {
     addListener: function(listener)
     {
       var wrapper = (message, sender) =>
       {
         if (this._windowID && this._windowID != message.targetID)
           return undefined;

This will make sure that ext_message messages won't merely go to onMessage callbacks but will be dispatched on the port as well - then with the ext-specific sender like the one we use for onMessage callbacks.

[abpbot]

A commit referencing this issue has landed:
​Issue 4968 - Fix messageResponder syntax, declaring functions inside a block not allowed in strict mode

[kzar]

Replying to trev:

For reference, for #4783 changes to work in Firefox another change is necessary - messageResponder now expects that messages send from content via ext.backgroundPage.sendMessage() can be received via port, with a sender object indicating page and frame. The latter makes it particularly problematic because the sender semantics are usually different on Firefox. So I think that we should go with the following hack...

Thanks that worked for me, here's my ​eslint-test branch with the changes I needed to get things working.

I also needed to apply ​Winsley's ext.i18n change, fix the antiadblockInit.js path and remove the load call from elemHideEmulation.js.