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

JsBuiltIns show as funcName rather than Constructor.prototype.funcName in stack traces #5643

Closed
jackhorton opened this issue Aug 27, 2018 · 2 comments

Comments

@jackhorton
Copy link
Contributor

try {
  [1, 2, 3].forEach(() => { throw new Error(); });
} catch (e) {
  print(e.stack);
}

This shows a line for forEach (native code) with jsbuiltins enabled, while it shows Array.prototype.forEach (native code) with jsbuiltins disabled. Same for .filter.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Aug 27, 2018

This should be simple to fix - just need to update:
https://github.com/Microsoft/ChakraCore/blob/0b13df812da459410db7adde83e1b17ac12c4152/lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp#L357

I note that the update would have to be slightly different to accommodate Object.fromEntries (which is static) compared with the other builtins which are properties of the prototype.

@jackhorton
Copy link
Contributor Author

Yup -- I was going to wait for fromEntries before doing anything to it, since we can use the new prototype property to say whether to add ".prototype" to the middle of the string. Also, it looks like theres no standard for native functions (V8 shows Array.forEach, SM/Firefox show... nothing? Stack traces in FF don't make a ton of sense to me), it would just be to not change our existing behavior.

@kfarnung kfarnung added this to the Backlog milestone Sep 5, 2018
chakrabot pushed a commit that referenced this issue Sep 17, 2018
…d Intl

Merge pull request #5698 from jackhorton:jsbuiltins/cleanup

Fixes #5644
Fixes #5643
Fixes an unlogged issue where all JsBuiltIns were constructable by default.

This mainly merged a lot of the code between tagPublicLibraryFunction, registerFunction, and registerChakraLibraryFunction. However, there are a number of smaller updates too:
1. Stopped creating a new ScriptFunction on each JsBuiltIn registration. As far as I can tell, the only reason this existed before was to get a ScriptFunction without any framedisplay/environment, but since that can be set manually, its cheaper to just set it on the existing ScriptFunction.
1. Intl functions no longer have to duplicate their name in the tagPublicLibraryCode argument and in script. I have edited a few Intl functions to confirm it works -- not sure if its worth it to go through and make all of the functions anonymous now. I also wanted to investigate changing JavascriptLibrary::InitializeFunction to not set the name attribute for these anonymous jsbuiltin functions since the name will always be overridden, but I am not sure if its possible.
1. function length is now set using default parameters rather than manually/after the fact. Not sure if I like this better or worse than having the function macro list the length and have it set it explicitly. We could theoretically do the same optimization in InitializeFunction to avoid the extra property set.
1. JsBuiltIns now uses a shared/projected enum for function registration like Intl.
1. Ran into an error when using default parameters for functions marked explicitly as "use strict," and I found the error message misleading, so I changed it to be the same as V8's which I found clearer.
1. CheckArrayAndGetLen never hit its error case and reported a slightly worse-looking runtime/accidental error as a result when this == null

Still need to run this through test262 to make sure there are no regressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants