Improve error messages in JSC

[Timothy Hatcher]

For:

var foo = {};
foo.bar();

Log: "foo.bar is not a function"

See: https://twitter.com/addyosmani/status/569157136137134081

[Geoffrey Garen]

I believe we have all the metadata to do this -- we just need to change the string format.

[Geoffrey Garen]

Maybe Saam can take this one down.

[Saam Barati]

(In reply to comment #2)
> Maybe Saam can take this one down.

Sure, but we already do something similar:

`
var x = {};
x.foo()
`
==>
"Exception: TypeError: undefined is not a function (evaluating 'x.foo()')"

Maybe we should just switch up the wording? I personally like it the way it is now, 
because it tells you both what the problem is and where the problem is 
(Chrome just says where it is, but not that it's undefined).
Maybe we should make the x.foo() part more prominent and not in parens, something like:

"Exception: TypeError: when evaluating 'x.foo()': undefined is not a function"

[Geoffrey Garen]

I think our current wording is more precise but less communicative.

"undefined is not a function" is a precise definition of the error processed by the VM.

"x.y is not a function" -- or, perhaps better, "x.y is undefined" -- is a communicative description of what the developer should go debug and/or fix.

I prefer the communicative approach even though it is less precise. No one has ever said, "I love C++ template error messages -- they are so precise!" An extreme example, but still relevant, I think.

I think the best option is text context followed by communicative error:

x.y(): x.y is undefined (TypeError)

[Saam Barati]

Created attachment 247212 [details]
work in progress

What do you guys think of the format of this change?
It would allow for easy modification in the future of 
error messages because we can just pass in a function
to do the rewriting.

The strings it produces now are:
"TypeError: evaluating 'x.bar()', undefined is not a function"
"TypeError: evaluating '(10)()', 10 is not a function"

We can change this specific text and play around with it, 
but more work needs to be done if we want to just extract "x.bar"
and also change the words "undefined/10/etc is not a function"
because the error object itself is created with this string.
We may need to tack on more meta data to the error message to 
capture the "undefined/10/etc".

Looking through the code that throws exceptions in the VM in
VM.cpp::throwException, what code paths will trigger
if (callFrame && callFrame->codeBlock())
to not be true?

Also, what code paths cause the code block to not have expression info?
i.e, when will "(!callFrame->codeBlock()->hasExpressionInfo())" be true?

[WebKit Commit Bot]

Attachment 247212 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:131:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:141:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 247212 [details]
work in progress

Attachment 247212 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5499415670292480

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
js/dom/exception-thrown-from-new.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
css3/selectors3/xhtml/css3-modsel-15c.xml
fast/regex/dom/cross-frame-callable.html
js/dom/basic-weakmap.html
fast/selectors/closest-general.html
http/tests/security/xss-DENIED-window-index-assign.html
js/typedarray-constructors.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html

[Build Bot]

Created attachment 247215 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 247212 [details]
work in progress

Attachment 247212 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6118480445177856

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
fast/dom/call-a-constructor-as-a-function.html
js/dom/exception-thrown-from-new.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
plugins/npruntime/plugin-scriptable-object-invoke-default.html
fast/regex/dom/cross-frame-callable.html
js/dom/basic-weakmap.html
fast/selectors/closest-general.html
css3/selectors3/xhtml/css3-modsel-15c.xml
http/tests/security/xss-DENIED-window-index-assign.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html
js/typedarray-constructors.html

[Build Bot]

Created attachment 247217 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Geoffrey Garen]

> Looking through the code that throws exceptions in the VM in
> VM.cpp::throwException, what code paths will trigger
> if (callFrame && callFrame->codeBlock())
> to not be true?

codeBlock() is null if the callee is a host function or host object.

[Geoffrey Garen]

Darn, seems like we do indeed need more metadata.

[Saam Barati]

(In reply to comment #12)
> Darn, seems like we do indeed need more metadata.

Yeah it depends on if we want to split out the "undefined"
from "undefined is not a function". Or we can hack it and
do some string processing when generating the error message. 

What do you think about the approach of keeping a function pointer 
member variable as a way to transform the error message? Obviously this
is a bit slower than before, but I'm not sure if it will be
a noticeable slow down (still need to run perf tests). It will also allow for
future flexibility. Not sure if it's worth it or not. 
Also, ErrorInstance would be 7 bytes larger.

[Geoffrey Garen]

> What do you think about the approach of keeping a function pointer 
> member variable as a way to transform the error message? Obviously this
> is a bit slower than before, but I'm not sure if it will be
> a noticeable slow down (still need to run perf tests). It will also allow for
> future flexibility. Not sure if it's worth it or not. 
> Also, ErrorInstance would be 7 bytes larger.

Size of error object doesn't matter, and string processing during throw is OK.

[Saam Barati]

Created attachment 247649 [details]
work in progress

I may want to abstract a couple more things into RuntimeType.
Still need to update the tests that deal w/ error messages.

This patch creates error messages like:

>>> var x = 20
undefined
>>> x()
Exception: TypeError: x is not a function, it is an Integer (20).
>>> var x = []
undefined
>>> x()
Exception: TypeError: x is not a function, it is an Object (Array).
>>> function Ctor(){}
undefined
>>> x = new Ctor
[object Object]
>>> x()
Exception: TypeError: x is not a function, it is an Object (Ctor).
>>> x = 20.2
20.2
>>> x()
Exception: TypeError: x is not a function, it is a Number (20.2).
>>> x = null
null
>>> x()
Exception: TypeError: x is not a function, it is null.
>>> x = undefined
undefined
>>> x()
Exception: TypeError: x is not a function, it is undefined.

[Saam Barati]

Created attachment 247706 [details]
work in progress

Should apply.

[WebKit Commit Bot]

Attachment 247706 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ErrorInstance.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:134:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 247706 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=247706&action=review

> Source/JavaScriptCore/runtime/ErrorInstance.h:54
> +    typedef String (*MessageGenerator) (const String& originalMessage, const String& sourceText, RuntimeType, SourceTextWhereErrorOccurred);

I need a better name for this.

> Source/JavaScriptCore/runtime/ErrorInstance.h:70
> +    RuntimeType m_runtimeType;

And a better name for this.

Any ideas?

[Build Bot]

Comment on attachment 247706 [details]
work in progress

Attachment 247706 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6347331972628480

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
http/tests/security/xss-DENIED-window-index-assign.html
js/dom/exception-thrown-from-new.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
css3/selectors3/xhtml/css3-modsel-15c.xml
fast/regex/dom/cross-frame-callable.html
js/dom/basic-weakmap.html
fast/selectors/closest-general.html
fast/dom/call-a-constructor-as-a-function.html
js/typedarray-constructors.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html

[Build Bot]

Created attachment 247729 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 247706 [details]
work in progress

Attachment 247706 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4899287775313920

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
http/tests/security/xss-DENIED-window-index-assign.html
js/dom/exception-thrown-from-new.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
plugins/npruntime/plugin-scriptable-object-invoke-default.html
fast/regex/dom/cross-frame-callable.html
js/dom/basic-weakmap.html
fast/selectors/closest-general.html
css3/selectors3/xhtml/css3-modsel-15c.xml
fast/dom/call-a-constructor-as-a-function.html
js/typedarray-constructors.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html

[Build Bot]

Created attachment 247730 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 248158 [details]
work in progress

Closer. Need to change a few tests inside run-javascriptcore-tests to reflect new error messages.

[Saam Barati]

(In reply to comment #23)
> Created attachment 248158 [details]
> work in progress
> 
> Closer. Need to change a few tests inside run-javascriptcore-tests to
> reflect new error messages.

Nevermind, looks like run-javascriptcore-tests pulls in the latest inside LayoutTests/js

[Saam Barati]

I'll look into getting a more useful message here too:

https://twitter.com/JosephPecoraro/status/574736713009926146

[Saam Barati]

Created attachment 248404 [details]
patch

[Joseph Pecoraro]

Comment on attachment 248404 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248404&action=review

Some drive by comments.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:145
> +        callFrame->vm().throwException(callFrame, createInvalidParameterError(callFrame, "Function.prototype.apply", "an Array-like object", arguments));

Nit: Should have a test covering this as well.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:109
> -        return jsCast<JSString*>(v.asCell());
> +        return jsString(&vm, makeString('"',  asString(v)->value(exec), '"'));

Should this escape double quotes in the string? Maybe truncate too? We should make sure there is a test covering this, since this looks like a change uncovered by the tests that are updated.

Nit: Also, since we know the string will have at least 2 characters you can use jsNontrivialString.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:190
> +    builder.append(" is not a function, it is ");

Nit: Any of the StringBuilder appends with a non-single-character can be appendLiteral.

> Source/JavaScriptCore/runtime/RuntimeType.cpp:16
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

Nit: We normally just have a 2 clause license nowadays. Unless you are using the 3 clause for a specific reason, new files typically have the 2 clause. See other copyright 2015 files. (Same comment applies to RuntimeType.h)

> Source/JavaScriptCore/runtime/RuntimeType.cpp:41
> +RuntimeType getRuntimeTypeForValue(JSValue v)
> +{
> +    RuntimeType ret;

Style: WebKit style is to not abbreviate variables names. These can be "value" and "returnType". Or you could early return in the sections below instead of the if/else chain and avoid the "ret" local.

> Source/JavaScriptCore/runtime/RuntimeType.cpp:86
> +    return "";

Nit: emptyString()

> Source/JavaScriptCore/runtime/RuntimeType.cpp:89
> +
> +

Nit: extra newline.

> Source/JavaScriptCore/runtime/RuntimeType.h:37
> +enum RuntimeType : uint8_t {

Is it worth adding Symbol type now? Would this hit the RELEASE_ASSERT_NOT_REACHED's in runtimeTypeAsString?

[Darin Adler]

Comment on attachment 248404 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248404&action=review

Need a new rebased version of this, since the patch does not apply.

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:36
> +    , m_sourceAppender(nullptr)
> +    , m_runtimeTypeForCause(TypeNothing)

Won’t need these if we initialize them in the header.

> Source/JavaScriptCore/runtime/ErrorInstance.h:70
> +    SourceAppender m_sourceAppender;
> +    RuntimeType m_runtimeTypeForCause;

Should initialize these here:

    SourceAppender m_sourceAppender { nullptr };
    RuntimeType m_runtimeTypeForCause { TypeNothing };

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:184
> +    size_t notAFunctionIdx = originalMessage.reverseFind(ASCIILiteral("is not a function"));

It’s strange that reverseFind is not overloaded so you can use a literal with it. It’s safer to use auto for the return type rather than size_t. In WebKit coding style we do not use abbreviations like "idx".

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:186
> +    String displayValue = originalMessage.left(notAFunctionIdx - 1);

Could use a StringView for this to avoid creating an unnecessary temporary string.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:190
> +    builder.append(" is not a function, it is ");

Should use appendLiteral.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:193
> +            builder.append("an ");

Should use appendLiteral.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:195
> +            builder.append("a ");

Should use appendLiteral.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:197
> +        builder.append(" (");

Should use appendLiteral.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:199
> +        builder.append(")");

Should use appendLiteral.

[Saam Barati]

(In reply to comment #27)
> Comment on attachment 248404 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248404&action=review
> 
> Some drive by comments.
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:145
> > +        callFrame->vm().throwException(callFrame, createInvalidParameterError(callFrame, "Function.prototype.apply", "an Array-like object", arguments));
> 
> Nit: Should have a test covering this as well.
> 
> > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:109
> > -        return jsCast<JSString*>(v.asCell());
> > +        return jsString(&vm, makeString('"',  asString(v)->value(exec), '"'));
> 
> Should this escape double quotes in the string? Maybe truncate too? We
> should make sure there is a test covering this, since this looks like a
> change uncovered by the tests that are updated.
Good point, I didn't think about this. I'm not certain what we should do
here. I'll think on it.

> 
> Nit: Also, since we know the string will have at least 2 characters you can
> use jsNontrivialString.
> 
> > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:190
> > +    builder.append(" is not a function, it is ");
> 
> Nit: Any of the StringBuilder appends with a non-single-character can be
> appendLiteral.
> 
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:16
> > + * 1.  Redistributions of source code must retain the above copyright
> > + *     notice, this list of conditions and the following disclaimer.
> > + * 2.  Redistributions in binary form must reproduce the above copyright
> > + *     notice, this list of conditions and the following disclaimer in the
> > + *     documentation and/or other materials provided with the distribution.
> > + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> > + *     its contributors may be used to endorse or promote products derived
> > + *     from this software without specific prior written permission.
> 
> Nit: We normally just have a 2 clause license nowadays. Unless you are using
> the 3 clause for a specific reason, new files typically have the 2 clause.
> See other copyright 2015 files. (Same comment applies to RuntimeType.h)
> 
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:41
> > +RuntimeType getRuntimeTypeForValue(JSValue v)
> > +{
> > +    RuntimeType ret;
> 
> Style: WebKit style is to not abbreviate variables names. These can be
> "value" and "returnType". Or you could early return in the sections below
> instead of the if/else chain and avoid the "ret" local.
> 
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:86
> > +    return "";
> 
> Nit: emptyString()
> 
> > Source/JavaScriptCore/runtime/RuntimeType.cpp:89
> > +
> > +
> 
> Nit: extra newline.
> 
> > Source/JavaScriptCore/runtime/RuntimeType.h:37
> > +enum RuntimeType : uint8_t {
> 
> Is it worth adding Symbol type now? Would this hit the
> RELEASE_ASSERT_NOT_REACHED's in runtimeTypeAsString?

I think this is worthy of its own patch. It's the next one on my list
and shouldn't be too difficult to implement.

Thanks for the comments. I'll address them. Also, I need
to go through and comment on a few things myself to get feedback.

[Saam Barati]

Comment on attachment 248404 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248404&action=review

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:141
> +    if (sourceText[idx] != ')') {

I want to make a test for this.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:156
> +        if (isInMultiLineComment)  {

I also want to make a test for this.

[Saam Barati]

(In reply to comment #28)
> Comment on attachment 248404 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248404&action=review

> 
> It’s strange that reverseFind is not overloaded so you can use a literal
> with it. It’s safer to use auto for the return type rather than size_t. In
> WebKit coding style we do not use abbreviations like "idx".
> 
I think I should drop the ASCIILiteral here,
it seems like the type conversion to reverseFind goes:
ASCIILiteral => const char* => WTFString
which doesn't make much sense.

[Saam Barati]

Created attachment 248432 [details]
patch

[Geoffrey Garen]

Comment on attachment 248432 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248432&action=review

> LayoutTests/js/exception-for-nonobject-expected.txt:7
> +PASS 1 instanceof {}.undefined threw exception TypeError: undefined is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof {}.undefined').

Is it possible to make this say "{}.undefined is not a function"?

This message is probably better off without "'instanceof' expects a Function as its argument. That seems redundant.

> LayoutTests/js/instance-of-immediates-expected.txt:6
> +PASS (1 instanceof 1) threw exception TypeError: 1 is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof 1').

Ditto.

> LayoutTests/js/typedarray-constructors-expected.txt:6
> +PASS Int8Array() threw exception TypeError: Int8Array is not a function, it is an Object (Function).

Is it possible to make this say "TypeError: Int8Array (Object) is not a function (evaluating 'Int8Array()')"?

[Build Bot]

Comment on attachment 248432 [details]
patch

Attachment 248432 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4919525124341760

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
fast/dom/call-a-constructor-as-a-function.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html
fast/regex/dom/cross-frame-callable.html
css3/selectors3/xhtml/css3-modsel-15c.xml
fast/selectors/closest-general.html
http/tests/security/xss-DENIED-window-index-assign.html
sputnik/Conformance/12_Statement/12.1_Block/S12.1_A4_T1.html

[Build Bot]

Created attachment 248437 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Saam Barati]

(In reply to comment #33)
> Comment on attachment 248432 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248432&action=review
> 
> > LayoutTests/js/exception-for-nonobject-expected.txt:7
> > +PASS 1 instanceof {}.undefined threw exception TypeError: undefined is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof {}.undefined').
> 
> Is it possible to make this say "{}.undefined is not a function"?
Should be doable.

> 
> This message is probably better off without "'instanceof' expects a Function
> as its argument. That seems redundant.
> 
> > LayoutTests/js/instance-of-immediates-expected.txt:6
> > +PASS (1 instanceof 1) threw exception TypeError: 1 is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof 1').
> 
> Ditto.
> 
> > LayoutTests/js/typedarray-constructors-expected.txt:6
> > +PASS Int8Array() threw exception TypeError: Int8Array is not a function, it is an Object (Function).
> 
> Is it possible to make this say "TypeError: Int8Array (Object) is not a
> function (evaluating 'Int8Array()')"?

Should we just specify that it's simply an Object or should we specify the name of constructor like:
TypeError: <call> (Constructor) is no a function (evaluation '<expr>')?
For example

function Foo() {}
var x = new Foo
x()

should this throw
TypeError: x (Foo) is not a function (evaluating 'x()')
or
TypeError: x (Object) is not a function (evaluating 'x()')

[Saam Barati]

Comment on attachment 248432 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248432&action=review

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:223
> +    return createError(exec, createTypeError, value, makeString("is not ", expectedType, ". '", op, "' expects ", expectedType, " as its argument."), defaultSourceAppender);

Looks like I also need to write a test for invalid parameter for "in" expressions

[Saam Barati]

(In reply to comment #33)
> Comment on attachment 248432 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248432&action=review
> 
> > LayoutTests/js/exception-for-nonobject-expected.txt:7
> > +PASS 1 instanceof {}.undefined threw exception TypeError: undefined is not a Function. 'instanceof' expects a Function as its argument. (evaluating '1 instanceof {}.undefined').
> 
> Is it possible to make this say "{}.undefined is not a function"?
Geoff, there is one stupid edge case that will make string processing here
not work. For example, all variations of instanceof occurring on left or right 
of operator. i.e
"instanceof" instanceof "instanceof"
"x" instanceof "instanceof"
"instanceof" instanceof "x"

Maybe it's just worth ignoring this?

Or we can only do it when we're sure that there is only one instanceof in the
source text (which will probably be as close to 100% of the time as we can get), 
like checking that find("instanceof") == reverseFind("instanceof").

[Build Bot]

Comment on attachment 248432 [details]
patch

Attachment 248432 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4976267011031040

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
http/tests/security/xss-DENIED-window-index-assign.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
plugins/npruntime/plugin-scriptable-object-invoke-default.html
fast/regex/dom/cross-frame-callable.html
css3/selectors3/xhtml/css3-modsel-15c.xml
fast/selectors/closest-general.html
fast/dom/call-a-constructor-as-a-function.html
sputnik/Conformance/12_Statement/12.1_Block/S12.1_A4_T1.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html

[Build Bot]

Created attachment 248440 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Geoffrey Garen]

> Should we just specify that it's simply an Object or should we specify the
> name of constructor like:
> TypeError: <call> (Constructor) is no a function (evaluation '<expr>')?
> For example
> 
> function Foo() {}
> var x = new Foo
> x()
> 
> should this throw
> TypeError: x (Foo) is not a function (evaluating 'x()')
> or
> TypeError: x (Object) is not a function (evaluating 'x()')

Hmmm...

I suppose the ideal solution is some kind of debug-only serialization for values. For example, a number might want to tell you that it is a number, and its value. Same for a boolean. An object might want to tell you its constructor's name, or perhaps some of its properties or something.

In this case, I don't love "Foo" as a stand-alone identifier because it's ambiguous between "the function named Foo" and "an object allocated by the function named Foo".

I guess maybe for now what we want is:

TypeError: x is not a function.

And long-term:

TypeError: x is not a function. (In 'x()', 'x' is { }).

And for other x's:
TypeError: x is not a function. (In 'x()', 'x' is { prop1: 12, prop2:... }).
TypeError: x is not a function. (In 'x()', 'x' is 7).
TypeError: x is not a function. (In 'x()', 'x' is false).
TypeError: x is not a function. (In 'x()', 'x' is function Foo(z) { var x = ...}).

[Saam Barati]

(In reply to comment #41)
> > Should we just specify that it's simply an Object or should we specify the
> > name of constructor like:
> > TypeError: <call> (Constructor) is no a function (evaluation '<expr>')?
> > For example
> > 
> > function Foo() {}
> > var x = new Foo
> > x()
> > 
> > should this throw
> > TypeError: x (Foo) is not a function (evaluating 'x()')
> > or
> > TypeError: x (Object) is not a function (evaluating 'x()')
> 
> Hmmm...
> 
> I suppose the ideal solution is some kind of debug-only serialization for
> values. For example, a number might want to tell you that it is a number,
> and its value. Same for a boolean. An object might want to tell you its
> constructor's name, or perhaps some of its properties or something.
> 
> In this case, I don't love "Foo" as a stand-alone identifier because it's
> ambiguous between "the function named Foo" and "an object allocated by the
> function named Foo".
> 
> I guess maybe for now what we want is:
> 
> TypeError: x is not a function.
> 
> And long-term:
> 
> TypeError: x is not a function. (In 'x()', 'x' is { }).
> 
> And for other x's:
> TypeError: x is not a function. (In 'x()', 'x' is { prop1: 12, prop2:... }).
> TypeError: x is not a function. (In 'x()', 'x' is 7).
> TypeError: x is not a function. (In 'x()', 'x' is false).
> TypeError: x is not a function. (In 'x()', 'x' is function Foo(z) { var x =
> ...}).

I think we should just do this now, and we can make it more comprehensive later.
But we have almost everything we need except greater fidelity for objects.
I think for objects we should just do:
TypeError: x is not a function. (In 'x()', 'x' is an instance of ConstructorName).

[Saam Barati]

Created attachment 248479 [details]
patch

(Uploading patch w/out tests so I can see which tests I need to run for these
 error messages b/c running all of run-webkit-tests are flaky on my machine).

[Saam Barati]

Created attachment 248481 [details]
patch

same as previous patch but w/ some of my own new tests.

[Build Bot]

Comment on attachment 248481 [details]
patch

Attachment 248481 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5286530079784960

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
http/tests/security/xss-DENIED-window-index-assign.html
js/dom/exception-thrown-from-new.html
js/exception-for-nonobject.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
plugins/npruntime/plugin-scriptable-object-invoke-default.html
fast/regex/dom/cross-frame-callable.html
css3/selectors3/xhtml/css3-modsel-15c.xml
js/instance-of-immediates.html
fast/selectors/closest-general.html
fast/dom/call-a-constructor-as-a-function.html
sputnik/Conformance/12_Statement/12.1_Block/S12.1_A4_T1.html
js/typedarray-constructors.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html

[Build Bot]

Created attachment 248486 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 248481 [details]
patch

Attachment 248481 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5052118280962048

New failing tests:
css3/selectors3/xml/css3-modsel-15c.xml
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
fast/dom/NodeList/nodelist-item-call-as-function.html
plugins/npruntime/object-from-destroyed-plugin.html
http/tests/security/xss-DENIED-window-index-assign.html
js/dom/exception-thrown-from-new.html
js/exception-for-nonobject.html
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.4/S15.2.4_A3.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/S15.1_A2_T1.html
sputnik/Conformance/13_Function_Definition/S13_A17_T2.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html
fast/regex/dom/cross-frame-callable.html
css3/selectors3/xhtml/css3-modsel-15c.xml
js/instance-of-immediates.html
fast/selectors/closest-general.html
fast/dom/call-a-constructor-as-a-function.html
sputnik/Conformance/12_Statement/12.1_Block/S12.1_A4_T1.html
js/typedarray-constructors.html

[Build Bot]

Created attachment 248487 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Oliver Hunt]

(In reply to comment #41)

> TypeError: x is not a function.
> 
> And long-term:
> 
> TypeError: x is not a function. (In 'x()', 'x' is { }).
> 
> And for other x's:
> TypeError: x is not a function. (In 'x()', 'x' is { prop1: 12, prop2:... }).
> TypeError: x is not a function. (In 'x()', 'x' is 7).
> TypeError: x is not a function. (In 'x()', 'x' is false).
> TypeError: x is not a function. (In 'x()', 'x' is function Foo(z) { var x =
> ...}).

We need to make sure that when producing these error messages we do not call into JS -- no implicit conversions, no getters, etc

[Saam Barati]

Created attachment 248548 [details]
patch

[Geoffrey Garen]

Comment on attachment 248548 [details]
patch

This is pretty awesome.

[Build Bot]

Comment on attachment 248548 [details]
patch

Attachment 248548 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5569067389812736

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
plugins/npruntime/object-from-destroyed-plugin.html
plugins/npruntime/plugin-scriptable-object-invoke-default.html

[Build Bot]

Created attachment 248551 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Geoffrey Garen]

Comment on attachment 248548 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248548&action=review

r=me if you fix the function name and the failing tests

> Source/JavaScriptCore/runtime/RuntimeType.h:47
> +RuntimeType getRuntimeTypeForValue(JSValue);

We reserve the word "get" for functions returning out parameters. This should just be runtimeTypeForValue.

[Saam Barati]

(In reply to comment #54)
> Comment on attachment 248548 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248548&action=review
> 
> r=me if you fix the function name and the failing tests
> 
> > Source/JavaScriptCore/runtime/RuntimeType.h:47
> > +RuntimeType getRuntimeTypeForValue(JSValue);
> 
> We reserve the word "get" for functions returning out parameters. This
> should just be runtimeTypeForValue.

I'm waiting on a build.
I need to look into one of the failing tests. Two are just new messages, but one seems to be failing. Not sure why yet.

[Saam Barati]

Created attachment 248640 [details]
patch

If this goes green on bots, I'll cq+

there are a few tests that have been finicky on my machine so I'm hoping they're all good on the bots.

[Build Bot]

Comment on attachment 248640 [details]
patch

Attachment 248640 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5210636262834176

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
plugins/npruntime/object-from-destroyed-plugin.html
plugins/npruntime/plugin-scriptable-object-invoke-default.html

[Build Bot]

Created attachment 248643 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 248644 [details]
patch

Lets give this another try.

I think one problem that is occurring is that my local machine is getting different constructor names than the bots in the error messages.
Not sure why this is.

[Build Bot]

Comment on attachment 248644 [details]
patch

Attachment 248644 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5409692427747328

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
plugins/npruntime/object-from-destroyed-plugin.html

[Build Bot]

Created attachment 248647 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Saam Barati]

JoePeck helped me diagnose the problem with the bots here.
The failed tests are the output of two tests run under WK1
instead of WK2.

Not really sure what to do here since the results are divergent.
Any advice?

[Geoffrey Garen]

> Not really sure what to do here since the results are divergent.
> Any advice?

If there's a good reason for the results to diverge, then we should just check in separate test expectations for WK1 vs WK2. Is there a good reason?

[Saam Barati]

(In reply to comment #63)
> > Not really sure what to do here since the results are divergent.
> > Any advice?
> 
> If there's a good reason for the results to diverge, then we should just
> check in separate test expectations for WK1 vs WK2. Is there a good reason?

I need to figure out if there is a good reason or not. Off the top
of my head, my guess is that there may be divergent code paths somewhere
and one path calls into one function that creates an error and the other
calls into another function that creates an error, and those functions
create error's with different "sourceAppender"s.

I'm finally on spring break so I should have some time to look
into it this week, and inevitably, figure out how wrong my above
hypothesis is.

[Saam Barati]

Created attachment 249300 [details]
patch

Hopefully this is the one.

[WebKit Commit Bot]

Comment on attachment 249300 [details]
patch

Clearing flags on attachment: 249300

Committed r181889: <http://trac.webkit.org/changeset/181889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Three tests need to have results updated on Windows now:

https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r181889%20(50590)/results.html

[Brent Fulgham]

(In reply to comment #68)
> Three tests need to have results updated on Windows now:
> 
> https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/
> r181889%20(50590)/results.html

Committed r181896: <http://trac.webkit.org/changeset/181896>

[Saam Barati]

(In reply to comment #69)
> (In reply to comment #68)
> > Three tests need to have results updated on Windows now:
> > 
> > https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/
> > r181889%20(50590)/results.html
> 
> Committed r181896: <http://trac.webkit.org/changeset/181896>

Awesome. Thanks for landing that!