The tests failed after my commit because a silly mistake on my part re-using existing constants in HtmlResponseWriterImpl for the CDATA strings, which contained a line return... hence breaking the tests assertions for the PartialReponseWriterImpl.
I am having some trouble with some corrupt SVN, but I will commit my re-fix and you will see how everything passes. Have some faith :) Bruno On 23 July 2010 11:28, Werner Punz <[email protected]> wrote: > Hi I have to add my 2c to the entire issue. > The main issue still is mainly at Primefaces and here is the reason, > primefaces uses an xml response type protocol in the background but does not > escape the cdata blocks like we and mojarra do in the ppr part. > Instead it just uses the standard response writer to emit xml and expects a > certain behavior type which is not clearly specified, to the worse I expect > that it runs still into some problems because of missing fixup code on its > side. > > Now I am doing some guessin ghere: > > Now if we suppress nested CDATA blocks there still will be usecases where > primefaces will fail (Just a guess, Primefaces might have fixed it, so if I > am wrong then beat me for it). The problem simply is if you use xml as > response format with a suppressing behavior: > > > imaging following valid html codeblock > > <script> > //<![CDATA[ > alert("<hello world>"); > //]]> > </script> > > or following > <script> > //<!-- > alert("]]>"); > //--> > </script> > > Now what happens is following if we embed the first case and suppress the > CDATA in our standard response writer > > <![CDATA[ > <script> > // > alert("<hello world>"); > // > </script> > ]> > > Which is very likely to fail in a parser in strict xhtml because > after the ppr processing following is sent to the engine: > <script> > // > alert("<hello world>"); > // > </script> > > The correct output would be here: > <![CDATA[ > <script> > //<![CDATA[ > alert("<hello world>"); > //]]><![CDATA[]]]]><![CDATA[[> > </script> > ]]> > > > The second case is even worse because it results on primefaces side im > <![CDATA[ > <script> > //<!-- > alert("]]>"); > //--> > </script> > ]]> > which is an absolute no go in xml because you can embed everything into > CDATA blockes except ]]> > > unless Primefaces does not do its own escaping on the strings (which it > very unlikely does not do. > > The correct output on this would look like: > > The second case is even worse because it results on primefaces side im > <![CDATA[ > <script> > //<!-- > alert("]]><![CDATA[]]]]><![CDATA[[>"); > //--> > </script> > ]> > > But both correct responses are only correct if you expect xml which in > itself embeds html code, so this is a no go for a standard response writer > which is expected just to emit xhtml/html code with nothing embedded. > > So as I said the problem is not as easy at the first look and the ultimate > fix must happen on the primefaces side here. > > The problem simply is if you use the html response writer for straight xml > encoding with embedded html what is happening here, then you run into those > problems, hence we have our own ppr response writer and also mojarra which > exactly deals with those kinds of problems on ppr level. > > Dont get me wrong I dont mind having a fix for nested CDATA blocks in the > html response writer, since the html response writer is for html this is > correct, but that won´t resolve all problems primefaces has here, because it > has to take into consideration of all this (and I have an eery feeling that > the suppressing fix on the Myfaces side has to be disabled for the ppr case > so that the ppr fixup code works properly because it delegates the html > response writer and fixes the issue afterwards) > > > > > Werner > > > > Am 22.07.10 20:11, schrieb Bruno Aranda: > >> But we are talking about the HtmlResponseWriterImpl, which outputs HTML. >> >> The fix I have done is in that class and it only checks if there is a >> CDATA already started before starting one when writing the scripts. It >> is slightly different to the original problem (about the HtmlResponse, >> which is different from the one in Mojarra). The fix simply checks if >> there is the CDATA around the script before opening another one inside >> the script. I think it is OK if we just do not nest CDATAs in the HTML >> response, even if it was allowed. >> >> And this fixes the problems with PrimeFaces too, without having to >> change the ResponseWriter or the PartialResponseWriterImpl... >> >> Bruno >> >> On 22 July 2010 16:59, Werner Punz <[email protected] >> <mailto:[email protected]>> wrote: >> >> Hia guys please also read up on my jira response. >> The entire thing is not as easy as it seems, because there are >> various ways a cdata block can be opened, first you can do it via >> startCDATA secondly you can do it via a direct write. >> >> I did some kind of double buffering in case of a cdata block was >> opened and then escaped the ]]> as multiple cdata blocks (the jira >> response explains the technique exactly) >> >> And yes there is somewhat a speed hit because of this, but in case >> of the partial response writer I did not have a chance because: >> >> But the partial response writer is somewhat different, because it >> has to press html code in a valid xml response format, so nested >> cdata blocks can occur naturally, the normal response writer is >> somewhat different because nested cdata blocks are only forbidden >> for xmlish output dialects others might allow it. >> >> Werner >> >> >> >> Am 22.07.10 17:47, schrieb Mark Struberg: >> >> >> But isn't the patch of Marcus Büttner doing this by maintaining >> a reference >> counter? >> >> Another question: how is the performance of all this >> scanning/dynamic >> replacement? >> >> LieGrue, >> strub >> >> >> From: Bruno Aranda<[email protected] >> <mailto:[email protected]>> >> >> To: MyFaces Development<[email protected] >> <mailto:[email protected]>> >> >> Sent: Thu, July 22, 2010 5:26:35 PM >> Subject: Re: Fixing ResponseWriter.startCDATA/endCDATA >> >> Further investigation of this incompatibility problem with >> myfaces leads me to >> the fact that in the HtmlResponseWriterImpl, when we write >> the content of a >> script, we create a CDATA element without checking if is >> nested at all. That is >> >> >> a problem, because if we use the standard response writer >> and we write a script >> >> >> section inside a CDATA section, the problem will be >> triggered... >> >> We need a way in HtmlResponseWriterImpl to check nested >> CDATA calls to the >> startCDATA or endCDATA methods I guess. >> >> Cheers, >> >> Bruno >> >> >> On 22 July 2010 15:15, Bruno Aranda<[email protected] >> <mailto:[email protected]>> wrote: >> >> >> Just clicked on sent and Werner had answered in the JIRA >> issue explaining the >> partial approach... >> >> >> Cheers, >> >> Bruno >> >> >> >> On 22 July 2010 15:12, Bruno >> Aranda<[email protected] >> <mailto:[email protected]>> wrote: >> >> >> As you can see in my black box tests with Mojarra, the >> behaviour is different in >> >> both implementations. In the base ResponseWriter class, >> they don't do anything >> >> >> in the startCDATA method and throw an undocumented >> exception in the endCDATA. >> >> >> In both implementations of the base class, they >> throw an exception if the >> startCDATA method is called and it had been called >> already... >> >> I don't quite understand our implementation of the >> PartialResponseWriterImpl. We >> >> do buffer nested CDATAs and write them when closing >> the parent one? This would >> >> >> still create nested CDATAs... I still need to >> understand this bit properly, >> >> Cheers, >> >> Bruno >> >> >> >> On 22 July 2010 13:58, Bruno >> Aranda<[email protected] >> <mailto:[email protected]>> wrote: >> >> >> yeah, sorry, my problem was running only the API >> tests :) >> >> >> Bruno >> >> >> >> On 22 July 2010 13:48, Matthias >> Wessendorf<[email protected] >> <mailto:[email protected]>> wrote: >> >> >> On Thu, Jul 22, 2010 at 2:14 PM, Matthias >> Wessendorf<[email protected] >> <mailto:[email protected]>> >> >> >> wrote: >> >> so, maybe there are now regressions? >> >> hrm. have you done some testing? >> >> >> Ah, the discussion is on the JIRA.. >> >> please run tests, before committing ;-) >> >> >> >> -M >> >> On Thu, Jul 22, 2010 at 2:07 PM, >> Matthias Wessendorf<[email protected] >> <mailto:[email protected]>> >> >> >> wrote: >> >> sounds right. >> >> does blame say more why it does not >> do nothing? >> >> It is also kinda strange since the >> TCK was successfully executed for >> 2.0.0 and 2.0.1; >> >> -Matthias >> >> On Thu, Jul 22, 2010 at 1:48 PM, >> Bruno Aranda<[email protected] >> <mailto:[email protected]>> >> >> >> wrote: >> >> Hi, >> >> Having problems with Primefaces >> again I have realised that >> something >> >> was >> >> working with Mojarra, but not >> with MyFaces. Again, is the >> ResponseWriter.startCDATA stuff >> which Primefaces invokes directly >> on >> >> its >> >> main phase listener. >> >> However, reading the javadocs: >> >> >> https://javaserverfaces.dev.java.net/nonav/docs/2.0/javadocs/index.html >> >> It says that method "should >> take no action when invoked"... >> which >> >> means >> >> that it should be completely >> empty as far as I understand. If >> that was >> >> the >> >> case, we would get the same >> behaviour in both >> implementations... >> >> Cheers, >> >> Bruno >> >> >> >> >> -- >> Matthias Wessendorf >> >> blog: >> >> http://matthiaswessendorf.wordpress.com/ >> sessions: >> http://www.slideshare.net/mwessendorf >> twitter: >> http://twitter.com/mwessendorf >> >> >> >> >> -- >> Matthias Wessendorf >> >> blog: >> http://matthiaswessendorf.wordpress.com/ >> sessions: >> http://www.slideshare.net/mwessendorf >> twitter: http://twitter.com/mwessendorf >> >> >> >> >> -- >> >> Matthias Wessendorf >> >> blog: http://matthiaswessendorf.wordpress.com/ >> sessions: >> http://www.slideshare.net/mwessendorf >> twitter: http://twitter.com/mwessendorf >> >> >> >> >> >> >> >> >> >> >> >> >> > >
