Bug 18774 – SQUIRRELFISH: print meaningful error messages

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Cameron Zwarich (cpst)]

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.

[Maciej Stachowiak]

This is causing the following failures:

  dom/* (116 of them)
  editing/selection/contenteditable-click-inside.html
  editing/selection/contenteditable-click-outside.html
  fast/forms/selected-index-assert.html
  fast/xpath/nsresolver-exception.xhtml
  platform/mac/fast/AppleScript/001.html
  tables/mozilla_expected_failures/core/captions1.html

[Cameron Zwarich (cpst)]

I'm assigning this to myself.

[Geoffrey Garen]

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.

[Geoffrey Garen]

<rdar://problem/5916152>

[Geoffrey Garen]

Not a SquirrelFishBlocker, since we can land on trunk without great exception messages.

[Anthony Ricaud]

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:
document.querySelectorAll('a').forEach()
TypeError: Value undefined does not allow function calls. 

[Oliver Hunt]

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'.
>

[Oliver Hunt]

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.

[Oliver Hunt]

Additional foo:
> var foo
undefined
> foo.b++
TypeError: Result of expression 'foo' [undefined] is not an object.
> ++foo.b
TypeError: Result of expression 'foo' [undefined] is not an object.

[Oliver Hunt]

> "someString"()
TypeError: Result of expression '"someString"' [someString] is not a function.
> new "someString"()
TypeError: Result of expression '"someString"' [someString] is not a constructor.
> 

[Cameron Zwarich (cpst)]

I'll review this.

[Cameron Zwarich (cpst)]

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?

[Oliver Hunt]

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