Bug 109495

Summary:

Fix svg/in-html/script-write.html with threaded HTML parser

Product:

WebKit

Reporter:

Tony Gentilcore <tonyg>

Component:

New Bugs

Assignee:

Tony Gentilcore <tonyg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric, ojan.autocc, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

106127

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Tony Gentilcore

Reported 2013-02-11 14:53:25 PST

Fix svg/in-html/script-write.html with threaded HTML parser

Attachments
Patch (1.57 KB, patch) 2013-02-11 14:57 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (3.60 KB, patch) 2013-02-11 18:18 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (3.50 KB, patch) 2013-02-11 18:27 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (13.37 KB, patch) 2013-02-13 11:49 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (14.25 KB, patch) 2013-02-13 12:34 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (18.15 KB, patch) 2013-02-13 15:34 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (18.15 KB, patch) 2013-02-13 15:54 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch for landing (14.71 KB, patch) 2013-02-13 16:48 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Tony Gentilcore

Comment 1 2013-02-11 14:57:39 PST

Created attachment 187688 [details] Patch

Adam Barth

Comment 2 2013-02-11 15:03:12 PST

Comment on attachment 187688 [details] Patch This is probably related to different ways of parsing scripts in foreign content. It's likely to be a bug with the threaded parser.

Tony Gentilcore

Comment 3 2013-02-11 18:18:35 PST

Created attachment 187742 [details] Patch

Tony Gentilcore

Comment 4 2013-02-11 18:19:23 PST

(In reply to comment #2) > (From update of attachment 187688 [details]) > This is probably related to different ways of parsing scripts in foreign content. It's likely to be a bug with the threaded parser. Good catch, there was a behavior difference here.

Tony Gentilcore

Comment 5 2013-02-11 18:27:28 PST

Created attachment 187745 [details] Patch

Eric Seidel (no email)

Comment 6 2013-02-11 18:38:36 PST

Comment on attachment 187745 [details] Patch This seems reasonable. All of those tags are HTML tags we would be handling. Are we sure the m_inForeignContent is correctly "false" for html in SVG? I'm not even sure what that's supposed to do... if say we were to encounter a <html><svg><foreignObject><plaintext>

Tony Gentilcore

Comment 7 2013-02-11 18:44:40 PST

> Are we sure the m_inForeignContent is correctly "false" for html in SVG? I'm not even sure what that's supposed to do... if say we were to encounter a <html><svg><foreignObject><plaintext> It seems worth adding a test case for that separately. But this patch just causes us to match the current behavior.

Eric Seidel (no email)

Comment 8 2013-02-11 19:14:56 PST

Comment on attachment 187745 [details] Patch I don't wish to derail your bug, but I think we should at least test this case with your patch: <!DOCTYPE html> <html> <body> <svg id="svg1" width="200" height="100" xmlns="http://www.w3.org/2000/svg"> <foreignObject x="0" y="0" width="200" height="100"> <div>foo</div> <plaintext> </foreignObject> </svg> <div>bar</div> </body> </html> And then either file a bug if we're changing behavior, or commit the test along with your change. The current behavior is kinda odd. :)

Eric Seidel (no email)

Comment 9 2013-02-11 19:15:11 PST

I should note that our current behavior matches FF. :)

Tony Gentilcore

Comment 10 2013-02-13 11:49:48 PST

Created attachment 188137 [details] Patch

Tony Gentilcore

Comment 11 2013-02-13 11:52:14 PST

Your test case was a good one. Matching that current behavior exposed a failure in some other svg tests. So we had to improve simulateTreeBuilder a little more. The ChangeLog explains. We pass all fast/parser and svg tests with this patch now.

Adam Barth

Comment 12 2013-02-13 11:53:11 PST

Comment on attachment 188137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188137&action=review > LayoutTests/fast/parser/foreignobject-in-foreigncontent.html:3 > + <body> Can we use dump-as-markup.js for this test?

Adam Barth

Comment 13 2013-02-13 11:53:51 PST

Comment on attachment 188137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188137&action=review > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:149 > + || threadSafeMatch(tagName, bTag) > + || threadSafeMatch(tagName, bigTag) > + || threadSafeMatch(tagName, blockquoteTag) > + || threadSafeMatch(tagName, bodyTag) > + || threadSafeMatch(tagName, brTag) > + || threadSafeMatch(tagName, centerTag) > + || threadSafeMatch(tagName, codeTag) > + || threadSafeMatch(tagName, ddTag) > + || threadSafeMatch(tagName, divTag) > + || threadSafeMatch(tagName, dlTag) > + || threadSafeMatch(tagName, dtTag) > + || threadSafeMatch(tagName, emTag) > + || threadSafeMatch(tagName, embedTag) > + || threadSafeMatch(tagName, h1Tag) > + || threadSafeMatch(tagName, h2Tag) > + || threadSafeMatch(tagName, h3Tag) > + || threadSafeMatch(tagName, h4Tag) > + || threadSafeMatch(tagName, h5Tag) > + || threadSafeMatch(tagName, h6Tag) > + || threadSafeMatch(tagName, headTag) > + || threadSafeMatch(tagName, hrTag) > + || threadSafeMatch(tagName, iTag) > + || threadSafeMatch(tagName, imgTag) > + || threadSafeMatch(tagName, liTag) > + || threadSafeMatch(tagName, listingTag) > + || threadSafeMatch(tagName, menuTag) Can we put all these into an inline helper function?

Eric Seidel (no email)

Comment 14 2013-02-13 11:55:58 PST

Comment on attachment 188137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188137&action=review > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:123 > + if (m_inForeignContent > + && (equalIgnoringCase(tagName, SVGNames::foreignObjectTag.localName()) So a <foreignObject> tag inside mathml? I think we need to know what namespace we're in to do this right. :(

Tony Gentilcore

Comment 15 2013-02-13 12:34:35 PST

Created attachment 188148 [details] Patch

Tony Gentilcore

Comment 16 2013-02-13 12:35:56 PST

> Can we use dump-as-markup.js for this test? Done > Can we put all these into an inline helper function? Done > So a <foreignObject> tag inside mathml? I think we need to know what namespace we're in to do this right. :( I split m_inForeignContent into m_inSVG and m_inMathML.

Adam Barth

Comment 17 2013-02-13 12:38:04 PST

Comment on attachment 188148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188148&action=review > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:173 > + m_inMathML = true; Can we be both m_inSVG and m_inMathML? If not, we might want to use an enum that has only the possible states.

Eric Seidel (no email)

Comment 18 2013-02-13 13:30:23 PST

Comment on attachment 188148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188148&action=review > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:177 > + if (inForeignContent() && tokenExitsForeignContent(token)) { > + m_inSVG = false; > + m_inMathML = false; > + } I'm concerned that this won't handle: <html><svg><foreignObject></foreignObject><title></svg>This text should appear correctly since we'll think we're in HTML as soon as we leave the fo?

Tony Gentilcore

Comment 19 2013-02-13 15:34:45 PST

Created attachment 188197 [details] Patch

WebKit Review Bot

Comment 20 2013-02-13 15:37:07 PST

Attachment 188197 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/parser/foreignobject-in-foreigncontent-expected.txt', u'LayoutTests/fast/parser/foreignobject-in-foreigncontent.html', u'LayoutTests/fast/parser/ignore-title-in-svg-after-foreignobject-expected.txt', u'LayoutTests/fast/parser/ignore-title-in-svg-after-foreignobject.html', u'LayoutTests/fast/parser/unmatched-close-foreignobject-expected.txt', u'LayoutTests/fast/parser/unmatched-close-foreignobject.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/parser/BackgroundHTMLParser.cpp', u'Source/WebCore/html/parser/BackgroundHTMLParser.h', u'Source/WebCore/html/parser/CompactHTMLToken.cpp', u'Source/WebCore/html/parser/CompactHTMLToken.h']" exit_code: 1 Source/WebCore/html/parser/BackgroundHTMLParser.h:74: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/html/parser/BackgroundHTMLParser.h:75: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Tony Gentilcore

Comment 21 2013-02-13 15:37:16 PST

> Can we be both m_inSVG and m_inMathML? If not, we might want to use an enum that has only the possible states. Done > > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:177 > > + if (inForeignContent() && tokenExitsForeignContent(token)) { > > + m_inSVG = false; > > + m_inMathML = false; > > + } > > I'm concerned that this won't handle: > > <html><svg><foreignObject></foreignObject><title></svg>This text should appear correctly since we'll think we're in HTML as soon as we leave the fo? You were right all along: this requires a stack. I added that and your test case. Our behavior matches the current parser on all the new tests. Please let me know if you can think of other interesting edge cases we should test.

Tony Gentilcore

Comment 22 2013-02-13 15:37:57 PST

(In reply to comment #20) > Attachment 188197 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/parser/foreignobject-in-foreigncontent-expected.txt', u'LayoutTests/fast/parser/foreignobject-in-foreigncontent.html', u'LayoutTests/fast/parser/ignore-title-in-svg-after-foreignobject-expected.txt', u'LayoutTests/fast/parser/ignore-title-in-svg-after-foreignobject.html', u'LayoutTests/fast/parser/unmatched-close-foreignobject-expected.txt', u'LayoutTests/fast/parser/unmatched-close-foreignobject.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/parser/BackgroundHTMLParser.cpp', u'Source/WebCore/html/parser/BackgroundHTMLParser.h', u'Source/WebCore/html/parser/CompactHTMLToken.cpp', u'Source/WebCore/html/parser/CompactHTMLToken.h']" exit_code: 1 > Source/WebCore/html/parser/BackgroundHTMLParser.h:74: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] > Source/WebCore/html/parser/BackgroundHTMLParser.h:75: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] > Total errors found: 2 in 12 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. I think it is better to have HTML and SVG enum values than Html and Svg. Please let me know if otherwise.

Eric Seidel (no email)

Comment 23 2013-02-13 15:44:40 PST

Comment on attachment 188197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188197&action=review > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:62 > + // FIXME: This is copied from HTMLTreeBuilder::processTokenInForeignContent and changed to use threadSafeMatch. > + const String& tagName = token.data(); > + return threadSafeMatch(tagName, bTag) Our technical debt for not solving the AtomicString problem is mounting... :p > Source/WebCore/html/parser/BackgroundHTMLParser.h:88 > + Vector<Namespace> m_namespaceStack; Do we want any initial capacity? Do we worry about keeping this up to date with document.writes which are processed on the main thread?

Tony Gentilcore

Comment 24 2013-02-13 15:54:00 PST

Created attachment 188206 [details] Patch

Tony Gentilcore

Comment 25 2013-02-13 15:55:40 PST

> Do we want any initial capacity? I think it is safe to say SVG and MathML are rare enough that we should use 1 as the initial capacity (to account for the HTML namespace added in the ctor). > Do we worry about keeping this up to date with document.writes which are processed on the main thread? That's a tough one. Do you have any ideas how we can handle that?

WebKit Review Bot

Comment 26 2013-02-13 15:58:08 PST

Attachment 188206 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/parser/foreignobject-in-foreigncontent-expected.txt', u'LayoutTests/fast/parser/foreignobject-in-foreigncontent.html', u'LayoutTests/fast/parser/ignore-title-in-svg-after-foreignobject-expected.txt', u'LayoutTests/fast/parser/ignore-title-in-svg-after-foreignobject.html', u'LayoutTests/fast/parser/unmatched-close-foreignobject-expected.txt', u'LayoutTests/fast/parser/unmatched-close-foreignobject.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/parser/BackgroundHTMLParser.cpp', u'Source/WebCore/html/parser/BackgroundHTMLParser.h', u'Source/WebCore/html/parser/CompactHTMLToken.cpp', u'Source/WebCore/html/parser/CompactHTMLToken.h']" exit_code: 1 Source/WebCore/html/parser/BackgroundHTMLParser.h:74: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/html/parser/BackgroundHTMLParser.h:75: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Adam Barth

Comment 27 2013-02-13 16:07:27 PST

> That's a tough one. Do you have any ideas how we can handle that? Yeah, we'll need to read the information from the stack of open elements and send it to the background HTML parser in the Checkpoint when we call "resumeFrom". Don't worry about it for this patch, but please file a bug so that we remember to write tests and fix it. :)

Tony Gentilcore

Comment 28 2013-02-13 16:14:08 PST

(In reply to comment #27) > > That's a tough one. Do you have any ideas how we can handle that? > > Yeah, we'll need to read the information from the stack of open elements and send it to the background HTML parser in the Checkpoint when we call "resumeFrom". Don't worry about it for this patch, but please file a bug so that we remember to write tests and fix it. :) Makes sense. Filed bug 109764.

Eric Seidel (no email)

Comment 29 2013-02-13 16:18:32 PST

Comment on attachment 188206 [details] Patch I think this is another big step forward, and we should land and iterate. I suspect that most of the tests you added could have been written as html5lib tests instead. If you want to explore that before landing, be my guest. :)

Eric Seidel (no email)

Comment 30 2013-02-13 16:19:49 PST

Comment on attachment 188206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188206&action=review > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:62 > + return threadSafeMatch(tagName, bTag) Actually. This is gonna be epiclly slow. I'm not sure we can do this? Right? This is going to run on every start/end token and do slow string compares? I guess it will just compare the hashes...

Tony Gentilcore

Comment 31 2013-02-13 16:24:40 PST

> I think this is another big step forward, and we should land and iterate. I suspect that most of the tests you added could have been written as html5lib tests instead. If you want to explore that before landing, be my guest. :) I'll do that before landing. > > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:62 > > + return threadSafeMatch(tagName, bTag) > > Actually. This is gonna be epiclly slow. I'm not sure we can do this? > > Right? This is going to run on every start/end token and do slow string compares? I guess it will just compare the hashes... The only reason I'm okay with it is that we only do it when we're in foreign content mode. If not in foreign content mode, we shortcut.

Tony Gentilcore

Comment 32 2013-02-13 16:48:46 PST

Created attachment 188224 [details] Patch for landing

Adam Barth

Comment 33 2013-02-13 16:57:43 PST

(In reply to comment #30) > (From update of attachment 188206 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188206&action=review > > > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:62 > > + return threadSafeMatch(tagName, bTag) > > Actually. This is gonna be epiclly slow. I'm not sure we can do this? > > Right? This is going to run on every start/end token and do slow string compares? I guess it will just compare the hashes... Yeah, it just compares the hashes. It might be worth trying the patch in bug 107337 again to see if it's a win now that we're doing more comparisons. This only runs in foreign content mode, so we might need to make a new svg-in-html parsing benchmark to see the effect.

WebKit Review Bot

Comment 34 2013-02-13 17:27:53 PST

Comment on attachment 188224 [details] Patch for landing Clearing flags on attachment: 188224 Committed r142829: <http://trac.webkit.org/changeset/142829>

WebKit Review Bot

Comment 35 2013-02-13 17:27:59 PST

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.