Threaded HTML Parser has an extra copy of every byte from the network
Created attachment 190868 [details] Patch
Created attachment 190871 [details] Patch
Can't change setDocument to use insert() as the XML parser doesn't implement insert. I added a FIXME instead.
Oh, bad news batman. crash log for DumpRenderTree (pid 9217): STDOUT: <empty> STDERR: ASSERTION FAILED: inputSource->hasOneRef() STDERR: ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp(659) : virtual void WebCore::HTMLDocumentParser::append(PassRefPtr<WTF::StringImpl>) STDERR: 1 0x19a0701 WebCore::HTMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) STDERR: 2 0x33d8a3f WebCore::Document::setContent(WTF::String const&) STDERR: 3 0x3322140 WebCore::XSLTProcessor::createDocumentFromSource(WTF::String const&, WTF::String const&, WTF::String const&, WebCore::Node*, WebCore::Frame*) STDERR: 4 0x33e8da6 WebCore::Document::applyXSLTransform(WebCore::ProcessingInstruction*) STDERR: 5 0x348bcc5 WebCore::DocumentStyleSheetCollection::collectActiveStyleSheets(WTF::Vector<WTF::RefPtr<WebCore::StyleSheet>, 0ul>&) STDERR: 6 0x348c7a2 WebCore::DocumentStyleSheetCollection::updateActiveStyleSheets(WebCore::DocumentStyleSheetCollection::UpdateFlag) STDERR: 7 0x33d8090 WebCore::Document::styleResolverChanged(WebCore::StyleResolverUpdateFlag) STDERR: 8 0x33e1155 WebCore::Document::didRemoveAllPendingStylesheet() STDERR: 9 0x348b8ae WebCore::DocumentStyleSheetCollection::removePendingSheet(WebCore::DocumentStyleSheetCollection::RemovePendingSheetNotificationType) STDERR: 10 0x3586712 WebCore::ProcessingInstruction::sheetLoaded() STDERR: 11 0x331eb32 WebCore::XSLStyleSheet::checkLoaded() STDERR: 12 0x3586ca9 WebCore::ProcessingInstruction::parseStyleSheet(WTF::String const&) STDERR: 13 0x3586e5c WebCore::ProcessingInstruction::setXSLStyleSheet(WTF::String const&, WebCore::KURL const&, WTF::String const&) STDERR: 14 0x3586edb non-virtual thunk to WebCore::ProcessingInstruction::setXSLStyleSheet(WTF::String const&, WebCore::KURL const&, WTF::String const&) STDERR: 15 0x30b11c4 WebCore::CachedXSLStyleSheet::checkNotify() STDERR: 16 0x30b10c3 WebCore::CachedXSLStyleSheet::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool) STDERR: 17 0x303956a WebCore::SubresourceLoader::didFinishLoading(double) STDERR: 18 0x30321af WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) STDERR: 19 0x241726a WebCore::ResourceHandleInternal::didFinishLoading(WebKit::WebURLLoader*, double) This means we have bugs in our XSLTProcessor... that append() call could yield, meaning the end() call directly following it would come before all of the data is processed. I guess that just means we just drop the rest of the document on the floor. :( I guess this means I need to implement XMLDocumentParser::insert() and use that instead, but somehow guard against it ever being used from document.write() too. I don't really want to create a new method for this.
Comment on attachment 190871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190871&action=review > Source/WTF/ChangeLog:14 > + The threaded html parser needs to accept ownership > + of a string buffer. The easiest way to do this seemed > + to be to use a PassRefPtr<StringImpl>, but there was no way > + to generated one from a String (easily), so I added one. > + > + * wtf/text/WTFString.h: > + (WTF::String::releaseImpl): Thus spreading the bad stuffs from RefPtr :(
(In reply to comment #5) > (From update of attachment 190871 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190871&action=review > > > Source/WTF/ChangeLog:14 > > + The threaded html parser needs to accept ownership > > + of a string buffer. The easiest way to do this seemed > > + to be to use a PassRefPtr<StringImpl>, but there was no way > > + to generated one from a String (easily), so I added one. > > + > > + * wtf/text/WTFString.h: > > + (WTF::String::releaseImpl): > > Thus spreading the bad stuffs from RefPtr :( I'm open to other suggestions. I originally considered using (String&) and String::swap() to steal out the ref, but that seemed more hacky. Alternatively I coud have a PassOwnPtr wrapper around an object which contained a String. Thats effectively what bind() is doing for us before passing it to the background thread. Or, I could change this API to take a Vector<UChar>, but that seems nasty. I'm assuming of course that the "bad stuffs" from RefPtr you mean is the ref-counted nature of strings which makes this x-thread communication extra tricky to get right.
Why is XSLT using an HTML parser? Document::setContent should probably pin the parser to the main thread.
> Thus spreading the bad stuffs from RefPtr :( What are the bad stuffs?
> I'm open to other suggestions. I originally considered using (String&) and String::swap() to steal out the ref, but that seemed more hacky. I don't know anything about the threading involved here so I don't have any solution to suggest. :( I am just sad the message passing is not cleaner. > Alternatively I coud have a PassOwnPtr wrapper around an object which contained a String. Thats effectively what bind() is doing for us before passing it to the background thread. > > Or, I could change this API to take a Vector<UChar>, but that seems nasty. > > I'm assuming of course that the "bad stuffs" from RefPtr you mean is the ref-counted nature of strings which makes this x-thread communication extra tricky to get right. I think a PassOwnPtr on a wrapper make sense regarding the ownership. As long as you don't leak the string impl, you avoid threading issues.
(In reply to comment #7) > Why is XSLT using an HTML parser? > > Document::setContent should probably pin the parser to the main thread. Yeah, that's fine. It just masks the bug here, but that's OK I guess. I believe the bug just involves dropping the rest of the document on the floor. If you use XSLT to produce a large html document (or one with <script> tags) I suspect we drop everything after the <script> on the floor. XSLT uses the HTML parser (or whatever parser) to parse the result of the XSLT transform.
(In reply to comment #9) > > I'm open to other suggestions. I originally considered using (String&) and String::swap() to steal out the ref, but that seemed more hacky. > > I don't know anything about the threading involved here so I don't have any solution to suggest. :( > > I am just sad the message passing is not cleaner. The model is that DocumentParser::append() wants to take ownership of the string so it can pass it to the parser thread. It only really cares in the threaded-parser case, but because it's a generic interface on DocumentParser, requires all callers to append() to be aware that append() expects to take ownership. > > Alternatively I coud have a PassOwnPtr wrapper around an object which contained a String. Thats effectively what bind() is doing for us before passing it to the background thread. > > > > Or, I could change this API to take a Vector<UChar>, but that seems nasty. > > > > I'm assuming of course that the "bad stuffs" from RefPtr you mean is the ref-counted nature of strings which makes this x-thread communication extra tricky to get right. > > I think a PassOwnPtr on a wrapper make sense regarding the ownership. As long as you don't leak the string impl, you avoid threading issues. struct ParseRequest { String source; }; DocumentParser::append(PassOwnPtr<ParseRequest>) would be one way to do this, yes.
Or even I guess PassOwnPtr<String>. :) Since they're equivalent. PassOwnPtr<StringImpl> seems like a bad idea, but all of these options seem bad. :)
I think I'll try changing it to PassOwnPtr<String> and see how ugly that looks. :)
> Yeah, that's fine. It just masks the bug here, but that's OK I guess. I believe the bug just involves dropping the rest of the document on the floor. If you use XSLT to produce a large html document (or one with <script> tags) I suspect we drop everything after the <script> on the floor. Why would we drop the rest of the document on the floor? This seems just like what happens when the network delivers data in one big packet.
(In reply to comment #14) > > Yeah, that's fine. It just masks the bug here, but that's OK I guess. I believe the bug just involves dropping the rest of the document on the floor. If you use XSLT to produce a large html document (or one with <script> tags) I suspect we drop everything after the <script> on the floor. > > Why would we drop the rest of the document on the floor? This seems just like what happens when the network delivers data in one big packet. You're right. I for whatever reason mis-remembered Document::setContent as calling DocumentParser::end() directly, it calls Document::close() which does a finish and does a forced-syncrhonous parse which will get the rest of the doc. So it doesn't matter that append yields in this case. I can just pin the parser and this ASSERT will go away.
> struct ParseRequest { > String source; > }; This is not really what I meant. (In reply to comment #13) > I think I'll try changing it to PassOwnPtr<String> and see how ugly that looks. :) I don't think that is any better. What I mean is there is no ownership passing semantic on RefPtr. I may simply be too obsessive about clean threading semantic. Or maybe I am completely missing the point. Let's say a decoder keep a reference to that String. Wouldn't this break the code? If it does, I think the object returned should instead be one with pure ownership (even if it is just a lame wrapper).
(In reply to comment #16) > > struct ParseRequest { > > String source; > > }; > > This is not really what I meant. > > (In reply to comment #13) > > I think I'll try changing it to PassOwnPtr<String> and see how ugly that looks. :) > > I don't think that is any better. OK. Then I'll keep it with PassRefPtr<StringImpl> for now. > What I mean is there is no ownership passing semantic on RefPtr. > I may simply be too obsessive about clean threading semantic. There isn't really a threading semantic here. Clients of DocumentParser don't care that it's going to pass the string across the thread. But HTMLDocumentParser does care (when in threading mode) that the string passed in is completely owned by it. HTMLDocumentParser doesn't want to deal with ref-counted Strings, but that's what we have in WebCore. :) PassRefPtr<StringImpl> doesn't really have the meaning that I want, but it seemed closer than String& at least. :) > Or maybe I am completely missing the point. > Let's say a decoder keep a reference to that String. Wouldn't this break the code? If it does, I think the object returned should instead be one with pure ownership (even if it is just a lame wrapper). Yes. If something kept a reference, that would break things. That's why it ASSERTs. I mean, it could even disable threading mode in that case as a fall-back, but ideally this would take a PassOwnPtr<Vector<UChar>> to fully convey the "I'm going to take ownership of this buffer, dammit" meaning.
Created attachment 190986 [details] Patch
Comment on attachment 190986 [details] Patch I understand benjaminp's concerns, but I think we should still move forward with this patch. It's true that we're creating a tighter coupling between the HTML parser and the charset decoder, but I don't see a way around that with our current string mechanism. (The ASSERT helps us maintain the invariant, so we should be well-covered w.r.t. testing.) In the long term, we should think about how to make it easier to write multithreaded code that uses strings in WebCore. The current String class is really difficult to use on multiple threads correctly.
Comment on attachment 190986 [details] Patch Clearing flags on attachment: 190986 Committed r144498: <http://trac.webkit.org/changeset/144498>
All reviewed patches have been landed. Closing bug.
This patch breaks our sh4 Qt/WebKit1 build slave: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/21351
/local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp: In member function 'void WebCore::XMLDocumentParser::resumeParsing()': /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:265:16: error: no matching function for call to 'WebCore::XMLDocumentParser::append(WebCore::SegmentedString&)' /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:265:16: note: candidate is: /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'WebCore::SegmentedString' to 'WTF::PassRefPtr<WTF::StringImpl>' /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp: In member function 'bool WebCore::XMLDocumentParser::appendFragmentSource(const WTF::String&)': /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:276:46: error: no matching function for call to 'WebCore::XMLDocumentParser::append(WTF::String)' /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:276:46: note: candidate is: /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'WTF::String' to 'WTF::PassRefPtr<WTF::StringImpl>' /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:277:18: error: no matching function for call to 'WebCore::XMLDocumentParser::append(const WTF::String&)' /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:277:18: note: candidate is: /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'const WTF::String' to 'WTF::PassRefPtr<WTF::StringImpl>' /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:278:47: error: no matching function for call to 'WebCore::XMLDocumentParser::append(WTF::String)' /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:278:47: note: candidate is: /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'WTF::String' to 'WTF::PassRefPtr<WTF::StringImpl>'
(In reply to comment #23) > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp: In member function 'void WebCore::XMLDocumentParser::resumeParsing()': > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:265:16: error: no matching function for call to 'WebCore::XMLDocumentParser::append(WebCore::SegmentedString&)' > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:265:16: note: candidate is: > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'WebCore::SegmentedString' to 'WTF::PassRefPtr<WTF::StringImpl>' For this error, I think we can do the same as in XMLDocumentParserLibxml2.cpp: - append(rest); + append(rest.toString().impl()); > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp: In member function 'bool WebCore::XMLDocumentParser::appendFragmentSource(const WTF::String&)': > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:276:46: error: no matching function for call to 'WebCore::XMLDocumentParser::append(WTF::String)' > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:276:46: note: candidate is: > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'WTF::String' to 'WTF::PassRefPtr<WTF::StringImpl>' > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:277:18: error: no matching function for call to 'WebCore::XMLDocumentParser::append(const WTF::String&)' > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:277:18: note: candidate is: > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'const WTF::String' to 'WTF::PassRefPtr<WTF::StringImpl>' > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:278:47: error: no matching function for call to 'WebCore::XMLDocumentParser::append(WTF::String)' > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:278:47: note: candidate is: > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: virtual void WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) > /local/wkit/slavebuildbot_LOCAL/workspace_LOCAL/qt-linux-sh4-release/build/Source/WebCore/xml/parser/XMLDocumentParser.h:110:22: note: no known conversion for argument 1 from 'WTF::String' to 'WTF::PassRefPtr<WTF::StringImpl>' For these 3 errors, I think this should builds, but I'm not sure that it's actually a good thing: - append(String("<qxmlstreamdummyelement>")); - append(source); - append(String("</qxmlstreamdummyelement>")); + append(String("<qxmlstreamdummyelement>").impl()); + append(source.impl()); + append(String("</qxmlstreamdummyelement>").impl());
Posting a patch now.
Why was is the Qt bubble above green if this patch breaks the qt build?
Attempted build fix in https://bugs.webkit.org/show_bug.cgi?id=111272
(In reply to comment #26) > Why was is the Qt bubble above green if this patch breaks the qt build? Because Qt has only one EWS which uses USE(libxml) codepath, but unfortunataly Qt SH4 and Qt Windows don't have EWS bots and they don't use libxml.
I would like to see the qtxml code path removed at some point. :). I know I approved the patches long ago, but I now think that was a mistake. The XML parser is tricky to get right once, and I think having two parser implementations is mostly a source of compile errors and security bugs these days. :) Thank you both for the build fix!
(In reply to comment #29) > I would like to see the qtxml code path removed at some point. :). I know I approved the patches long ago, but I now think that was a mistake. The XML parser is tricky to get right once, and I think having two parser implementations is mostly a source of compile errors and security bugs these days. :) > > Thank you both for the build fix! cc Qt maintainers
*** Bug 107332 has been marked as a duplicate of this bug. ***
(In reply to comment #30) > (In reply to comment #29) > > I would like to see the qtxml code path removed at some point. :). I know I approved the patches long ago, but I now think that was a mistake. The XML parser is tricky to get right once, and I think having two parser implementations is mostly a source of compile errors and security bugs these days. :) > > > > Thank you both for the build fix! > > cc Qt maintainers Thanks Ossy for the Cc. I think Eric has a fair point and I'm inclined to agree. Do you envision that the project would require libxml at all times or do you think that it makes sense to also support a build without libxml (and thus XHTML) support?
(In reply to comment #32) > (In reply to comment #30) > > (In reply to comment #29) > > > I would like to see the qtxml code path removed at some point. :). I know I approved the patches long ago, but I now think that was a mistake. The XML parser is tricky to get right once, and I think having two parser implementations is mostly a source of compile errors and security bugs these days. :) > > > > > > Thank you both for the build fix! > > > > cc Qt maintainers > > Thanks Ossy for the Cc. > > I think Eric has a fair point and I'm inclined to agree. > > Do you envision that the project would require libxml at all times or do you think that it makes sense to also support a build without libxml (and thus XHTML) support? libxml seems very prevalent to me. Either we should use something better for all of WebKit, or we should fix libxml to be more awesome. It's probably more common to see xml content served to XMLHttpRequest than it is to run across a truly XHTML page. I don't believe it is possible to use the web w/o some XML support, sadly.
(In reply to comment #33) [...] > > I think Eric has a fair point and I'm inclined to agree. > > > > Do you envision that the project would require libxml at all times or do you think that it makes sense to also support a build without libxml (and thus XHTML) support? > > libxml seems very prevalent to me. Either we should use something better for all of WebKit, or we should fix libxml to be more awesome. > > It's probably more common to see xml content served to XMLHttpRequest than it is to run across a truly XHTML page. I don't believe it is possible to use the web w/o some XML support, sadly. Oh, excellent point. Yeah that makes a build without libxml useless. I'll bring it up in the Qt community.