Bug 40645 – Fragment parsing needs to go through the HTML5 Parser code path

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Fragment parsing needs to go through the HTML5 Parser code path

[Eric Seidel (no email)]

Created attachment 58837 [details]
work in progress

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

My end() theory is bogus.  The Document doesn't own this DocumentParser, so it never deletes it. :)

[Eric Seidel (no email)]

The crash is caused by bug 40657.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 59314 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Created attachment 59317 [details]
Patch

[Adam Barth]

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

[Eric Seidel (no email)]

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

[Adam Barth]

That patch makes zero sense.  It doesn't seem to block <img src="about:blank" onerror="alert(1)">.

[Adam Barth]

It also randomly nukes <dog happyhref="javascript:potato"></dog>

[Adam Barth]

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.

[Eric Seidel (no email)]

Created attachment 59333 [details]
Updated per Adam's review comments

[Adam Barth]

(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.  :)

[Adam Barth]

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.

[Eric Seidel (no email)]

(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.

[Early Warning System Bot]

Attachment 59333 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3306541

[Eric Seidel (no email)]

Created attachment 59334 [details]
Hopefully fix the EWS bots

[Early Warning System Bot]

Attachment 59334 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3286565

[Eric Seidel (no email)]

OK, so what am I missing?  I edited the .pro file to include the header...

[WebKit Review Bot]

Attachment 59333 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3330559

[Eric Seidel (no email)]

Created attachment 59337 [details]
helps if you git add the new file

[WebKit Review Bot]

Attachment 59333 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3287557

[WebKit Review Bot]

Attachment 59334 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3338594

[Adam Barth]

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.

[Adam Barth]

Committed r61637: <http://trac.webkit.org/changeset/61637>

[WebKit Review Bot]

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

[Eric Seidel (no email)]

This checkin exposed a bug in the Qt clipboard code:
https://bugs.webkit.org/show_bug.cgi?id=41025