-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ES6 constructor returns class instead of object #1496
Comments
For what it's worth, I decided to look a little bit further into it, and was able to reproduce the error with the latest source code. I ran the script below via
The script is:
The output of the script is:
|
Awesome! Thanks @eebbing, having a simple repro that we can run with |
not sure if this helps (hopefully, it might), but this testcase can be further simplified by running to only compile
Both Inliner and Lower use In this case, IR after GlobOpt
|
@eebbing @ianwjhalliday @Krovatkin thanks for participating - could you help review the PR pls? |
… hit. Github issue chakra-core#1496 ES6 constructor returns class instead of object upon constructorCache hit. constructorCache->ctorHasNoExplicitReturnValue set to 'true' for ES6 class constructors, therefore opcode 'GetNewScObject' is optimized away in inliner. Fix by de-asserting Flags_HasNoExplicitReturnValue for ES6 class constructors.
… hit. Github issue chakra-core#1496 ES6 constructor returns class instead of object upon constructorCache hit. constructorCache->ctorHasNoExplicitReturnValue set to 'true' for ES6 class constructors, therefore opcode 'GetNewScObject' is optimized away in inliner. Fix by de-asserting Flags_HasNoExplicitReturnValue for ES6 class constructors.
… hit. Github issue chakra-core#1496 ES6 constructor returns class instead of object upon constructorCache hit. constructorCache->ctorHasNoExplicitReturnValue set to 'true' for ES6 class constructors, therefore opcode 'GetNewScObject' is optimized away in inliner. Fix by de-asserting Flags_HasNoExplicitReturnValue for ES6 class constructors.
Fix is merged. |
As of today with the current Fast Ring version of Edge (40.15025.1000), I am still seeing this issue, though the behavior changed slightly: In my test-case the problem reproduces as soon as I open the developer tools. It does not happen anymore when the tools stay closed. Once they are open, the constructor does not return the instance but the class. Is this expected? Is there a different version of Chakra running once the dev tools are open? |
Tried both (long and short) test cases above as JS code in HTML files, opened through Node http-server in Edge 40.15026.1000.0 with F12 opened. Couldn't repro. Do you have a repro you can share? Thanks! |
I'm a colleague of @yGuy . We have prepared a repro that works against our public URLs: https://gist.github.com/michabaur/2a627a3e7a5703dbd682b6ef3274e5dc. Just extract the two files, open test.html, and then open the dev tools. For us, this breaks after a few seconds in Edge 40.15025.1000 |
@michabaur @yGuy thanks for the test case. |
What Edge version/release will contain this fix? |
Fixes #161. Versions before 40 have a JIT bug where a constructor can return the class instead of an instance. See chakra-core/ChakraCore#1496 and chakra-core/ChakraCore#2532. Tested with http://jsfiddle.net/0k59gbL8/5/. Fails on 38, passes on 40.
Fixes #161. Versions before 40 have a JIT bug where a constructor can return the class instead of an instance. See chakra-core/ChakraCore#1496 and chakra-core/ChakraCore#2532. Tested with http://jsfiddle.net/0k59gbL8/5/. Fails on 38, passes on 40.
The code example below should log the numbers from 0 up to 500. Instead, the code breaks after about 100 iterations. The issue is that after about 100 iterations the constructor of class
B
returns the class itself instead of an instance ofB
.The fact that this issue only occurs after multiple iterations suggests to me that the bug is somewhere in the JIT engine.
By the way, the error disappears if the method body of
A.toB()
is replaced byreturn new B(this);
.I tested the following code in Microsoft Edge 38.14393.0.0 (Microsoft EdgeHTML 14.14393)
The text was updated successfully, but these errors were encountered: