[Webkit-unassigned] [Bug 50708] Parse error during XSLT transformation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50708


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #75999|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #9 from Alexey Proskuryakov <ap at webkit.org>  2010-12-08 18:59:36 PST ---
(From update of attachment 75999)
View in context: https://bugs.webkit.org/attachment.cgi?id=75999&action=review

This looks great, thanks! I've had a number of trivial nitpicks, at least a few of which need to be fixed before landing.

I'm marking this r+, so a committer can fix these before landing. Or ideally, you could upload an updated patch, and then we'll land through commit-queue.

> WebCore/ChangeLog:11
> +        * xml/XSLTProcessorLibxslt.cpp:
> +        (WebCore::writeToVector):

This auto-generated list is intended to be completed with a brief per-function description of changes. Many active contributors don't do that, but it's a useful practice.

> WebCore/xml/XSLTProcessorLibxslt.cpp:50
>  #include <wtf/text/CString.h>
>  #include <wtf/Vector.h>
> +#include <wtf/unicode/UTF8.h>

A pre-existing issue: we sort headers in ASCII order (same as UNIX sort command or Xcode Sort Selection script). So upper case comes first.

> WebCore/xml/XSLTProcessorLibxslt.cpp:166
> +    if (len == 0)

WebKit style is "if (!len)".

> WebCore/xml/XSLTProcessorLibxslt.cpp:171
> +    UChar* bufferUChar;
> +    String stringBuffer(StringImpl::createUninitialized(len, bufferUChar));
> +    UChar* bufferUCharEnd = bufferUChar + len;

I think that StringBuffer would look better here. String::fromUTF8() code that you copied this from probably predates StringBuffer.

> WebCore/xml/XSLTProcessorLibxslt.cpp:173
> +    // Try converting into the buffer.

Why try? Can converting really fail?

Simply converting into a buffer doesn't seem worth a comment to me.

> WebCore/xml/XSLTProcessorLibxslt.cpp:176
> +    WTF::Unicode::ConversionResult result =
> +        WTF::Unicode::convertUTF8ToUTF16(&stringCurrent, buffer + len, &bufferUChar, bufferUCharEnd);

No need to wrap this line, it's short enough.

> WebCore/xml/XSLTProcessorLibxslt.cpp:177
> +    if (result == WTF::Unicode::conversionOK || result == WTF::Unicode::sourceExhausted) {

Common WebKit style it to have early return on error case, not to nest "if"s for success cases.

> WebCore/xml/XSLTProcessorLibxslt.cpp:183
>      return len;

Is this correct error handling? Per <http://xmlsoft.org/html/libxml-xmlIO.html#xmlOutputWriteCallback>, -1 should be returned.

Also, looks like we don't really ever expect any errors here, so ASSERT_NOT_REACHED() would be good, with or without release build handling.

> LayoutTests/ChangeLog:10
> +        * fast/xsl/xslt-bad-encoding-expected.txt: Added.
> +        * fast/xsl/xslt-bad-encoding.xml: Added.
> +        * fast/xsl/xslt-bad-encoding.xsl: Added.

This is not a good name for the test, as the source files are all properly encoded. I'd suggest something like "utf8-chunks.xml".

> LayoutTests/fast/xsl/xslt-bad-encoding.xml:30
> +ééééééééééééééééééééééééééééééééééééééééééééééééé

Is this enough to reliably trigger the bug under different versions of libxml2 and other XML back-ends? Maybe make this section longer, just to be safe?

> LayoutTests/fast/xsl/xslt-bad-encoding.xsl:3
> +				xmlns:xsl="http://www.w3.org/1999/XSL/Transform">

Our subversion pre-commit hooks will complain about tabs in files. Please convert to spaces.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.