Bug 107975

Summary: Don't use the threaded HTML parser for javascript: URLs
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106127    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Tony Gentilcore 2013-01-25 12:09:16 PST
Don't use the threaded HTML parser for javascript: URLs
Comment 1 Tony Gentilcore 2013-01-25 12:14:24 PST
Created attachment 184792 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-01-25 12:25:23 PST
Comment on attachment 184792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184792&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests because covered by existing tests.

Do you have an idea as to how many this causes to pass?

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:186
> +bool HTMLDocumentParser::shouldUseThreading() const
> +{
> +    return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL();
> +}

We had this exact logic in an earlier version, but removed it in favor of a solution which could handle empty documnts (in addition to about:blank, which just happens to be empty).  I'm surprised javascript is counted as a blank url?
Comment 3 Tony Gentilcore 2013-01-25 12:31:13 PST
(In reply to comment #2)
> (From update of attachment 184792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184792&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests because covered by existing tests.
> 
> Do you have an idea as to how many this causes to pass?

No. I only ran the tests in loading/ and it caused us to pick up 3 there. Not sure how many other tests like that are out there.

> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:186
> > +bool HTMLDocumentParser::shouldUseThreading() const
> > +{
> > +    return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL();
> > +}
> 
> We had this exact logic in an earlier version, but removed it in favor of a solution which could handle empty documnts (in addition to about:blank, which just happens to be empty).  I'm surprised javascript is counted as a blank url?

Yeah, JavaScript is counted as a blank URL. The thing that gets piped to append() is the result of the JavaScript execution, not the JavaScript itself and at that time that document has an empty URL.
Comment 4 Adam Barth 2013-01-25 17:18:23 PST
> Yeah, JavaScript is counted as a blank URL. The thing that gets piped to append() is the result of the JavaScript execution, not the JavaScript itself and at that time that document has an empty URL.

Maybe we should switch it to using insert instead?
Comment 5 Adam Barth 2013-01-25 17:20:16 PST
Comment on attachment 184792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184792&action=review

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:185
> +    return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL();

We should put this logic in HTMLParserOptions so that m_options reflects reality.

IMHO, disabling threading for blank URLs makes sense.  Given that we've wanted to do this twice, it's probably the right thing to do.
Comment 6 Tony Gentilcore 2013-01-28 10:57:37 PST
Created attachment 185009 [details]
Patch
Comment 7 Tony Gentilcore 2013-01-28 10:58:16 PST
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:185
> > +    return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL();
> 
> We should put this logic in HTMLParserOptions so that m_options reflects reality.

Done
Comment 8 Adam Barth 2013-01-28 11:09:29 PST
Comment on attachment 185009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185009&action=review

> Source/WebCore/html/parser/HTMLParserOptions.cpp:44
> -    useThreading = settings && settings->threadedHTMLParser();
> +    useThreading = settings && settings->threadedHTMLParser() && (!document || !document->url().isBlankURL());

settings being non-zero implies that document is non-zero, so you can remove the check for !document.
Comment 9 Tony Gentilcore 2013-01-28 11:13:56 PST
Created attachment 185016 [details]
Patch for landing
Comment 10 WebKit Review Bot 2013-01-28 11:37:13 PST
Comment on attachment 185016 [details]
Patch for landing

Clearing flags on attachment: 185016

Committed r140986: <http://trac.webkit.org/changeset/140986>
Comment 11 WebKit Review Bot 2013-01-28 11:37:17 PST
All reviewed patches have been landed.  Closing bug.