1702903 - Add-on SDK and for each in Add-ons broken in 2.53.7

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Khvoinitsky Mikhail [:m_khvoinitsky]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Firefox/87.0

Steps to reproduce:

Install or enable an add-on which uses Add-on SDK (tested with https://addons.thunderbird.net/seamonkey/addon/dark-bg-light-text-seamonkey/ )

Actual results:

Add-on doesn't work, there is an error in Browser Console:
"yield is a reserved identifier", stacktrace:
resource://gre/modules/commonjs/sdk/deprecated/api-utils.js:125:NaN
resource://gre/modules/commonjs/sdk/util/contract.js:11:36
resource://gre/modules/commonjs/sdk/content/loader.js:11:22
resource://gre/modules/commonjs/sdk/page-mod.js:10:38
resource://ktborporoqokbkzzjjqwqtncooifjm-at-jetpack/lib/index.js:4:21run
resource://gre/modules/commonjs/sdk/addon/runner.js:143:19startup/</<
resource://gre/modules/commonjs/sdk/addon/runner.js:83:9process
resource://gre/modules/Promise-backend.js:928:23walkerLoop
resource://gre/modules/Promise-backend.js:812:7scheduleWalkerLoop/<
resource://gre/modules/Promise-backend.js:748:11

Examining api-utils.js:125 shows that there is yield statement inside non-asterisk function which is deprecated generator syntax which seems to be the problem.

Expected results:

Add-on should work

Comment 1

[Dmitry Butskoy (:buc)]

Caused by the backport of bug #1390106 part3, gitlab commit here .

After the change of the initialization value from JSVERSION_LATEST to JSVERSION_DEFAULT (with the value of zero), the four version checks in the code now always return false.

Since the corresponding legacy code still remains, it seems that dropping these checks should temporary help. At least it helps for "for each" case.

NOTE about the second hunk: it is not "... || true", since it is kinda another fix, just backported from bug #1414546 .

diff -rup mozilla/js/src/frontend/Parser.cpp mozilla-OK/js/src/frontend/Parser.cpp
--- mozilla/js/src/frontend/Parser.cpp  2021-04-02 01:19:13.000000000 +0300
+++ mozilla-OK/js/src/frontend/Parser.cpp       2021-04-03 23:27:59.951404210 +0300
@@ -6686,7 +6686,6 @@ Parser<ParseHandler, CharT>::yieldExpres
       case NotGenerator:
         // We are in code that has not seen a yield, but we are in JS 1.7 or
         // later.  Try to transition to being a legacy generator.
-        MOZ_ASSERT(tokenStream.versionNumber() >= JSVERSION_1_7);
         MOZ_ASSERT(pc->lastYieldOffset == ParseContext::NoYieldOffset);

         if (!abortIfSyntaxParser())
@@ -9194,7 +9193,7 @@ Parser<ParseHandler, CharT>::checkLabelO
         return true;
     if (TokenKindIsContextualKeyword(tt)) {
         if (tt == TOK_YIELD) {
-            if (yieldHandling == YieldIsKeyword || versionNumber() >= JSVERSION_1_7) {
+            if (yieldHandling == YieldIsKeyword) {
                 errorAt(offset, JSMSG_RESERVED_ID, "yield");
                 return false;
             }
diff -rup mozilla/js/src/frontend/Parser.h mozilla-OK/js/src/frontend/Parser.h
--- mozilla/js/src/frontend/Parser.h    2021-04-03 23:18:51.960861471 +0300
+++ mozilla-OK/js/src/frontend/Parser.h 2021-04-03 23:19:53.644359744 +0300
@@ -175,7 +175,7 @@ class ParserBase : public StrictModeGett
     // whether it's prohibited due to strictness, JS version, or occurrence
     // inside a star generator.
     bool yieldExpressionsSupported() {
-        return (versionNumber() >= JSVERSION_1_7 && !pc->isAsync()) ||
+        return !pc->isAsync() ||
                pc->isStarGenerator() ||
                pc->isLegacyGenerator();
     }
@@ -247,7 +247,7 @@ class ParserBase : public StrictModeGett
 #if !JS_HAS_FOR_EACH_IN
         return false;
 #else
-        return options().forEachStatementOption && versionNumber() >= JSVERSION_1_6;
+        return options().forEachStatementOption;
 #endif
     }

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1702903-versionjs-25371.patch

This fixes the for each issue and takes out the old legacy generator. Not yet asking for review. Need to think if we want to restore the legacy generator stuff generally. Deprected since Gecko 26 and I would like to have it out for blocking future fixes in Spidermonkey.

Comment 3

[Khvoinitsky Mikhail [:m_khvoinitsky]]

I tried to fix add-on SDK for new parser which didn't take a lot of effort (I haven't tested the whole SDK, only checked if my add-on is working). The changes are:

diff --git a/addon-sdk/source/lib/sdk/deprecated/api-utils.js b/addon-sdk/source/lib/sdk/deprecated/api-utils.js
index 856fc50cb6..1075fa6e69 100644
--- a/addon-sdk/source/lib/sdk/deprecated/api-utils.js
+++ b/addon-sdk/source/lib/sdk/deprecated/api-utils.js
@@ -115,7 +115,7 @@ exports.validateOptions = function validateOptions(options, requirements) {
 };
 
 exports.addIterator = function addIterator(obj, keysValsGenerator) {
-  obj.__iterator__ = function(keysOnly, keysVals) {
+  obj.__iterator__ = function*(keysOnly, keysVals) {
     let keysValsIterator = keysValsGenerator.call(this);
 
     // "for (.. in ..)" gets only keys, "for each (.. in ..)" gets values,
diff --git a/addon-sdk/source/lib/sdk/ui/state.js b/addon-sdk/source/lib/sdk/ui/state.js
index 3da71bd44c..dcd53cd7f1 100644
--- a/addon-sdk/source/lib/sdk/ui/state.js
+++ b/addon-sdk/source/lib/sdk/ui/state.js
@@ -30,7 +30,9 @@ const { freeze } = Object;
 const { merge } = require('../util/object');
 lazyRequire(this, '../event/core', "on", "off", "emit");
 
-lazyRequire(this, '../lang/weak-set', "add", "remove", "has", "clear", "iterator");
+const weak_set = require('../lang/weak-set');
+const { remove, has } = weak_set;
+const iterator = weak_set[Symbol.iterator];
 lazyRequire(this, '../lang/type', "isNil");
 
 lazyRequire(this, '../view/core', "viewFor");
diff --git a/addon-sdk/source/lib/sdk/util/rules.js b/addon-sdk/source/lib/sdk/util/rules.js
index a315dce24a..abf95779eb 100644
--- a/addon-sdk/source/lib/sdk/util/rules.js
+++ b/addon-sdk/source/lib/sdk/util/rules.js
@@ -46,8 +46,8 @@ exports.Rules = Rules;
 
 function filterMatches(instance, uri) {
   let matches = [];
-  for (let i in instance) {
-    if (new MatchPattern(instance[i]).test(uri)) matches.push(instance[i]);
+  for (let i of Object.values(instance)) {
+    if (new MatchPattern(i).test(uri)) matches.push(i);
   }
   return matches;
 }

Comment 4

[Khvoinitsky Mikhail [:m_khvoinitsky]]

Slightly fixed version (fixed: missing weak_set.add import, better variable name for values iteration):

diff --git a/addon-sdk/source/lib/sdk/deprecated/api-utils.js b/addon-sdk/source/lib/sdk/deprecated/api-utils.js
index 856fc50cb6..1075fa6e69 100644
--- a/addon-sdk/source/lib/sdk/deprecated/api-utils.js
+++ b/addon-sdk/source/lib/sdk/deprecated/api-utils.js
@@ -115,7 +115,7 @@ exports.validateOptions = function validateOptions(options, requirements) {
 };
 
 exports.addIterator = function addIterator(obj, keysValsGenerator) {
-  obj.__iterator__ = function(keysOnly, keysVals) {
+  obj.__iterator__ = function*(keysOnly, keysVals) {
     let keysValsIterator = keysValsGenerator.call(this);
 
     // "for (.. in ..)" gets only keys, "for each (.. in ..)" gets values,
diff --git a/addon-sdk/source/lib/sdk/ui/state.js b/addon-sdk/source/lib/sdk/ui/state.js
index 3da71bd44c..9695eaa95c 100644
--- a/addon-sdk/source/lib/sdk/ui/state.js
+++ b/addon-sdk/source/lib/sdk/ui/state.js
@@ -30,7 +30,9 @@ const { freeze } = Object;
 const { merge } = require('../util/object');
 lazyRequire(this, '../event/core', "on", "off", "emit");
 
-lazyRequire(this, '../lang/weak-set', "add", "remove", "has", "clear", "iterator");
+const weak_set = require('../lang/weak-set');
+const { add, remove, has } = weak_set;
+const iterator = weak_set[Symbol.iterator];
 lazyRequire(this, '../lang/type', "isNil");
 
 lazyRequire(this, '../view/core', "viewFor");
diff --git a/addon-sdk/source/lib/sdk/util/rules.js b/addon-sdk/source/lib/sdk/util/rules.js
index a315dce24a..3e5cb0e893 100644
--- a/addon-sdk/source/lib/sdk/util/rules.js
+++ b/addon-sdk/source/lib/sdk/util/rules.js
@@ -46,8 +46,8 @@ exports.Rules = Rules;
 
 function filterMatches(instance, uri) {
   let matches = [];
-  for (let i in instance) {
-    if (new MatchPattern(instance[i]).test(uri)) matches.push(instance[i]);
+  for (let v of Object.values(instance)) {
+    if (new MatchPattern(v).test(uri)) matches.push(v);
   }
   return matches;
 }

Comment 5

[Frank-Rainer Grahl (:frg)]

Attachment: 1702903-versionjs-25371.patch

I added the patches for the add-on sdk. Kept clear for now. Completly unfamiliar with this stuff and not sure if it has side effects. The other parts now drop the version checks. Would probably be enough to fix the bug but so we can later drop the legacy syntax if we want to.

buc: I think the removal from Bug 1414546 is not the right thing to do here. Should be an if which results in an unconditional always "true" which can be removed because we always assume we are in js > 1.7 code and so not allowing identifiers and labels named yield. So always returning false. Hopefully I interpreted the SpiderMonkey code right.

Tested the dark dark-bg-light-text add-on with this version. Patch is now in the unofficial version to and should appear in Bills next 2.53.8b1 pre.

Comment 6

[Dmitry Butskoy (:buc)]

Comment on attachment 9213504 [details] [diff] [review]
1702903-versionjs-25371.patch

LGTM

There are two places left in the code that, in theory, can broke old user scripts:

Setting to JSVERSION_UNKNOWN (instead default) on any presence of ";version=" in dom/xul/nsXULContentSink.cpp .

The construction ";version=latest" now is not allowed in js/xpconnect/src/XPCComponents.cpp

Maybe set to JSVERSION_DEFAULT for both cases?

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1702903-versionjs-v1_1-25371.patch

I think there are no version checks left after the patch. Or at least I didn't find any. So we might be able to take versioning out without affecting the current functionality.

Setting to JSVERSION_UNKNOWN (instead default) on any presence of ";version=" in dom/xul/nsXULContentSink.cpp .

Not needed. Just not set an error. Tested with putting a version in a xul file. Error printed and js loaded. I didn't test if add-ons hit this code patch but they should now continue to work.

The construction ";version=latest" now is not allowed in js/xpconnect/src/XPCComponents.cpp
Maybe set to JSVERSION_DEFAULT for both cases?

I think this should stay. Only affects scripts which explicitely set a version and an easy fixer then. This is all obsolete stuff and should be removed.
[Approval Request Comment]
Regression caused by (bug #): Bug 1390106
User impact if declined: add-on breakage
Testing completed (on m-c, etc.): 2.53.8b1 pre
Risk to taking this patch (and alternatives if risky): moderate.
String changes made by this patch: --

If no objections I would recommend doing a 2.53.7.1 with this patch.

Comment 8

[Ian Neal]

Comment on attachment 9213566 [details] [diff] [review]
1702903-versionjs-v1_1-25371.patch

[Triage Comment]
Seems to be correct r/a=me

Comment 10

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-mozilla/-/commit/91bd46dce6b53fe7881431ecb6cebf7172c1c30d
Fix chrome js code dependent on js versions. r=IanN a=IanN

Comment 11

[Frank-Rainer Grahl (:frg)]

Attachment: 1702903-2-versionjs-2538.patch

This needs a followup. I half expected this but hoped it would not be so.

The code which broke declared yield itself (probably class or var).

switch(fieldIdx){
case 0:yield.idTextField=item.items.items[0];break;
case 1:yield.typeCombo=item.items.items[0];break;
case 2:yield.valueTextField=item.items.items[0];break;

Fix is to allow legacy generators only in the system or add-on compartments.

[Approval Request Comment]
Regression caused by (bug #): bug 1702903
User impact if declined: some websites where yield is not used as a generator e.g. field definition will not work.
Testing completed (on m-c, etc.): 2.53.8
Risk to taking this patch (and alternatives if risky): Low risk. Alternative is to rip out legacy generators.
String changes made by this patch: --

Comment 12

[Ian Neal]

Comment on attachment 9221353 [details] [diff] [review]
1702903-2-versionjs-2538.patch

[Triage Comment]
Thanks, r/a=me

Comment 13

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-mozilla/-/commit/648d64cd4eaef624925c22f4109bf4eb99dbf7c4
Follow-up: Allow legacy generators only in system and add-ons code. r=IanN a=IanN
Target 2.53.8