Bug 107975 – Don't use the threaded HTML parser for javascript: URLs

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Gentilcore]

Don't use the threaded HTML parser for javascript: URLs

[Tony Gentilcore]

Created attachment 184792 [details]
Patch

[Eric Seidel (no email)]

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?

[Tony Gentilcore]

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

[Adam Barth]

> 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?

[Adam Barth]

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.

[Tony Gentilcore]

Created attachment 185009 [details]
Patch

[Tony Gentilcore]

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

[Adam Barth]

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.

[Tony Gentilcore]

Created attachment 185016 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 185016 [details]
Patch for landing

Clearing flags on attachment: 185016

Committed r140986: <http://trac.webkit.org/changeset/140986>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.