66423 – libxml2 fragment parser loses prefix namespaces

RESOLVED FIXED Bug 66423

libxml2 fragment parser loses prefix namespaces

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

Summary libxml2 fragment parser loses prefix namespaces

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	Details Formatted Diff Diff
Patch (8.50 KB, patch) 2011-08-18 10:40 PDT, Vicki Pfau	no flags	Details Formatted Diff Diff
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	Details Formatted Diff Diff
Patch for landing (7.00 KB, patch) 2011-08-31 13:25 PDT, Vicki Pfau	no flags	Details Formatted Diff Diff
Patch for landing (7.02 KB, patch) 2011-09-14 22:11 PDT, Vicki Pfau	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component XML

Assignee

Vicki Pfau

Reported

2011-08-17 16:07 PDT

Modified

2011-09-14 22:25 PDT History

CC List

3 users Show

URL

Keywords

Depends on

Blocks