Closed Bug 1702903 Opened 1 month ago Closed 26 days ago

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

Categories

(SeaMonkey :: General, defect)

SeaMonkey 2.53 Branch
defect

Tracking

(seamonkey2.53+ fixed, seamonkey2.57esr unaffected)

RESOLVED FIXED
seamonkey2.53
Tracking Status
seamonkey2.53 + fixed
seamonkey2.57esr --- unaffected

People

(Reporter: m_khvoinitsky, Assigned: frg)

References

Details

(Whiteboard: SM2.53.7.1)

Attachments

(1 file, 2 obsolete files)

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

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
     }
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch 1702903-versionjs-25371.patch (obsolete) — Splinter Review

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.

Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Summary: Add-on SDK is broken in 2.53.7 → Add-on SDK and for each in Add-ons broken in 2.53.7

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;
 }

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;
 }
Attached patch 1702903-versionjs-25371.patch (obsolete) — Splinter Review

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.

Attachment #9213495 - Attachment is obsolete: true
Attachment #9213504 - Flags: feedback?(mikhail-bmo)
Attachment #9213504 - Flags: feedback?(dmitry)

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?

Attachment #9213504 - Flags: feedback?(dmitry) → feedback+

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.

Attachment #9213504 - Attachment is obsolete: true
Attachment #9213504 - Flags: feedback?(mikhail-bmo)
Attachment #9213566 - Flags: review?(iann_bugzilla)
Attachment #9213566 - Flags: approval-comm-release?
Blocks: 1703363

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

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

Attachment #9213566 - Flags: review?(iann_bugzilla)
Attachment #9213566 - Flags: review+
Attachment #9213566 - Flags: approval-comm-release?
Attachment #9213566 - Flags: approval-comm-release+
Duplicate of this bug: 1704151
Status: ASSIGNED → RESOLVED
Closed: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.53
Whiteboard: SM2.53.7.1
You need to log in before you can comment on or make changes to this bug.