Function.prototype.toString should not decompile the AST
https://bugs.webkit.org/show_bug.cgi?id=142853
Reviewed by Sam Weinig.
Source/JavaScriptCore:
To recover the function parameter string, Function.prototype.toString
decompiles the function parameters from the AST. This is bad for a few
reasons:
(1) It requires us to keep pieces of the AST live forever. This is an
awkward design and a waste of memory.
(2) It doesn't match Firefox or Chrome (because it changes whitespace
and ES6 destructuring expressions).
(3) It doesn't scale to ES6 default argument parameters, which require
arbitrarily complex decompilation.
(4) It can counterfeit all the line numbers in a function (because
whitespace can include newlines).
(5) It's expensive, and we've seen cases where websites invoke
Function.prototype.toString a lot by accident.
The fix is to do what we do for the rest of the function: Just quote the
original source text.
Since this change inevitably changes some function stringification, I
took the opportunity to make our stringification match Firefox's and
Chrome's.
* API/tests/testapi.c:
(assertEqualsAsUTF8String): Be more informative when this fails.
(main): Updated to match new stringification rules.
* bytecode/UnlinkedCodeBlock.cpp:
(JSC::UnlinkedFunctionExecutable::paramString): Deleted. Yay!
* bytecode/UnlinkedCodeBlock.h:
* parser/Nodes.h:
(JSC::StatementNode::isFuncDeclNode): New helper for constructing
anonymous functions.
* parser/SourceCode.h:
(JSC::SourceCode::SourceCode): Allow zero because WebCore wants it.
* runtime/CodeCache.cpp:
(JSC::CodeCache::getFunctionExecutableFromGlobalCode): Updated for use
of function declaration over function expression.
* runtime/Executable.cpp:
(JSC::FunctionExecutable::paramString): Deleted. Yay!
* runtime/Executable.h:
(JSC::FunctionExecutable::parameterCount):
* runtime/FunctionConstructor.cpp:
(JSC::constructFunctionSkippingEvalEnabledCheck): Added a newline after
the opening brace to match Firefox and Chrome, and a space after the comma
to match Firefox and WebKit coding style. Added the function name to
the text of the function so it would look right when stringify-ing. Switched
from parentheses to braces to produce a function declaration instead of
a function expression because we are required to exclude the function's
name from its scope, and that's what a function declaration does.
* runtime/FunctionPrototype.cpp:
(JSC::functionProtoFuncToString): Removed an old workaround because the
library it worked around doesn't really exist anymore, and the behavior
doesn't match Firefox or Chrome. Use type profiling offsets instead of
function body offsets because we want to include the function name and
the parameter string, rather than stitching them in manually by
decompiling the AST.
(JSC::insertSemicolonIfNeeded): Deleted.
* tests/mozilla/js1_2/function/tostring-1.js:
* tests/mozilla/js1_5/Scope/regress-185485.js:
(with.g): Updated these test results for formatting changes.
Source/WebCore:
* bindings/js/JSLazyEventListener.cpp:
(WebCore::JSLazyEventListener::initializeJSFunction): Adjust the line
number of attribute event listeners to account for the leading newline
now added by JavaScriptCore.
This solution is not perfect, but there are a lot of pre-existing problems
with line and column reporting for attribute event listeners, and this
preserves existing behavior with reasonable reliability.
LayoutTests:
Updated test results to match new rules for Function.prototype.toString.
* fast/dom/TreeWalker/acceptNode-filter-expected.txt: Removed a space
because it was not in the original source.
* fast/events/window-onerror2-expected.txt: Column number changed because
the event listener body starts on its own line now. This was a bit wrong
before and is still a bit wrong now in a different way.
* fast/profiler/dead-time-expected.txt:
* fast/profiler/inline-event-handler-expected.txt:
* fast/profiler/stop-profiling-after-setTimeout-expected.txt: Line number
changed because WebCore shifts line nubmers on attribute event listeners
by one.
* js/class-syntax-default-constructor-expected.txt: Constructor name
is not present now because it is not present in the source text. This
test failed before and it still fails now in a slightly different way.
* js/destructuring-assignment-expected.txt: Destructuring arguments now
match their source text faithfully.
* js/dfg-redundant-load-of-captured-variable-proven-constant-expected.txt:
Removed a space because it was not present in the original source text.
* js/dfg-resolve-global-specific-dictionary-expected.txt: Ditto.
* js/function-toString-semicolon-insertion-expected.txt: Removed.
* js/script-tests/function-toString-semicolon-insertion.js: Removed.
* js/function-toString-semicolon-insertion.html: Removed. This test checked
for a work-around that I have removed.
* js/object-literal-computed-methods-expected.txt:
* js/object-literal-methods-expected.txt: These tests fail because object
literal methods do not register their function names appropriately. This
was a pre-existing failure that is now more explicit.
* js/dom/JSON-parse-expected.txt:
* js/dom/JSON-stringify-expected.txt: Whitespace removed because it was
not present in the original.
* js/dom/dfg-strcat-over-objects-then-exit-on-it-expected.txt: Ditto.
* js/dom/function-prototype-expected.txt:
* js/dom/function-prototype.html: Ditto.
* js/dom/parse-error-external-script-in-new-Function-expected.txt: Line
changed by one due to new extra newline.
* js/dom/script-start-end-locations-expected.txt: Lines and columns
changed due to new extra newline.
* js/dom/toString-and-valueOf-override-expected.txt: Whitespace removed
because it was not present in the original.
* js/dom/script-tests/dfg-strcat-over-objects-then-exit-on-it.js: Ditto.
* js/kde/lval-exceptions-expected.txt: Ditto.
* js/script-tests/dfg-redundant-load-of-captured-variable-proven-constant.js: Ditto.
* js/script-tests/dfg-resolve-global-specific-dictionary.js: Ditto.
* platform/mac/http/tests/media/media-source/mediasource-sourcebuffer-mode-expected.txt: Ditto.
* storage/domstorage/localstorage/string-conversion-expected.txt: Ditto.
* storage/domstorage/sessionstorage/string-conversion-expected.txt: Ditto.
* userscripts/window-onerror-for-isolated-world-1-expected.txt:
* userscripts/window-onerror-for-isolated-world-2-expected.txt: Line numbers
changed because of new anonymous function formatting. These line numbers
were wrong before and they are still wrong now.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@181810 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/runtime/FunctionConstructor.cpp b/Source/JavaScriptCore/runtime/FunctionConstructor.cpp
index dbca868..76a7140 100644
--- a/Source/JavaScriptCore/runtime/FunctionConstructor.cpp
+++ b/Source/JavaScriptCore/runtime/FunctionConstructor.cpp
@@ -88,25 +88,26 @@
JSObject* constructFunctionSkippingEvalEnabledCheck(ExecState* exec, JSGlobalObject* globalObject, const ArgList& args, const Identifier& functionName, const String& sourceURL, const TextPosition& position)
{
- // Functions need to have a space following the opening { due to for web compatibility
- // see https://bugs.webkit.org/show_bug.cgi?id=24350
- // We also need \n before the closing } to handle // comments at the end of the last line
+ // How we stringify functions is sometimes important for web compatibility.
+ // See https://bugs.webkit.org/show_bug.cgi?id=24350.
String program;
if (args.isEmpty())
- program = ASCIILiteral("(function() { \n})");
+ program = makeString("{function ", functionName.string(), "() {\n\n}}");
else if (args.size() == 1)
- program = makeString("(function() { ", args.at(0).toString(exec)->value(exec), "\n})");
+ program = makeString("{function ", functionName.string(), "() {\n", args.at(0).toString(exec)->value(exec), "\n}}");
else {
StringBuilder builder;
- builder.appendLiteral("(function(");
+ builder.appendLiteral("{function ");
+ builder.append(functionName.string());
+ builder.append("(");
builder.append(args.at(0).toString(exec)->value(exec));
for (size_t i = 1; i < args.size() - 1; i++) {
- builder.append(',');
+ builder.append(", ");
builder.append(args.at(i).toString(exec)->value(exec));
}
- builder.appendLiteral(") { ");
+ builder.appendLiteral(") {\n");
builder.append(args.at(args.size() - 1).toString(exec)->value(exec));
- builder.appendLiteral("\n})");
+ builder.appendLiteral("\n}}");
program = builder.toString();
}
