Bug 30412 – Web Inspector should get human readable DOM Exceptions

• Component: Web Inspector (Deprecated)
• Status: RESOLVED DUPLICATE

[Brian Weinstein]

In the inspector, when you get a DOM exception, there should be a plain text English explanation of the error condition.

[Brian Weinstein]

Created attachment 41251 [details]
DOM Exceptions on Page JavaScript content - not console

[Mark Rowe (bdash)]

Where are the descriptions of the exceptions from?

[Darin Adler]

Comment on attachment 41251 [details]
DOM Exceptions on Page JavaScript content - not console

> +    if (DOMCoreException* domException = toDOMCoreException(exception))
> +        errorMessage = domException->longDescription();
> +    else if (RangeException* rangeException = toRangeException(exception))
> +        errorMessage = rangeException->longDescription();
> +    else if (EventException* eventException = toEventException(exception))
> +        errorMessage = eventException->longDescription();
> +    else if (XMLHttpRequestException* xmlhttpException = toXMLHttpRequestException(exception))
> +        errorMessage = xmlhttpException->longDescription();
> +#if ENABLE(SVG)
> +    else if (SVGException* svgException = toSVGException(exception))
> +        errorMessage = svgException->longDescription();
> +#endif
> +#if ENABLE(XPATH)
> +    else if (XPathException* pathException = toXPathException(exception))
> +        errorMessage = pathException->longDescription();
> +#endif

I'd suggest putting all this logic into a toExceptionBase function that returns an ExceptionBase* or 0. I will need this same list, but having a separate function will make things a bit clearer if we have to do this elsewhere, and also factor out the call to the longDescription function.

> +String ExceptionBase::longDescription() const
> +{
> +    return toString() + ": " + m_description;
> +}

I think it would be better to just have a function returning m_description. The code to construct an error message by combining the "Error: " with the message and then with ": " and then with the description could go closer to where it's actually logged. I think having the format built into the exception object is not really so good. We have to do that in the case of toString but in this case we can, and should, factor it out.

What about test cases?

review- because of the lack of test cases for this new feature

[Brian Weinstein]

(In reply to comment #2)
> Where are the descriptions of the exceptions from?

Except for the ones below the FIXME, the descriptions are from the DOM specifications that define each of the exceptions. The DOM Core exceptions are from : http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-258A00AF

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 41251 [details])
> > +    if (DOMCoreException* domException = toDOMCoreException(exception))
> > +        errorMessage = domException->longDescription();
> > +    else if (RangeException* rangeException = toRangeException(exception))
> > +        errorMessage = rangeException->longDescription();
> > +    else if (EventException* eventException = toEventException(exception))
> > +        errorMessage = eventException->longDescription();
> > +    else if (XMLHttpRequestException* xmlhttpException = toXMLHttpRequestException(exception))
> > +        errorMessage = xmlhttpException->longDescription();
> > +#if ENABLE(SVG)
> > +    else if (SVGException* svgException = toSVGException(exception))
> > +        errorMessage = svgException->longDescription();
> > +#endif
> > +#if ENABLE(XPATH)
> > +    else if (XPathException* pathException = toXPathException(exception))
> > +        errorMessage = pathException->longDescription();
> > +#endif
> 
> I'd suggest putting all this logic into a toExceptionBase function that returns
> an ExceptionBase* or 0. I will need this same list, but having a separate
> function will make things a bit clearer if we have to do this elsewhere, and
> also factor out the call to the longDescription function.

This is a good idea, I will put this in a later patch.

> 
> > +String ExceptionBase::longDescription() const
> > +{
> > +    return toString() + ": " + m_description;
> > +}
> 
> I think it would be better to just have a function returning m_description. The
> code to construct an error message by combining the "Error: " with the message
> and then with ": " and then with the description could go closer to where it's
> actually logged. I think having the format built into the exception object is
> not really so good. We have to do that in the case of toString but in this case
> we can, and should, factor it out.

I'm a little unclear about what this comment means, I understand that there should be a description() function that returns m_description, but are you saying that I should build the long description from JSDOMBindings instead of having a longDescription() function? If that's what you mean, then I can do that as well.

> 
> What about test cases?

Test cases are a little tricky, because this isn't supposed to change the way the DOM exception is reported to JavaScript, just the way it is logged to the user. I am working on adding something to the newly landed inspector test cases, that should work nicely for this case.
> 
> review- because of the lack of test cases for this new feature

 Thanks for the good review! I will have a new patch up soon.

[Brian Weinstein]

The problem I'm running into writing a test case for this using the inspector is that I'd like to do something like:

try {
    document.appendChild();
} catch (e) {
    console.log(e);
}

And check that output in the expected result, however, that doesn't follow the same code path that was fixed in this test.

If I just put in an unchecked call to document.appendChild(), it follows the reportException code path, but breaks JavaScript in the rest of the test.

[Timothy Hatcher]

Why doesen't console.log(e) work? I would expect these descriptions to be available on the error object.

We also have support for Web Inspector tests now, take a look at:

http://trac.webkit.org/browser/trunk/LayoutTests/inspector/console-tests.html

[Brian Weinstein]

(In reply to comment #7)
> Why doesen't console.log(e) work? I would expect these descriptions to be
> available on the error object.
> 

I saw the inspector testing framework, and I tried to write a test like that, but  the issue is, as Sam explained to me, we can't change the exception toString method, because JavaScript developers look for that with how they handle their error messages.

console.log(exception) goes through the code path that calls the exception's toString method, the method I can't change.

When an unchecked exception happens (through something like <script>document.appendChild()</script>), this goes through a different code path  (reportException) which is updated in this patch to use the new long description. The other paths (console.log(exception), and generating an exception through the console) use the exception.toString method which is not supposed to be changed. That is why writing an inspector test isn't working well for me right now, because I would have to throw an uncaught exception with this patch, which doesn't seem like a good idea. I could try it in a new test though.

This patch isn't the whole fix, I also need to fix the cases of console.log(exception), and typing something that generates an exception into the console, and I'm not sure the best way to handle those.

[Brian Weinstein]

Created attachment 41311 [details]
Addresses Darin's Comments + Layout Tests

[Sam Weinig]

Comment on attachment 41311 [details]
Addresses Darin's Comments + Layout Tests

>  
> +ExceptionBase* toExceptionBase(JSC::JSValue value)
> +{
> +    if (DOMCoreException* domException = toDOMCoreException(value))
> +        return reinterpret_cast<ExceptionBase*>(domException);
> +    else if (RangeException* rangeException = toRangeException(value))
> +        return reinterpret_cast<ExceptionBase*>(rangeException);
> +    else if (EventException* eventException = toEventException(value))
> +        return reinterpret_cast<ExceptionBase*>(eventException);
> +    else if (XMLHttpRequestException* xmlHttpException = toXMLHttpRequestException(value))
> +        return reinterpret_cast<ExceptionBase*>(xmlHttpException);
> +#if ENABLE(SVG)
> +    else if (SVGException* svgException = toSVGException(value))
> +        return reinterpret_cast<ExceptionBase*>(svgException);
> +#endif
> +#if ENABLE(XPATH)
> +    else if (XPathException* pathException = toXPathException(value))
> +        return reinterpret_cast<ExceptionBase*>(pathException);
> +#endif
> +    
> +    return 0;
> +}

This is in the wrong place.  For the most part, the DOM should not know about the bindings.  This should instead be in a new file in WebCore/bindings/js/ called something like JSExceptionBase.h/cpp.

[Brian Weinstein]

> This is in the wrong place.  For the most part, the DOM should not know about
> the bindings.  This should instead be in a new file in WebCore/bindings/js/
> called something like JSExceptionBase.h/cpp.

Ok, I will fix that and upload a new patch.

[Brian Weinstein]

Created attachment 41332 [details]
Addresses Sam's Comment

[Darin Adler]

Comment on attachment 41332 [details]
Addresses Sam's Comment

I was going to suggest putting that function inside JSDOMBinding.cpp rather than suggesting creating a new file. A new file is OK though.

New source files contributed by Apple engineers normally should have the BSD-style license, not LGPL.

> +    if (DOMCoreException* domException = toDOMCoreException(value))
> +        return reinterpret_cast<ExceptionBase*>(domException);
> +    else if (RangeException* rangeException = toRangeException(value))
> +        return reinterpret_cast<ExceptionBase*>(rangeException);
> +    else if (EventException* eventException = toEventException(value))
> +        return reinterpret_cast<ExceptionBase*>(eventException);
> +    else if (XMLHttpRequestException* xmlHttpException = toXMLHttpRequestException(value))
> +        return reinterpret_cast<ExceptionBase*>(xmlHttpException);
> +#if ENABLE(SVG)
> +    else if (SVGException* svgException = toSVGException(value))
> +        return reinterpret_cast<ExceptionBase*>(svgException);
> +#endif
> +#if ENABLE(XPATH)
> +    else if (XPathException* pathException = toXPathException(value))
> +        return reinterpret_cast<ExceptionBase*>(pathException);
> +#endif

We don't use else after return. So you can remove all those else.

> +#ifndef JSExceptionBase_h
> +#define JSExceptionBase_h
> +
> +#include "ExceptionBase.h"
> +
> +namespace WebCore {
> +
> +ExceptionBase* toExceptionBase(JSC::JSValue);
> +
> +} // namespace WebCore
> +
> +#endif // JSExceptionBase_h

This header only needs a forward-declaration of the classes ExceptionBase and JSC::JSValue, it doesn't actually have to include ExceptionBase.h.

> +    "index or size was negative, or greater than the allowed value",

These descriptions look like sentences, so they should have a capital letter at the start and a period at the end.

> +        Adds tests for output of uncaught exceptions to the Web Inspector
> +        Console. Tests DOM exceptions 1, 3, and 8 (INDEX_SIZE_ERR, NOT_FOUND_ERR, and
> +        HIERARCHY_REQUEST_ERR).

Why not test them all? Is there any way to test more than one exception in the same test?

review+, but please consider the comments above

[Brian Weinstein]

(In reply to comment #13)
> (From update of attachment 41332 [details])
> I was going to suggest putting that function inside JSDOMBinding.cpp rather
> than suggesting creating a new file. A new file is OK though.
> 
> New source files contributed by Apple engineers normally should have the
> BSD-style license, not LGPL.

Fixed.

> 
> We don't use else after return. So you can remove all those else.
> 

Done.

> 
> This header only needs a forward-declaration of the classes ExceptionBase and
> JSC::JSValue, it doesn't actually have to include ExceptionBase.h.
> 

Done.

> > +    "index or size was negative, or greater than the allowed value",
> 
> These descriptions look like sentences, so they should have a capital letter at
> the start and a period at the end.

Fixed.

> 
> > +        Adds tests for output of uncaught exceptions to the Web Inspector
> > +        Console. Tests DOM exceptions 1, 3, and 8 (INDEX_SIZE_ERR, NOT_FOUND_ERR, and
> > +        HIERARCHY_REQUEST_ERR).
> 
> Why not test them all? Is there any way to test more than one exception in the
> same test?

I'm not sure how to test more than one in the same test, and the reason I didn't test them all is that I was unsure how to test them all. I will file a bug to add tests for all of the exceptions we are logging.

> 
> review+, but please consider the comments above

Thanks!

[Brian Weinstein]

Part one landed in http://trac.webkit.org/changeset/49723.

[Mark Rowe (bdash)]

This caused three regression tests to start failing on the buildbot: <http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r49726%20(6180)/results.html>.

[Pavel Feldman]

Inspector testing harness is only used for the tests that require validating the frontend state. I don't think your tests need that. Doesn't simple scenario where you initiate an error and compare the CONSOLE dump work?

[Alexey Proskuryakov]

This looks like a duplicate of bug 6745 to me.

[Brian Weinstein]

*** This bug has been marked as a duplicate of bug 6745 ***