Bug 82577

Ha, did the fix of libxml finally land in lion? :) We have some hacks in the controller of libxml because of bugs in older versions of libxml, which where shipped till SL. IIRC we already have a bunch of these bug reports about broken entity support in newer versions of libxml.

(In reply to comment #1) > Ha, did the fix of libxml finally land in lion? :) We have some hacks in the controller of libxml because of bugs in older versions of libxml, which where shipped till SL. IIRC we already have a bunch of these bug reports about broken entity support in newer versions of libxml. No Lion includes libxml 2.7.3, which does not include the fix. We need to add a new workaround for that :( I have a patch ready, need to tidy it up and submit.

Created attachment 134993 [details] Patch

Uploading my workaround with --suggest-reviewers, looking for comments! I tried to do it as un-intrusive as possible, I hope it's fine as-is.

Forgot to say: the actual patch is only 7k, the rest are rebaselines - no worries ;-)

Comment on attachment 134993 [details] Patch Can you create a USE(LIBXML_ENTITY_PARSER_WORKAROUND) ifdef and put all this code behind the ifdef? That will make it easier for us to remove it in the future.

(In reply to comment #6) > (From update of attachment 134993 [details]) > Can you create a USE(LIBXML_ENTITY_PARSER_WORKAROUND) ifdef and put all this code behind the ifdef? That will make it easier for us to remove it in the future. I followed the existing style for another libxml workaround in the same file. I don't see the benefit of adding USE() macro support for hacks like this - if you disable it in Platform.h at some point, you still need to remove it from the cpp file as well. If I shall change it, the other libxml workaround needs to be changed as well. Is it worth it?

Comment on attachment 134993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134993&action=review > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:548 > + , m_depthTriggeringEntityExpansion(-1) > + , m_isParsingEntityDeclaration(false) Seems these should also be removed at compile time if the workaround is not in place. > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:743 > +#if LIBXML_VERSION >= 20704 Which suggests that this needs to be turned into a #define WORKAROUND_ENTITY_PARSING_BUG or similar, so it can be re-used as a compile time check in various places throughtout this file.

(In reply to comment #6) > (From update of attachment 134993 [details]) > Can you create a USE(LIBXML_ENTITY_PARSER_WORKAROUND) ifdef and put all this code behind the ifdef? That will make it easier for us to remove it in the future. I think that this patch makes removing the hack pretty easy already, since the things it does are guarded by hackAroundLibXMLEntityParsingBug(). Furthermore, the behavior of hackAroundLibXMLEntityParsingBug() already depends on the libxml version, and already has a comment describing that it is a workaround for a bug that existed before 2.7.4. I also think it's good for code quality to have this patch use a guard that can be checked using a C++ conditional rather than a C preprocessor conditional, since the latter tends to make the code ugly. Though in this patch, it may not make a big difference, and if it were me, I'd probably have used C preprocessor conditionals, for the reasons that Eric stated. The thing that C preprocessor conditionals instead of C++ conditionals would give us here is to remove the 8 bytes of data that seem to be added to the XMLDocumentParser class. But that does not seem like much, since the class already has a lot of state. In particular, most of the other cruft that this code adds ought to be removed at compile-time already. hackAroundLibXMLEntityParsingBug() is a static inline that returns a constant, so that should result in simplification of control flow. entityDeclarationHandler() is the only source of time overhead; at best it will be a tail call, leading to a useless jump. At worst, it'll be a handful of instructions that do some stack fiddling. But I can't imagine that matters much. Nikolas, do you have insight into what the performance and memory usage impacts of this patch are?

> Which suggests that this needs to be turned into a #define WORKAROUND_ENTITY_PARSING_BUG or similar, so it can be re-used as a compile time check in various places throughtout this file. This seems like a better approach to me, too.

(In reply to comment #9) > (In reply to comment #6) > > (From update of attachment 134993 [details] [details]) > > Can you create a USE(LIBXML_ENTITY_PARSER_WORKAROUND) ifdef and put all this code behind the ifdef? That will make it easier for us to remove it in the future. > > I think that this patch makes removing the hack pretty easy already, since the things it does are guarded by hackAroundLibXMLEntityParsingBug(). Furthermore, the behavior of hackAroundLibXMLEntityParsingBug() already depends on the libxml version, and already has a comment describing that it is a workaround for a bug that existed before 2.7.4. Completely agreed. > Nikolas, do you have insight into what the performance and memory usage impacts of this patch are? Nope, I didn't measure any performance with this patch. Do you think this should be tested? Can you suggest a file that should be benchmarked?

(In reply to comment #10) > > Which suggests that this needs to be turned into a #define WORKAROUND_ENTITY_PARSING_BUG or similar, so it can be re-used as a compile time check in various places throughtout this file. > > This seems like a better approach to me, too. I just followed the existing pattern, as I didn't specifically want to avoid initializing the variables if not needed. I thought it's not worth it.

Any further comments?

Comment on attachment 134993 [details] Patch Sorry for the delay! I've been distracted this week. R=me.

Filip, any thoughts on Comment #6, Comment #8, and Comment #10 ?

(In reply to comment #15) > Filip, any thoughts on Comment #6, Comment #8, and Comment #10 ? #6, #8: see my previous reply. #10: spoke to Alexey in person. He still views Nikolas' style as being somewhat unusual. I still view it as being desirable, because it reduces #ifdef's, which make the code harder to read. I don't think these stylistic issues are so grotesque as to preclude this patch from landing. I don't think that using "if (fooBar()) { ... }" makes it any harder to remove the code than using "#if FOO_BAR \n ... \n #endif". I don't think there is a performance implication to using "if (fooBar())" instead of "#if FOO_BAR", since the function in question is static inline. I haven't heard further replies from you or Eric, and nobody flagged the patch as r-. This led me to believe that you didn't feel strongly enough about your views on the style of if statement being used to block the patch from landing as-is. The patch has sat around for long enough that I decided that it would be better to let the patch land than to continue arguing about these small things. If you do feel strongly about preferring "#if FOO_BAR" over "if (fooBar())" then I suppose there is still time to r- the patch. It's your call. I like the patch as is and would rather err on the side of letting it land but I don't have strong feelings either way.

For what it's worth, #if's *are* easier to remove since you can simply feed the files in question through unifdef.

(In reply to comment #17) > For what it's worth, #if's *are* easier to remove since you can simply feed the files in question through unifdef. I'll take care of removing the code once Lion ships with a newer libxml, okay? Thanks for the review Filip! I'll land it as-is today. I can rework the patch to use ifdefs but only if we change the other libxml bug workaround to the same style, as a mixture doesn't make much sense. Agreed?

Committed r113299: <http://trac.webkit.org/changeset/113299>

This appears to have made a bunch of tests start failing, esp. some in css3/selectors3/xml

Rolled out in r113385 because of large number of failing tests. Reopening bug.

(In reply to comment #20) > This appears to have made a bunch of tests start failing, esp. some in css3/selectors3/xml *sigh* I forgot that only cr-linux EWS runs tests. The cr-linux slave has a libxml2 version > 2.7.3. I'll investigate.

(In reply to comment #20) > This appears to have made a bunch of tests start failing, esp. some in css3/selectors3/xml Funny, css3/selectors3/xhtml/css3-modsel-120.xml - this fails but has no entities at all. if (hackAroundLibXMLEntityParsingBug() && context()->depth > depthTriggeringEntityExpansion() && uri.isNull() && prefix.isNull()) I should check if depthTriggeringEntityExpansion is actually != -1. I'll reland this with this obvious fix, after rerunning all layout tests on Mac.

Relanded in r113542.