#816 ES6 stringification needs to be made spec compliant (ES2019 misc)

Closed
opened 2 years ago by Sa-Ja-Di · 25 comments
Sa-Ja-Di commented 2 years ago (Migrated from github.com)

As Yami_ wrote: here:
https://forum.palemoon.org/viewtopic.php?f=3&t=20610#p154138

The News section issue was fixed in Firefox 55:
Last bad: 2017-04-17
First good: 2017-04-18
Pushlog
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9379831bb9c3d9abfea7dbf8dd06dbdab1d81dc4&tochange=bb38d935d699e0529f9e0bb35578d381026415c4

Perhaps that helps to limit the number of culprits.

Also, i have seen a warning in the error console which looks rather strange:

Timestamp: 07.10.2018 13:11:43
Warning: Unexpected end of file while searching for length value for matched media condition.
Source File: from DOM
Line: 0

Repeats a lot of times when visiting the Gog main page

As Yami_ wrote: here: https://forum.palemoon.org/viewtopic.php?f=3&t=20610#p154138 The News section issue was fixed in Firefox 55: Last bad: 2017-04-17 First good: 2017-04-18 Pushlog https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9379831bb9c3d9abfea7dbf8dd06dbdab1d81dc4&tochange=bb38d935d699e0529f9e0bb35578d381026415c4 Perhaps that helps to limit the number of culprits. Also, i have seen a warning in the error console which looks rather strange: Timestamp: 07.10.2018 13:11:43 Warning: Unexpected end of file while searching for length value for matched media condition. Source File: from DOM Line: 0 Repeats a lot of times when visiting the Gog main page
kn-yami commented 2 years ago (Migrated from github.com)
Owner

Actually the whole iframe#js-promo-iframe (including the news section) is broken. I think I know why: one of the JS frameworks (Angular, to be precise) used on the web page displayed in this iframe fails to initialize. Why it fails to initialize? Because it tries to call Function.prototype.apply.call(n, o, t) with n being a class. This obviously fails, for example:

js> class C {};
js> Function.prototype.apply.call(C);
typein:2:1 TypeError: class constructors must be invoked with |new|

Why is tries to “execute” a class? Angular differentiates between classes and functions by looking for the class keyword in string representation of the given object. This fails in UXP version of SpiderMonkey, for example:

js> class C {};
js> C.toString();
"function C() {\n    \n}"

Why does string representation of a class is a function? Probably because classes are in fact “special functions”, at least according to Mozilla. The issue with string representation of classes was reported to Mozilla as Bug 1216630 (commits that fixed the issue are in my pushlog), and to Angular as angular/angular.js#14240. SpiderMonkey versions with those fixes applied will behave like in this example:

js> class C {};
js> C.toString();
"class C {}"
Actually the whole `iframe#js-promo-iframe` (including the _news_ section) is broken. I think I know why: one of the JS frameworks (Angular, to be precise) used on the web page displayed in this `iframe` fails to initialize. Why it fails to initialize? Because it tries to call `Function.prototype.apply.call(n, o, t)` with `n` being a class. This obviously fails, for example: ``` js> class C {}; js> Function.prototype.apply.call(C); typein:2:1 TypeError: class constructors must be invoked with |new| ``` Why is tries to "execute" a class? Angular differentiates between classes and functions by looking for the `class` keyword in string representation of the given object. This fails in UXP version of SpiderMonkey, for example: ``` js> class C {}; js> C.toString(); "function C() {\n \n}" ``` Why does string representation of a class is a function? Probably because _classes are in fact "special functions"_, [at least according to Mozilla](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes#Defining_classes). The issue with string representation of classes was reported to Mozilla as [Bug 1216630](https://bugzilla.mozilla.org/show_bug.cgi?id=1216630) (commits that fixed the issue are in my pushlog), and to Angular as angular/angular.js#14240. SpiderMonkey versions with those fixes applied will behave like in this example: ``` js> class C {}; js> C.toString(); "class C {}" ```
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Good analysis - thanks. Although I’m scratching my head as to why a framework would use toString() on a jsObj and then expect to use the result as a source for further processing...

Good analysis - thanks. Although I'm scratching my head as to why a framework would use toString() on a jsObj and then expect to use the result as a source for further processing...
THEtomaso commented 2 years ago (Migrated from github.com)
Owner

GOG seems to have adjusted their “News” section now, to accommodate non-compliant browsers.
But of course, those Mozilla fixes can still be tested against GOG.com, through the Wayback Machine:
https://web.archive.org/web/20181007215424/https://www.gog.com/

GOG seems to have adjusted their "News" section now, to accommodate non-compliant browsers. But of course, those Mozilla fixes can still be tested against GOG.com, through the Wayback Machine: `https://web.archive.org/web/20181007215424/https://www.gog.com/`
kn-yami commented 2 years ago (Migrated from github.com)
Owner

It is still broken for me: broken iframe.

It is still broken for me: [broken iframe](https://www.gog.com/front_page).
THEtomaso commented 2 years ago (Migrated from github.com)
Owner

I was specifically refering to the “News” section at the bottom here:
https://www.gog.com/

I was specifically refering to the "News" section at the bottom here: `https://www.gog.com/`
kn-yami commented 2 years ago (Migrated from github.com)
Owner

This news section is also included in that iframe. The reason why it works now is simple: GOG has two designs of their web page: the normal one that works with UXP and the sale one that does not. The normal one does not have any iframe elements. The sale one is different, specifically it contains the custom-made sale landing page instead of the normal content. Below that there is an iframe containing a slightly modified version of the normal design that does not work with UXP. Since the sale has ended they reverted the web page to the normal design that just works.

This _news_ section is also included in that `iframe`. The reason why it works now is simple: GOG has two designs of their web page: the _normal_ one that works with UXP and the _sale_ one that does not. The _normal_ one does not have any `iframe` elements. The _sale_ one is different, specifically it contains the custom-made sale landing page instead of the normal content. Below that there is an `iframe` containing a slightly modified version of the _normal_ design that does not work with UXP. Since the sale has ended they reverted the web page to the normal design that just works.
Sa-Ja-Di commented 2 years ago (Migrated from github.com)
Owner

Which means this issue still should be fixed if possible :)

Which means this issue still should be fixed if possible :)
THEtomaso commented 2 years ago (Migrated from github.com)
Owner

Yes, of course.
The current GOG page didn’t even exist at the time when those Mozilla fixes were committed.
So, they were obviously created to target other issues.

Yes, of course. The current GOG page didn't even exist at the time when those Mozilla fixes were committed. So, they were obviously created to target other issues.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

If we want to port all this, we need the following:

I don’t see myself having time for this any time soon. Marking it as Bounty material.

If we want to port all this, we need the following: - [x] Porting of the toString revision from TC39 #960 - [x] Porting of the front-end parser restructuring to make further code adoption easier (and have it make more sense/be more correct) - [ ] Porting of the actual fix bug https://bugzilla.mozilla.org/show_bug.cgi?id=1216630 - [ ] Porting of follow-up bugfixes [1357483](https://bugzilla.mozilla.org/show_bug.cgi?id=1357483) [1357506](https://bugzilla.mozilla.org/show_bug.cgi?id=1357506) [1359622](https://bugzilla.mozilla.org/show_bug.cgi?id=1359622) and [1367204](https://bugzilla.mozilla.org/show_bug.cgi?id=1367204) I don't see myself having time for this any time soon. Marking it as Bounty material.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

I’m going to have a jab at the TC39 toString revision for standards compliance reasons. I’ll split it out into a separate issue blocking this one since it’s rather involved.

I'm going to have a jab at the TC39 toString revision for standards compliance reasons. I'll split it out into a separate issue blocking this one since it's rather involved.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Buh. #960 was fairly straightforward but a bunch of work. I think everything in the revision proposal except for the representation of “class” has been fixed, so stage 1 is complete.
If you want to work on this, please use branch 816 which has the necessary prerequisite changes for this issue merged in.

Buh. #960 was fairly straightforward but a bunch of work. I think everything in the revision proposal except for the representation of "class" has been fixed, so stage 1 is complete. If you want to work on this, please use branch `816` which has the necessary prerequisite changes for this issue merged in.
kn-yami commented 2 years ago (Migrated from github.com)
Owner

If you do not have anything agings this, I think I can try to work on this after all.

If you do not have anything agings this, I think I can try to work on this after all.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

@kn-yami Sure thing! I’ll assign you as working on this.

@kn-yami Sure thing! I'll assign you as working on this.
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

@kn-yami No progress in 2 months noted. Are you still working on this? The whole point of the bounty program is NOT so people can claim issues to then go stagnant.

@kn-yami No progress in 2 months noted. Are you still working on this? The whole point of the bounty program is NOT so people can claim issues to then go stagnant.
kn-yami commented 1 year ago (Migrated from github.com)
Owner

The whole point of the bounty program is NOT so people can claim issues to then go stagnant.

I know. Please unassign me. And sorry it took me so long to say this - I really wanted to fix this issue (in fact I did work on it today).
If anyone else wants to fix this please be aware that Mozilla patches for this issue depend on bug 1296814.

Again, I am sorry...

> The whole point of the bounty program is NOT so people can claim issues to then go stagnant. I know. Please unassign me. And sorry it took me so long to say this - I really wanted to fix this issue (in fact I did work on it today). If anyone else wants to fix this please be aware that Mozilla patches for this issue depend on [bug 1296814](https://bugzilla.mozilla.org/show_bug.cgi?id=1296814). Again, I am sorry...
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

I’d rather avoid 1296814 if possible and work around the tree-wide refactoring done there.

It is not a big disaster if it can’t be avoided since the patches landed very close to our fork-off point, but care must be taken to not break the parser.

I'd rather avoid 1296814 if possible and work around the tree-wide refactoring done there. It is not a big disaster if it can't be avoided since the patches landed very close to our fork-off point, but care must be taken to not break the parser.
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Assigning myself to port 1296814, considering it’s generally a desired change to make parser structure better.

Assigning myself to port 1296814, considering it's generally a desired change to make parser structure better.
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

I’ve ported the parser changes, which were once again fairly straightforward even if extensive, but opening this issue up again to anyone wanting to tackle this for a bounty, considering I’m unlikely to be able to work on this in the coming week. Parser changes and TC39 revision code have been merged to master so the platform can benefit from these improvements so far.

I've ported the parser changes, which were once again fairly straightforward even if extensive, but opening this issue up again to anyone wanting to tackle this for a bounty, considering I'm unlikely to be able to work on this in the coming week. Parser changes and TC39 revision code have been merged to master so the platform can benefit from these improvements so far.
JustOff commented 1 year ago (Migrated from github.com)
Owner

I’ve ported the parser changes

Looks like this has led to side effects.

> I've ported the parser changes Looks like this has led to [side effects](https://forum.palemoon.org/viewtopic.php?p=166037#p166037).
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

The “side effect” being that “export” is a reserved keyword? Not our problem, is it?

The "side effect" being that "export" is a reserved keyword? Not our problem, is it?
mattatobin commented 1 year ago (Migrated from github.com)
Owner

Nah and is fixable easily, just we need to to watch for that sort of thing and be sure to announce it in relnotes.

Nah and is fixable easily, just we need to to watch for that sort of thing and be sure to announce it in relnotes.
JustOff commented 1 year ago (Migrated from github.com)
Owner

It’s good that everything turned out to be simpler than it seemed initially. Sorry for the false alarm, although I still don’t fully understand all the changes that need to be considered from now.

It's good that everything turned out to be simpler than it seemed initially. Sorry for the false alarm, although I still don't fully understand all the changes that need to be considered from now.
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

I’ll list examples (taken from kangax’s compatibility table) of the behavior as expected with the revision proposal, which is part of ES2019, but only a miscellaneous spec, not a main feature. We support 5 of 7 of these features now, and the last 2 that need implementation deal with classes.

  1. Functions created with the Function constructor. SUPPORTED
var fn = Function('a', ' /\x2A a \x2A/ b, c /\x2A b \x2A/ //', '/\x2A c \x2A/ ; /\x2A d \x2A/ //');
var str = 'function anonymous(a, /\x2A a \x2A/ b, c /\x2A b \x2A/ //\n) {\n/\x2A c \x2A/ ; /\x2A d \x2A/ //\n}';
return fn + '' === str;
  1. arrows SUPPORTED
var str = 'a => b';
return eval('(' + str + ')') + '' === str;
  1. [native code] SUPPORTED
const NATIVE_EVAL_RE = /\bfunction\b[\s\S]*\beval\b[\s\S]*\([\s\S]*\)[\s\S]*\{[\s\S]*\[[\s\S]*\bnative\b[\s\S]+\bcode\b[\s\S]*\][\s\S]*\}/;
return NATIVE_EVAL_RE.test(eval + '');
  1. class expressions with implicit constructor NOT SUPPORTED
var str = 'class A {}';
return eval('(' + str + ')') + '' === str;

Currently returns:

function A() {
    
}
  1. class expressions with explicit constructor NOT SUPPORTED
var str = 'class /\x2A a \x2A/ A /\x2A b \x2A/ extends /\x2A c \x2A/ function B(){} /\x2A d \x2A/ { /\x2A e \x2A/ constructor /\x2A f \x2A/ ( /\x2A g \x2A/ ) /\x2A h \x2A/ { /\x2A i \x2A/ ; /\x2A j \x2A/ } /\x2A k \x2A/ m /\x2A l \x2A/ ( /\x2A m \x2A/ ) /\x2A n \x2A/ { /\x2A o \x2A/ } /\x2A p \x2A/ }';
return eval('(/\x2A before \x2A/' + str + '/\x2A after \x2A/)') + '' === str;

Currently returns:

constructor /* f */ ( /* g */ ) /* h */ { /* i */ ; /* j */ }
  1. unicode escape sequences in identifiers SUPPORTED
var str = 'function \\u0061(\\u{62}, \\u0063) { \\u0062 = \\u{00063}; return b; }';
return eval('(/\x2A before \x2A/' + str + '/\x2A after \x2A/)') + '' === str;
  1. methods and computed property names SUPPORTED
var str = '[ /\x2A a \x2A/ "f" /\x2A b \x2A/ ] /\x2A c \x2A/ ( /\x2A d \x2A/ ) /\x2A e \x2A/ { /\x2A f \x2A/ }';
return eval('({ /\x2A before \x2A/' + str + '/\x2A after \x2A/ }.f)') + '' === str;
I'll list examples (taken from kangax's compatibility table) of the behavior as expected with the revision proposal, which is part of ES2019, but only a miscellaneous spec, not a main feature. We support 5 of 7 of these features now, and the last 2 that need implementation deal with classes. 1. Functions created with the Function constructor. SUPPORTED ``` JavaScript var fn = Function('a', ' /\x2A a \x2A/ b, c /\x2A b \x2A/ //', '/\x2A c \x2A/ ; /\x2A d \x2A/ //'); var str = 'function anonymous(a, /\x2A a \x2A/ b, c /\x2A b \x2A/ //\n) {\n/\x2A c \x2A/ ; /\x2A d \x2A/ //\n}'; return fn + '' === str; ``` 2. arrows SUPPORTED ``` JavaScript var str = 'a => b'; return eval('(' + str + ')') + '' === str; ``` 3. [native code] SUPPORTED ``` JavaScript const NATIVE_EVAL_RE = /\bfunction\b[\s\S]*\beval\b[\s\S]*\([\s\S]*\)[\s\S]*\{[\s\S]*\[[\s\S]*\bnative\b[\s\S]+\bcode\b[\s\S]*\][\s\S]*\}/; return NATIVE_EVAL_RE.test(eval + ''); ``` 4. class expressions with implicit constructor NOT SUPPORTED ``` JavaScript var str = 'class A {}'; return eval('(' + str + ')') + '' === str; ``` Currently returns: ``` function A() { } ``` 5. class expressions with explicit constructor NOT SUPPORTED ``` JavaScript var str = 'class /\x2A a \x2A/ A /\x2A b \x2A/ extends /\x2A c \x2A/ function B(){} /\x2A d \x2A/ { /\x2A e \x2A/ constructor /\x2A f \x2A/ ( /\x2A g \x2A/ ) /\x2A h \x2A/ { /\x2A i \x2A/ ; /\x2A j \x2A/ } /\x2A k \x2A/ m /\x2A l \x2A/ ( /\x2A m \x2A/ ) /\x2A n \x2A/ { /\x2A o \x2A/ } /\x2A p \x2A/ }'; return eval('(/\x2A before \x2A/' + str + '/\x2A after \x2A/)') + '' === str; ``` Currently returns: ``` constructor /* f */ ( /* g */ ) /* h */ { /* i */ ; /* j */ } ``` 6. unicode escape sequences in identifiers SUPPORTED ``` JavaScript var str = 'function \\u0061(\\u{62}, \\u0063) { \\u0062 = \\u{00063}; return b; }'; return eval('(/\x2A before \x2A/' + str + '/\x2A after \x2A/)') + '' === str; ``` 7. methods and computed property names SUPPORTED ``` JavaScript var str = '[ /\x2A a \x2A/ "f" /\x2A b \x2A/ ] /\x2A c \x2A/ ( /\x2A d \x2A/ ) /\x2A e \x2A/ { /\x2A f \x2A/ }'; return eval('({ /\x2A before \x2A/' + str + '/\x2A after \x2A/ }.f)') + '' === str; ```
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Marking as assigned since @g4jc is working on this as part of a bigger patch

Marking as assigned since @g4jc is working on this as part of a bigger patch
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Resolved by PR #1192

Resolved by PR #1192
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.