Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4968 closed defect (fixed)

SyntaxError in firefox after adblockplusui 7be8b3f3c5fb

Reported by: wspee Assignee: trev
Priority: P1 Milestone:
Module: User-Interface Keywords:
Cc: greiner, kzar, trev Blocked By:
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29379570/

Description (last modified by kzar)

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.

Change History (13)

comment:1 Changed 3 years ago by kzar

  • Cc greiner kzar trev added
  • Description modified (diff)

comment:2 follow-up: Changed 3 years ago by 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

comment:3 Changed 3 years ago by 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.)

comment:4 Changed 3 years ago by 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).

comment:5 Changed 3 years ago by trev

  • Owner set to trev

comment:6 in reply to: ↑ 2 Changed 3 years ago by 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).

comment:7 Changed 3 years ago by trev

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

comment:8 Changed 3 years ago by trev

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:9 Changed 3 years ago by greiner

  • Priority changed from Unknown to P1
  • Ready set

comment:10 follow-up: Changed 3 years ago by 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.

comment:12 Changed 3 years ago by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:13 in reply to: ↑ 10 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.