Bug 54802

Summary: Provide location information for exceptions thrown by JspDocumentParser [PATCH]
Product: Tomcat 7 Reporter: Konstantin Kolinko <knst.kolinko>
Component: JasperAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: enhancement    
Priority: P2    
Version: 7.0.39   
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Attachments: Text of Error page shown for bug 54801 by current trunk
Text of Error page shown for bug 54801 by trunk with r1464781
2013-04-05_tc8_54802_1.patch
Text of Error page shown for bug 54801 by trunk with r1464781 + patch

Description Konstantin Kolinko 2013-04-04 22:41:48 UTC
Steps to reproduce - see bug 54801

It causes an exception to be thrown by
org.apache.jasper.compiler.JspDocumentParser.checkScriptingBody()

The issue is that the exception in bug 54801 does not provide any information on the file where it occurred. It does not show in which tag file it happened, and thus it is hard to follow.

There are four places in JspDocumentParser where it throws a "SAXException" without any location information.  I propose to replace

  throw new SAXException(msg);

with

  throw new SAXParseException(msg, locator);

I am not sure that line numbers will be correct in all four cases, but at least it will provide the name of the file. This is better than nothing.
Comment 1 Konstantin Kolinko 2013-04-04 22:52:38 UTC
Created attachment 30154 [details]
Text of Error page shown for bug 54801 by current trunk

Text of error page. The tag file name is nowhere to be found.

In the "localhost.date.log" file only the inner org.xml.sax.SAXException is printed. The outer ones are shown by the error page only.
Comment 2 Konstantin Kolinko 2013-04-04 23:16:28 UTC
Created attachment 30155 [details]
Text of Error page shown for bug 54801 by trunk with r1464781

Fixed in trunk by r1464781

Here is what is now displayed by the error page. The "localhost.date.log" contains the same JasperException as shown by error page.

The good point is that the message includes the file name, as desired.

The bad point is that the original SAXParseException and its stack trace are not displayed and are not logged. The "JspDocumentParser.checkScriptingBody()" method where the original exception were thrown is not mentioned.
Comment 3 Konstantin Kolinko 2013-04-05 15:10:56 UTC
Created attachment 30157 [details]
2013-04-05_tc8_54802_1.patch

A patch that improves information shown by error page and logged.

> but at least it will provide the name of the file

First, the above assumption is actually incorrect. A locator does not provide the file name. It knows line and column numbers only  (and systemId, publicId of the resource).

The name was provided by  catch(SAXParseException) clause  at
JspDocumentParser.parse(JspDocumentParser.java:205)

Still, I would say that line numbers are useful.


Second, the original exception was lost in the same place (JspDocumentParser.java:205). It is easy to fix by adding a new method to jasper.compiler.DefaultErrorHandler. See the patch.

Third, the logging is done in StandardWrapper.invoke(). It logged only the innermost exception in ex.getCause() chain. I am saying that it is wrong as
 1. A usual rule is that outer exceptions provide additional contextual information.
 2. The error page shows the full exception chain. Omitting it in the log message is inconsistent and leaves administrator without a clue.


Though one of my assumptions was wrong, as I mention above, I plan to leave r1464781 as is and go on with changes outlined in this patch.
Comment 4 Konstantin Kolinko 2013-04-05 15:14:10 UTC
Created attachment 30158 [details]
Text of Error page shown for bug 54801 by trunk with r1464781 + patch
Comment 5 Konstantin Kolinko 2013-04-09 16:58:12 UTC
Jasper part of this is fixed in 7.0.x by r1466122 and will be in 7.0.40.


> Third, the logging is done in StandardWrapper.invoke(). It logs only the
> innermost exception in ex.getCause() chain. I am saying that it is wrong as
>  1. A usual rule is that outer exceptions provide additional contextual
> information.
>  2. The error page shows the full exception chain. Omitting it in the log
> message is inconsistent and leaves administrator without a clue.


The StandardWrapper part of this has not been resolved yet.
Comment 6 Mark Thomas 2018-12-03 20:44:39 UTC
In this instance the root cause exception contains all the information necessary to find the bug.

I've done some svn archaeology and the root cause unwrapping was added in r303681 and was added for 5.5.x. Looking at the some mailing list discussions that followed, one of the reasons this was added was that Jasper was wrapped the root cause without adding useful info and the Jasper exception was being logged.

I did find a few issues because of this unwrapping but they are all fairly old and have all been resolved.

My general sense re-reading those issues was that - on balance - the unwrapping is useful and we'd address on a case by case basis any instances where the wrapping caused issues such as information loss. On that basis I'm intending to resolve this issue as fixed but I'll leave it open for at least a week to give folks a chance to review it.
Comment 7 Mark Thomas 2018-12-14 17:40:42 UTC
Resolving this as (sufficiently) fixed.