Handling NullPointerException correctly in GadgetHtmlParser.parseDom()

Catching NullPointerException and encapsulating it in a GadgetException so that correct INTERNAL_SERVER_ERROR can be thrown when parseDomImpl() method fails.

Details:

Caja throws the following exception for some cases:

org.apache.shindig.gadgets.GadgetException: java.lang.NullPointerException
       at org.apache.shindig.gadgets.parse.GadgetHtmlParser.parseDom(GadgetHtmlParser.java:101)
       at com.google.gadgets.server.GoogleHtmlParserRouter.parseDom(GoogleHtmlParserRouter.java:43)
       at org.apache.shindig.gadgets.rewrite.MutableContent.getDocument(MutableContent.java:220)
       at org.apache.shindig.gadgets.rewrite.DomWalker$Rewriter.rewrite(DomWalker.java:153)
       at org.apache.shindig.gadgets.rewrite.DomWalker$Rewriter.rewrite(DomWalker.java:144)
       at org.apache.shindig.gadgets.rewrite.DefaultResponseRewriterRegistry.rewriteHttpResponse(DefaultResponseRewriterRegistry.java:55)
       at org.apache.shindig.gadgets.servlet.AccelHandler.doFetch(AccelHandler.java:94)
       at org.apache.shindig.gadgets.servlet.ProxyBase.fetch(ProxyBase.java:165)
       at org.apache.shindig.gadgets.servlet.HtmlAccelServlet.doGet(HtmlAccelServlet.java:63)
Caused by: java.lang.NullPointerException
       at com.google.caja.parser.html.CajaTreeBuilder.appendComment(CajaTreeBuilder.java:153)
       at com.google.caja.parser.html.CajaTreeBuilder.appendCommentToDocument(CajaTreeBuilder.java:147)
       at nu.validator.htmlparser.impl.TreeBuilder.comment(TreeBuilder.java:779)
       at com.google.caja.parser.html.Html5ElementStack.processComment(Html5ElementStack.java:447)
       at com.google.caja.parser.html.DomParser.parseDom(DomParser.java:520)
       at com.google.caja.parser.html.DomParser.parseFragment(DomParser.java:270)
       at com.google.caja.parser.html.DomParser.parseFragment(DomParser.java:251)
       at org.apache.shindig.gadgets.parse.caja.CajaHtmlParser.parseFragmentImpl(CajaHtmlParser.java:96)
       at org.apache.shindig.gadgets.parse.caja.CajaHtmlParser.parseDomImpl(CajaHtmlParser.java:51)
       at org.apache.shindig.gadgets.parse.GadgetHtmlParser.parseDom(GadgetHtmlParser.java:92)
       ... 35 more

[vikaas.arora, 2010-07-22 05:13:12]

Add some change description for this change.
Intercepting such unhandled exceptions and converting them to server error
(Internal error) is OK. Just add few liners on the case/scenarios when this
exception can be thrown.
Do we have appropriate bug raised against the underlying HtmlParser (Caja) here
?

http://codereview.appspot.com/1871046/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
(right):

http://codereview.appspot.com/1871046/diff/1/2#newcode98
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java:98:
} catch (NullPointerException e) {
Do we know the reason for this NullPointerException (thrown from
CajaHtmlParser).

[gagan.goku, 2010-07-23 02:33:32]

Yes, we have a bug raised with caja team

http://codereview.appspot.com/1871046/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
(right):

http://codereview.appspot.com/1871046/diff/1/2#newcode98
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java:98:
} catch (NullPointerException e) {
On 2010/07/22 05:13:13, vikaas.arora wrote:
> Do we know the reason for this NullPointerException (thrown from
> CajaHtmlParser).

Somewhere deep inside the caja code, an NPE is thrown on some inputs.
I have added the stack trace in the description now.

[vikaas.arora, 2010-07-23 03:24:41]

lgtm

[gagan.goku, 2010-07-23 10:26:58]

Please review.

[gagan.goku, 2010-07-23 10:29:07]

Please review this at http://codereview.appspot.com/1871046/show

Affected files:

java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java


Index:
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
===================================================================
---
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
  (revision 966379)
+++
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
  (working copy)
@@ -90,13 +90,14 @@
    if (document == null) {
      try {
        document = parseDomImpl(source);
-      } catch (GadgetException e) {
-        throw e;
      } catch (DOMException e) {
        // DOMException is a RuntimeException
        document = errorDom(e);
        HtmlSerialization.attach(document, serializerProvider.get(),
source);
        return document;
+      } catch (NullPointerException e) {
+        throw new
GadgetException(GadgetException.Code.INTERNAL_SERVER_ERROR,
+                                  "Caught exception in parseDomImpl", e);
      }

      HtmlSerialization.attach(document, serializerProvider.get(), source);

On Fri, Jul 23, 2010 at 3:56 PM, <gagan.goku@gmail.com> wrote:

> Please review.
>
>
> http://codereview.appspot.com/1871046/show
>

[anupama.dutta, 2010-07-26 11:56:55]

LGTM.

[johnfargo, 2010-07-26 18:36:08]

As discussed independently, this is definitely a bit of a kluge to accommodate
problems that exist in specific configurations of execution rather than being
well-encapsulated. Even so, I'll commit this with a TODO to consider removal, in
order to improve the error messaging for this common case.

[gagan.goku, 2010-07-26 18:47:49]

On 2010/07/26 18:36:08, johnfargo wrote:
> As discussed independently, this is definitely a bit of a kluge to accommodate
> problems that exist in specific configurations of execution rather than being
> well-encapsulated. Even so, I'll commit this with a TODO to consider removal,
in
> order to improve the error messaging for this common case.

Thanks John

[johnfargo, 2010-07-26 21:03:09]

committed

On 2010/07/26 18:47:49, gagan.goku wrote:
> On 2010/07/26 18:36:08, johnfargo wrote:
> > As discussed independently, this is definitely a bit of a kluge to
accommodate
> > problems that exist in specific configurations of execution rather than
being
> > well-encapsulated. Even so, I'll commit this with a TODO to consider
removal,
> in
> > order to improve the error messaging for this common case.
> 
> Thanks John