Bug 18774

Summary: SQUIRRELFISH: print meaningful error messages
Product: WebKit Reporter: Cameron Zwarich (cpst) <zwarich>
Component: JavaScriptCoreAssignee: Oliver Hunt <oliver>
Severity: Normal CC: ggaren, mjs, oliver, rik, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Description Flags
Patch o' doom zwarich: review+

Description Cameron Zwarich (cpst) 2008-04-27 12:39:04 PDT
Currently, many error messages include a textual representation of the expression being evaluated. This doesn't happen with SquirrelFish, which is the cause of the vast majority of the layout test failures. We currently intercept thrown exceptions and add line and sourceURL information, which is not used when printing the exception. Maciej suggested that we use this information when printing the exception and add column information as well. This will require tracking columns in the parser and storing them in the nodes.
Comment 1 Maciej Stachowiak 2008-05-03 01:25:11 PDT
This is causing the following failures:

  dom/* (116 of them)
Comment 2 Cameron Zwarich (cpst) 2008-05-04 21:25:54 PDT
I'm assigning this to myself.
Comment 3 Geoffrey Garen 2008-05-06 21:15:34 PDT
The error message that was popular in the dom/* tests was

"TypeError: Value undefined (result of expression acronymElem.setIdAttributeNS) is not object."

This happens when converting a value to object, usually for the sake of a function call.

Trunk provides many other sorts of contextual decompilation, though. The best way to see them all is probably to peruse nodes.cpp.
Comment 4 Geoffrey Garen 2008-05-06 21:17:11 PDT
Comment 5 Geoffrey Garen 2008-05-06 21:23:06 PDT
Not a SquirrelFishBlocker, since we can land on trunk without great exception messages.
Comment 6 Anthony Ricaud 2008-06-01 15:37:43 PDT
For reference, Firefox says things like that :
>>> document.querySelectorAll('a').forEach()
TypeError: document.querySelectorAll is not a function
>>> document.getElementsByTagName('a').forEach()
TypeError: document.getElementsByTagName("a").forEach is not a function

while WebKit r34271 says:
TypeError: Value undefined does not allow function calls. 
Comment 7 Oliver Hunt 2008-07-18 06:30:58 PDT
Created attachment 22359 [details]
Patch o' doom

Here we go, error messages that Do Not Suck (tm)

A few issues -- I haven't yet fixed all the error messages so some, particularly syntax errors, still do not produce awe inspiring messages, but hey have all the information required to do so.

All the standard errors are now good and wonderful, and all have the additional character position information attached.

The error messages are currently of the form
> undefined.b++
TypeError: Result of expression 'undefined' [undefined] is not an object.
> undefined[0]
TypeError: Result of expression 'undefined' [undefined] is not an object.
> undefined()
TypeError: Result of expression 'undefined' [undefined] is not a function.
> new undefined()
TypeError: Result of expression 'undefined' [undefined] is not a constructor.
> 1 in undefined
TypeError: Result of expression 'undefined' [undefined] is not a valid argument for 'in'.
> 1 instanceof undefined
TypeError: Result of expression 'undefined' [undefined] is not a valid argument for 'instanceof'.
Comment 8 Oliver Hunt 2008-07-18 06:32:43 PDT
OH, and this effects a nmber of layout tests that are looking at what the error messages are, however until  i get the go ahead on the current template i won't update the test results.
Comment 9 Oliver Hunt 2008-07-18 06:37:36 PDT
Additional foo:
> var foo
> foo.b++
TypeError: Result of expression 'foo' [undefined] is not an object.
> ++foo.b
TypeError: Result of expression 'foo' [undefined] is not an object.

Comment 10 Oliver Hunt 2008-07-18 06:38:55 PDT
> "someString"()
TypeError: Result of expression '"someString"' [someString] is not a function.
> new "someString"()
TypeError: Result of expression '"someString"' [someString] is not a constructor.

Comment 11 Cameron Zwarich (cpst) 2008-07-18 11:46:28 PDT
I'll review this.
Comment 12 Cameron Zwarich (cpst) 2008-07-18 14:02:18 PDT
Comment on attachment 22359 [details]
Patch o' doom

+int CodeBlock::lineNumberForVPC(const Instruction* vPC) {

The opening brace should be on the next line.

+        * kjs/SourceRange.h:

This is in the wrong position of the header list.

+                divot = 0; // Overflow has occurred, we can only give line number info for errors for this region

Might as well put the comment on its own line(s) like all the others in this if-else statement.

+    JSObject* exception = Error::create(exec, TypeError, createErrorMessage(exec, codeBlock, line, divotPoint, divotPoint + endOffset, value, message), line, codeBlock->ownerNode->sourceId(), codeBlock->ownerNode->sourceURL());

Maybe it would be better to break off the createErrorMessage() call into its own line, because this line is already quite long. This goes for the other creations of errors as well.

+        int32_t m_startOffset;

I don't think you want the extra line here.

Other than that, it looks good, so r=me with those changes. Do you have any tests for your new exception messages and line/column info?
Comment 13 Oliver Hunt 2008-07-18 18:46:25 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/JavaScriptCore.exp
	M	JavaScriptCore/VM/CodeBlock.cpp
	M	JavaScriptCore/VM/CodeBlock.h
	M	JavaScriptCore/VM/CodeGenerator.cpp
	M	JavaScriptCore/VM/CodeGenerator.h
	M	JavaScriptCore/VM/ExceptionHelpers.cpp
	M	JavaScriptCore/VM/ExceptionHelpers.h
	M	JavaScriptCore/VM/Machine.cpp
	M	JavaScriptCore/VM/Machine.h
	M	JavaScriptCore/kjs/DebuggerCallFrame.cpp
	M	JavaScriptCore/kjs/Error.cpp
	M	JavaScriptCore/kjs/Error.h
	M	JavaScriptCore/kjs/JSGlobalObjectFunctions.cpp
	M	JavaScriptCore/kjs/JSImmediate.cpp
	M	JavaScriptCore/kjs/JSNotAnObject.h
	M	JavaScriptCore/kjs/JSObject.h
	M	JavaScriptCore/kjs/Parser.h
	M	JavaScriptCore/kjs/SourceRange.h
	M	JavaScriptCore/kjs/grammar.y
	M	JavaScriptCore/kjs/lexer.cpp
	M	JavaScriptCore/kjs/lexer.h
	M	JavaScriptCore/kjs/nodes.cpp
	M	JavaScriptCore/kjs/nodes.h
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/css/resources/font-face-descriptor-multiple-values-parsing.js
	M	LayoutTests/fast/dom/SelectorAPI/dumpNodeList-expected.txt
	M	LayoutTests/fast/forms/select-namedItem-expected.txt
	A	LayoutTests/fast/js/exception-expression-offset.html
	A	LayoutTests/fast/js/resources/exception-expression-offset.js
	M	LayoutTests/fast/xsl/transform-xhr-doc-expected.txt
	M	LayoutTests/http/tests/security/aboutBlank/xss-DENIED-navigate-opener-document-write-expected.txt
	M	LayoutTests/http/tests/security/aboutBlank/xss-DENIED-navigate-opener-javascript-url-expected.txt
	M	LayoutTests/http/tests/security/aboutBlank/xss-DENIED-set-opener-expected.txt
	M	LayoutTests/http/tests/security/cross-frame-access-call.html
	M	LayoutTests/platform/mac/fast/events/updateLayoutForHitTest-expected.txt
	M	LayoutTests/platform/mac/svg/custom/createelement-expected.txt
	M	LayoutTests/platform/mac/tables/mozilla/bugs/bug53690-1-expected.txt
	M	LayoutTests/platform/mac/tables/mozilla_expected_failures/bugs/bug92868_1-expected.txt
Committed r35245