Bug 66423

Summary: libxml2 fragment parser loses prefix namespaces
Product: WebKit Reporter: Vicki Pfau <jeffrey+webkit>
Component: XMLAssignee: 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:
Description Flags
MathML repro
none
Patch
none
Patch
none
SVG repro
none
Patch
none
Patch for landing
none
Patch for landing none

Vicki Pfau
Reported 2011-08-17 16:07:00 PDT
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.
Attachments
MathML repro (430 bytes, application/xhtml+xml)
2011-08-17 16:07 PDT, Vicki Pfau 		no flags
Details
Patch (8.41 KB, patch)
2011-08-17 16:39 PDT, Vicki Pfau 		no flags
DetailsFormatted DiffDiff
Patch (8.50 KB, patch)
2011-08-18 10:40 PDT, Vicki Pfau 		no flags
DetailsFormatted DiffDiff
SVG repro (722 bytes, application/xhtml+xml)
2011-08-31 12:50 PDT, Vicki Pfau 		no flags
Details
Patch (7.01 KB, patch)
2011-08-31 13:06 PDT, Vicki Pfau 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.00 KB, patch)
2011-08-31 13:25 PDT, Vicki Pfau 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.02 KB, patch)
2011-09-14 22:11 PDT, Vicki Pfau 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Vicki Pfau
Comment 1 2011-08-17 16:39:45 PDT
Created attachment 104280 [details] Patch
Darin Adler
Comment 2 2011-08-17 16:49:02 PDT
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.
Vicki Pfau
Comment 3 2011-08-18 10:40:11 PDT
Created attachment 104362 [details] Patch
WebKit Review Bot
Comment 4 2011-08-19 16:37:19 PDT
Comment on attachment 104362 [details] Patch Clearing flags on attachment: 104362 Committed r93452: <http://trac.webkit.org/changeset/93452>
WebKit Review Bot
Comment 5 2011-08-19 16:37:23 PDT
All reviewed patches have been landed. Closing bug.
Peter Kasting
Comment 6 2011-08-23 14:23:54 PDT
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.)
Peter Kasting
Comment 7 2011-08-23 15:08:27 PDT
Reverted r93452 for reason: Broke Leopard, Snowleopard, and Chromium bots Committed r93638: <http://trac.webkit.org/changeset/93638>
Darin Adler
Comment 8 2011-08-23 15:31:06 PDT
I see, the new test was failing.
Vicki Pfau
Comment 9 2011-08-30 08:35:45 PDT
This bug appears to be in the Chromium bug tracker as http://code.google.com/p/chromium/issues/detail?id=88865
Vicki Pfau
Comment 10 2011-08-31 12:50:50 PDT
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.
Vicki Pfau
Comment 11 2011-08-31 13:06:40 PDT
Created attachment 105818 [details] Patch
Eric Seidel (no email)
Comment 12 2011-08-31 13:11:09 PDT
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. :)
Vicki Pfau
Comment 13 2011-08-31 13:25:36 PDT
Created attachment 105820 [details] Patch for landing
WebKit Review Bot
Comment 14 2011-08-31 16:13:02 PDT
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
Vicki Pfau
Comment 15 2011-09-14 22:11:19 PDT
Created attachment 107457 [details] Patch for landing
WebKit Review Bot
Comment 16 2011-09-14 22:25:51 PDT
Comment on attachment 107457 [details] Patch for landing Clearing flags on attachment: 107457 Committed r95169: <http://trac.webkit.org/changeset/95169>
WebKit Review Bot
Comment 17 2011-09-14 22:25:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.