Issue 2152046: Don't return in CajaContentRewriter's finally clause (Closed)
Descriptionreturn inside a finally clause causes all uncaught exceptions inside the try clause to become disregarded.
See:
http://www.owasp.org/index.php/Return_Inside_Finally_Block
http://weblogs.java.net/blog/staufferjames/archive/2007/06/_dont_return_in.html
Patch Set 1 #Patch Set 2 : #
MessagesTotal messages: 7
Absolutely LGTM.
Sign in to reply to this message.
This change surfaces several (rather odd) unit test failures in
EndToEndTest. Do you see the same errors?
java.lang.AssertionError
at com.google.caja.lexer.FilePosition.<init>(FilePosition.java:52)
at
com.google.caja.lexer.SourceBreaks.toFilePosition(SourceBreaks.java:73)
at com.google.caja.lexer.FilePosition.span(FilePosition.java:137)
at
com.google.caja.parser.js.ObjProperty.<init>(ObjProperty.java:40)
at
com.google.caja.parser.js.ValueProperty.<init>(ValueProperty.java:17)
On Sun, Sep 12, 2010 at 10:24 PM, <jasvir@gmail.com> wrote:
> Absolutely LGTM.
>
> http://codereview.appspot.com/2152046/
>
Sign in to reply to this message.
I think I narrowed down the e2e caja test failures.
When the test gadget contains:
var tests = {
jsonStringifyTest: function() {}
}
I get the error: java.lang.AssertionError
at com.google.caja.lexer.FilePosition.<init>(FilePosition.java:52)
at
com.google.caja.lexer.SourceBreaks.toFilePosition(SourceBreaks.java:73)
at com.google.caja.lexer.FilePosition.span(FilePosition.java:137)
at com.google.caja.parser.js.ObjProperty.<init>(ObjProperty.java:40)
at com.google.caja.parser.js.ValueProperty.<init>(ValueProperty.java:17)
at
com.google.caja.parser.quasiliteral.DefaultValijaRewriter$42.fire(DefaultValijaRewriter.java:1343)
The test case "works" when the properties inside tests are removed.
On Mon, Sep 13, 2010 at 2:34 PM, Chirag Shah <chiragshah1@gmail.com> wrote:
> This change surfaces several (rather odd) unit test failures in
> EndToEndTest. Do you see the same errors?
>
> java.lang.AssertionError
> at
com.google.caja.lexer.FilePosition.<init>(FilePosition.java:52)
> at
com.google.caja.lexer.SourceBreaks.toFilePosition(SourceBreaks.java:73)
> at com.google.caja.lexer.FilePosition.span(FilePosition.java:137)
> at
com.google.caja.parser.js.ObjProperty.<init>(ObjProperty.java:40)
> at
com.google.caja.parser.js.ValueProperty.<init>(ValueProperty.java:17)
>
> On Sun, Sep 12, 2010 at 10:24 PM, <jasvir@gmail.com> wrote:
>> Absolutely LGTM.
>>
>> http://codereview.appspot.com/2152046/
>>
>
Sign in to reply to this message.
I am not able to test this right now but I suspect this is occurring because
FilePosition information is not being maintained correctly during the parse.
On 2010/09/14 18:22:12, chirag wrote:
> I think I narrowed down the e2e caja test failures.
>
> When the test gadget contains:
> var tests = {
> jsonStringifyTest: function() {}
> }
>
> I get the error: java.lang.AssertionError
> at com.google.caja.lexer.FilePosition.<init>(FilePosition.java:52)
> at
> com.google.caja.lexer.SourceBreaks.toFilePosition(SourceBreaks.java:73)
> at com.google.caja.lexer.FilePosition.span(FilePosition.java:137)
> at com.google.caja.parser.js.ObjProperty.<init>(ObjProperty.java:40)
> at
com.google.caja.parser.js.ValueProperty.<init>(ValueProperty.java:17)
> at
>
com.google.caja.parser.quasiliteral.DefaultValijaRewriter$42.fire(DefaultValijaRewriter.java:1343)
>
> The test case "works" when the properties inside tests are removed.
>
>
>
> On Mon, Sep 13, 2010 at 2:34 PM, Chirag Shah <mailto:chiragshah1@gmail.com>
wrote:
> > This change surfaces several (rather odd) unit test failures in
> > EndToEndTest. Do you see the same errors?
> >
> > java.lang.AssertionError
> > at
> com.google.caja.lexer.FilePosition.<init>(FilePosition.java:52)
> > at
> com.google.caja.lexer.SourceBreaks.toFilePosition(SourceBreaks.java:73)
> > at
com.google.caja.lexer.FilePosition.span(FilePosition.java:137)
> > at
> com.google.caja.parser.js.ObjProperty.<init>(ObjProperty.java:40)
> > at
> com.google.caja.parser.js.ValueProperty.<init>(ValueProperty.java:17)
> >
> > On Sun, Sep 12, 2010 at 10:24 PM, mailto: <jasvir@gmail.com> wrote:
> >> Absolutely LGTM.
> >>
> >> http://codereview.appspot.com/2152046/
> >>
> >
>
Sign in to reply to this message.
Ah thanks! The FilePosition information was inconsistent when constructing com.google.caja.parser.html.Dom with the org.w3c.dom.Document generated by Neko. - compiler.addInput(new Dom(root), javaGadgetUri); To work around the issue, I passed the serialized document to com.google.caja.parser.html.DomParser, and let the cajoler work with the fresh ParseTreeNode. + CharProducer cp = CharProducer.Factory.create(new StringReader(docContent), is); + DomParser parser = new DomParser(new HtmlLexer(cp), true, is, mq); + compiler.addInput(AncestorChain.instance(new Dom(parser.parseFragment())), javaGadgetUri); The e2e tests are now passing with this change!! Unfortunately this change does introduce a negative performance impact. Thanks, Chirag On Tue, Sep 14, 2010 at 1:25 PM, <jasvir@gmail.com> wrote: > I am not able to test this right now but I suspect this is occurring > because FilePosition information is not being maintained correctly > during the parse. > > On 2010/09/14 18:22:12, chirag wrote: >> >> I think I narrowed down the e2e caja test failures. > >> When the test gadget contains: >> var tests = { >> jsonStringifyTest: function() {} >> } > >> I get the error: java.lang.AssertionError >> at > > com.google.caja.lexer.FilePosition.<init>(FilePosition.java:52) >> >> at > > com.google.caja.lexer.SourceBreaks.toFilePosition(SourceBreaks.java:73) >> >> at > > com.google.caja.lexer.FilePosition.span(FilePosition.java:137) >> >> at > > com.google.caja.parser.js.ObjProperty.<init>(ObjProperty.java:40) >> >> at > > com.google.caja.parser.js.ValueProperty.<init>(ValueProperty.java:17) >> >> at > > com.google.caja.parser.quasiliteral.DefaultValijaRewriter$42.fire(DefaultValijaRewriter.java:1343) > >> The test case "works" when the properties inside tests are removed. > > > >> On Mon, Sep 13, 2010 at 2:34 PM, Chirag Shah > > <mailto:chiragshah1@gmail.com> wrote: >> >> > This change surfaces several (rather odd) unit test failures in >> > EndToEndTest. Do you see the same errors? >> > >> > java.lang.AssertionError >> > at >> com.google.caja.lexer.FilePosition.<init>(FilePosition.java:52) >> > at > > com.google.caja.lexer.SourceBreaks.toFilePosition(SourceBreaks.java:73) >> >> > at > > com.google.caja.lexer.FilePosition.span(FilePosition.java:137) >> >> > at > > com.google.caja.parser.js.ObjProperty.<init>(ObjProperty.java:40) >> >> > at > > com.google.caja.parser.js.ValueProperty.<init>(ValueProperty.java:17) >> >> > >> > On Sun, Sep 12, 2010 at 10:24 PM, mailto: <jasvir@gmail.com> wrote: >> >> Absolutely LGTM. >> >> >> >> http://codereview.appspot.com/2152046/ >> >> >> > > > > > > http://codereview.appspot.com/2152046/ >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||
