Summary: | Fragment parsing needs to go through the HTML5 Parser code path | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||
Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric, gustavo, ossy, tonyg, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Other | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Bug Depends on: | 40657 | ||||||||||||||||
Bug Blocks: | 46692, 39259 | ||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2010-06-15 16:49:05 PDT
Created attachment 58837 [details]
work in progress
The current patch crashes in a few tests at: bool peek(SegmentedString& source, int& lineNumber) { m_nextInputCharacter = *source; under the fragment parsing path. I do not yet know why. This change could probably be simplified some. The fragment parsing path is a mess anyway. Not really sure how to clean it up. Part of me feels like these parse*DocumentFragment methods belong in their own header or on DocumentFragment iself, not sure. For the HTML5 codepath I put the call on HTML5DocumentParser so I could make the DocumentFragment* version of the constructor private. editing/pasteboard/copy-crash.html stderr fast/forms/textarea-textlength.html stderr platform/mac/fast/text/ligature-subdivision.html expected actual diff pretty diff stderr Are the tests which crash. I suspect we might end up calling end() on the DocumentParser, and thus trying to delete a stack pointer. Although I would expect the delete to fail, not random later crashes. My end() theory is bogus. The Document doesn't own this DocumentParser, so it never deletes it. :) I've documented the failures here: https://spreadsheets.google.com/ccc?key=0AppchfQ5mBrEdDFJUW5DOGNsdmtvZkN0ZmIzMjdaT0E&hl=en#gid=6 I may find a way to land this code "off" so that we can attack those failures in a distributed fashion like we did for the main parser conversion. Created attachment 59314 [details]
Patch
Attachment 59314 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/dom/Element.cpp:45: Alphabetical sorting problem. [build/include_order] [4]
WebCore/html/HTML5TreeBuilder.cpp:34: Alphabetical sorting problem. [build/include_order] [4]
WebCore/xml/XSLTProcessor.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
WebCore/html/HTMLElement.cpp:42: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 4 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 59317 [details]
Patch
Comment on attachment 59314 [details]
Patch
WebCore/html/HTML5DocumentParser.cpp:372
+ // This call is virtual.
call -> function
WebCore/html/HTML5DocumentParser.cpp:378
+ // This call is used throughout the implementation.
call -> function
WebCore/html/HTML5DocumentParser.cpp:494
+ && document->page()->settings()->html5ParserEnabled();
We should reverse the default when we can't find the Settings object at some point. This occurs during boot.
WebCore/html/HTML5DocumentParser.cpp:503
+ parser.write(source, false);
false here because we're not from the network. I guess that interacts properly with the preload scanner.
WebCore/html/HTML5DocumentParser.h:56
+ static void parseHTMLDocumentFragment(const String&, DocumentFragment*, FragmentScriptingPermission = FragmentScriptingAllowed);
I feel like this should be a method of the DocumentFragment object. (Followup patch?)
WebCore/html/HTML5TreeBuilder.cpp:207
+ m_lastScriptElement->removeChildren();
Crazy
WebCore/html/HTML5TreeBuilder.h:30
+ #include "MappedAttributeEntry.h" // For FragmentScriptingPermission!?
We should move FragmentScriptingPermission
Btw, the horrible FragmentScriptingPermission hack exists just for: http://trac.webkit.org/browser/trunk/LayoutTests/editing/pasteboard/paste-visible-script.html Which came from: http://trac.webkit.org/changeset/53659 https://bugs.webkit.org/show_bug.cgi?id=33970 That patch makes zero sense. It doesn't seem to block <img src="about:blank" onerror="alert(1)">. It also randomly nukes <dog happyhref="javascript:potato"></dog> In any case, that doesn't block us here, but we should either make that check bulletproof or get rid of it entirely. A half-working hack helps no one. Created attachment 59333 [details]
Updated per Adam's review comments
(In reply to comment #12) > That patch makes zero sense. It doesn't seem to block <img src="about:blank" onerror="alert(1)">. I could be wrong about this one. :) Comment on attachment 59333 [details]
Updated per Adam's review comments
Politics aside, this is a good patch. Thanks. :)
LayoutTests/fast/tokenizer/lessthan-terminates-tags-and-attrs-expected.txt:5
+ FAIL: <foo<bar> parsed as <foo<bar></foo<bar> instead of <foo><bar></bar></foo>
Can you double-check this in minefield?
WebCore/html/HTML5TreeBuilder.h:113
+ // FIXME: FragmentScriptingPermission is a HACK for platform/Pasteboard.
This seems a bit over the top, but ok.
(In reply to comment #17) > LayoutTests/fast/tokenizer/lessthan-terminates-tags-and-attrs-expected.txt:5 > + FAIL: <foo<bar> parsed as <foo<bar></foo<bar> instead of <foo><bar></bar></foo> > Can you double-check this in minefield? Yup. Minefield agrees. Attachment 59333 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3306541 Created attachment 59334 [details]
Hopefully fix the EWS bots
Attachment 59334 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3286565 OK, so what am I missing? I edited the .pro file to include the header... Attachment 59333 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3330559 Created attachment 59337 [details]
helps if you git add the new file
Attachment 59333 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3287557 Attachment 59334 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3338594 Comment on attachment 59337 [details]
helps if you git add the new file
I trust you got all the compile demons out of the patch.
Committed r61637: <http://trac.webkit.org/changeset/61637> http://trac.webkit.org/changeset/61637 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/61636 http://trac.webkit.org/changeset/61637 This checkin exposed a bug in the Qt clipboard code: https://bugs.webkit.org/show_bug.cgi?id=41025 |