Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async/await, this time with feeling [M1185106] #521

Closed
classilla opened this issue Sep 15, 2018 · 10 comments
Closed

async/await, this time with feeling [M1185106] #521

classilla opened this issue Sep 15, 2018 · 10 comments

Comments

@classilla
Copy link
Owner

A number of broken sites rely on this functionality, and it just made it into Fx52. I'm not 100% sure it's doable, but reviewing the changesets suggest that much of it should work sans the Annex B portions.

https://bugzilla.mozilla.org/show_bug.cgi?id=1185106
(note deps)

classilla added a commit that referenced this issue Sep 15, 2018
classilla added a commit that referenced this issue Sep 15, 2018
@classilla
Copy link
Owner Author

We need to implement JS Promises :/

@classilla
Copy link
Owner Author

The C++ version used here seems to originate from https://hg.mozilla.org/mozilla-central/rev/309ecb16acfe ( https://bugzilla.mozilla.org/show_bug.cgi?id=1313049 )

However, the root implementation it descends from is here: https://bugzilla.mozilla.org/show_bug.cgi?id=911216
That is a nightmare.

@classilla
Copy link
Owner Author

https://bug911216.bmoattachments.org/attachment.cgi?id=8727872 contains the base implementation plus some tests. We could ignore all the builtin/Promise.* stuff and just use the ones from 52ESR. The jsapi.* changes would generally apply with the differences in InvokeArgs and this version does implement js::CallSelfHostedFunction for us.

However, the changes in vm/SelfHosting.cpp are different from what is in ESR52. This would be a very big merge job.

@classilla
Copy link
Owner Author

Aborting the current parser work while I think this over. The other patches should be webcompatible.
aborted_521.txt

@classilla
Copy link
Owner Author

Let's use the machinery we do have to at least generate a warning when such functions are encountered.

At least we have DOM Promises so that's something (except finally which is probably not the issue at hand).

@roytam1
Copy link

roytam1 commented Feb 25, 2019

wonder if older round of async/await patches can be used and fix for final spec? https://hg.mozilla.org/mozilla-central/rev/2384726c541f

@classilla
Copy link
Owner Author

That's an interesting idea. It's of the right era. I'm not sure what the test failures are, but it's worth a shot.

@classilla
Copy link
Owner Author

It builds and passes most tests but I have some serious doubts about whether it will work properly in the browser mostly due to its hal-fassed Promise implementation. It fails these tests, but largely as a consequence of its shell implementation.

## js1_5/GC/regress-319980-01.js: rc = 3, run time = 0.669532
BUGNUMBER: 319980
STATUS: GC not called during non-fatal out of memory
This test should never fail explicitly. You must view the memory usage during the test. This test fails if memory usage for each subtest grows
js1_5/GC/regress-319980-01.js:39:3 TypeError: Intervals other than 0 are not supported
Stack:
  @js1_5/GC/regress-319980-01.js:39:3
REGRESSION - js1_5/GC/regress-319980-01.js
[ 4392|    1|    0|  228]  32% ===========>                           |6450.2s
## js1_5/GC/regress-319980-01.js: rc = 3, run time = 0.933304
BUGNUMBER: 319980
STATUS: GC not called during non-fatal out of memory
This test should never fail explicitly. You must view the memory usage during the test. This test fails if memory usage for each subtest grows
js1_5/GC/regress-319980-01.js:39:3 TypeError: Intervals other than 0 are not supported
Stack:
  @js1_5/GC/regress-319980-01.js:39:3
REGRESSION - js1_5/GC/regress-319980-01.js
[ 4669|    2|    0|  254]  34% ============>                          |6641.3s
## js1_5/Regress/regress-314401.js: rc = 3, run time = 0.649189
BUGNUMBER: 314401
STATUS: setTimeout(eval,0,"",null)|setTimeout(Script,0,"",null) should not crash
js1_5/Regress/regress-314401.js:23:3 ReferenceError: window is not defined
Stack:
  @js1_5/Regress/regress-314401.js:23:3
REGRESSION - js1_5/Regress/regress-314401.js
[ 4670|    3|    0|  254]  34% ============>                          |6642.3s
## js1_5/Regress/regress-314401.js: rc = 3, run time = 0.95136
BUGNUMBER: 314401
STATUS: setTimeout(eval,0,"",null)|setTimeout(Script,0,"",null) should not crash
js1_5/Regress/regress-314401.js:23:3 ReferenceError: window is not defined
Stack:
  @js1_5/Regress/regress-314401.js:23:3
REGRESSION - js1_5/Regress/regress-314401.js
[12990|    4|    0| 1098] 100% ======================================>|15033.8s
REGRESSIONS
    --baseline-eager js1_5/GC/regress-319980-01.js
    --ion-eager --ion-offthread-compile=off js1_5/GC/regress-319980-01.js
    --baseline-eager js1_5/Regress/regress-314401.js
    --ion-eager --ion-offthread-compile=off js1_5/Regress/regress-314401.js
FAIL

Next step is add a couple more tests from esr52 and then see if it actually works right in the browser with DOM Promises.

@classilla
Copy link
Owner Author

It doesn't spin the event loop properly, so sites relying on it don't work right. However, many sites that didn't load at all now at least parse, and some load further. Github now enables a couple more buttons. With #541 also loaded, the site is now complaining about supporting await async which is permitted by the spec and not addressed by the patch above, and it also doesn't like ()=>{!async function(){ ... }.

@classilla
Copy link
Owner Author

I've concluded I can't get the syntactic changes to work with this version. Backing out and putting the work here for reference.
issue521-failed-again.txt

roytam1 added a commit to roytam1/mozilla45esr that referenced this issue Jun 15, 2019
classilla added a commit that referenced this issue Aug 24, 2019
roytam1 added a commit to roytam1/mozilla45esr that referenced this issue Aug 26, 2019
roytam1 added a commit to roytam1/mozilla45esr that referenced this issue Aug 26, 2019
…fourfox#521 (comment):

- #521: baseline parser support for async/await, with toggle, without bytecode (passes tests) (0e5746aaf)
- #521: fix yield handling (includes M1305566 pts 4-7) (2d25f717b)
- #521: make async functions throw for compatibility when enabled (46b01b5d4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants