| 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: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 106127 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Tony Gentilcore
2013-02-11 14:53:25 PST
Created attachment 187688 [details]
Patch
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.
Created attachment 187742 [details]
Patch
(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. Created attachment 187745 [details]
Patch
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>
> 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.
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. :) I should note that our current behavior matches FF. :) Created attachment 188137 [details]
Patch
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. 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? 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? 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. :( Created attachment 188148 [details]
Patch
> 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. 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. 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? Created attachment 188197 [details]
Patch
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.
> 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. (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. 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? Created attachment 188206 [details]
Patch
> 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? 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.
> 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. :)
(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. 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. :)
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... > 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. Created attachment 188224 [details]
Patch for landing
(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. Comment on attachment 188224 [details] Patch for landing Clearing flags on attachment: 188224 Committed r142829: <http://trac.webkit.org/changeset/142829> All reviewed patches have been landed. Closing bug. |