| Summary: | libxml2 fragment parser loses prefix namespaces | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vicki Pfau <jeffrey+webkit> | ||||||||||||||||
| Component: | XML | Assignee: | Vicki Pfau <jeffrey+webkit> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | darin, pkasting, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 104280 [details]
Patch
Comment on attachment 104280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104280&action=review > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:614 > for (Element* element = elemStack.last(); !elemStack.isEmpty(); elemStack.removeLast()) { > + element = elemStack.last(); The definition should be removed entirely from the for statement. There’s no point to setting up element twice or having it declared inside the for statement at all. Created attachment 104362 [details]
Patch
Comment on attachment 104362 [details] Patch Clearing flags on attachment: 104362 Committed r93452: <http://trac.webkit.org/changeset/93452> All reviewed patches have been landed. Closing bug. This test seems to have been failing everywhere since it was introduced. On the Chromium bots, which produce pixel results, the result looks like "ab" instead of "a^b" (^ for superscript, of course). On the Leopard and Snowleopard bots, which just produce text results, the text results differ from the expectations. Because Darin isn't around on IRC and I don't know Jeffrey's handle, I'm rolling this out. (Sorry about the delay since the time this was landed; I didn't notice this issue yesterday.) Reverted r93452 for reason: Broke Leopard, Snowleopard, and Chromium bots Committed r93638: <http://trac.webkit.org/changeset/93638> I see, the new test was failing. This bug appears to be in the Chromium bug tracker as http://code.google.com/p/chromium/issues/detail?id=88865 Created attachment 105815 [details]
SVG repro
The old test was failing in patched Chromium because Chromium doesn't currently support MathML. Here is a different test case that uses SVG instead.
Created attachment 105818 [details]
Patch
Comment on attachment 105818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105818&action=review The patch looks great. I would perfer you landed an updated test if you have a minute. > LayoutTests/ChangeLog:1 > +2011-08-31 Jeffrey Pfau <jeffrey@endrift.com> apple.com? > LayoutTests/fast/parser/innerhtml-with-prefixed-elements.xhtml:10 > +<html xmlns="http://www.w3.org/1999/xhtml"> > + <head> > + <title>Namespace chaining</title> > + </head> > + <body> > + <div id="d" xmlns:svg="http://www.w3.org/2000/svg"> > + TEST FAILED > + </div> > + <script type="text/javascript"><![CDATA[ > + var div = document.getElementById("d"); I think normally we use 4 space indent. > LayoutTests/fast/parser/innerhtml-with-prefixed-elements.xhtml:11 > + div.innerHTML = "<svg:svg width='20' height='20'><svg:defs id='defs'><svg:text>TEST FAILED</svg:text></svg:defs><svg:circle cx='10' cy='10' r='4' id='c'/></svg:svg>"; Would be much better to use a 100x100 green square to show success (it's an old convention from the w3c CSS working group test suite). > Source/WebCore/ChangeLog:1 > +2011-08-31 Jeffrey Pfau <jeffrey@endrift.com> apple.com? > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:614 > - for (Element* element = elemStack.last(); !elemStack.isEmpty(); elemStack.removeLast()) { > + for (; !elemStack.isEmpty(); elemStack.removeLast()) { > + Element* element = elemStack.last(); Wow. I wonder how many other bad for loops like this we have. Some day c++ may have a foreach. :) Created attachment 105820 [details]
Patch for landing
Comment on attachment 105820 [details] Patch for landing Rejecting attachment 105820 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: vg-fonts-word-spacing.html = IMAGE+TEXT Regressions: Unexpected image mismatch : (5) fast/text/atsui-multiple-renderers.html = IMAGE fast/text/international/danda-space.html = IMAGE fast/text/international/thai-baht-space.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE Regressions: Unexpected text diff mismatch : (1) fast/parser/innerhtml-with-prefixed-elements.xhtml = TEXT Full output: http://queues.webkit.org/results/9564976 Created attachment 107457 [details]
Patch for landing
Comment on attachment 107457 [details] Patch for landing Clearing flags on attachment: 107457 Committed r95169: <http://trac.webkit.org/changeset/95169> All reviewed patches have been landed. Closing bug. |
Created attachment 104263 [details] MathML repro When doing fragment parsing (e.g. with innerHTML) with libxml2, the namespaces on context elements above the innermost are not handled.