Bug 62336 – Script-created parsers should ignore data from the network

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Adam Barth]

Script-created parsers should ignore data from the network

[Adam Barth]

Created attachment 96499 [details]
Patch

[Darin Adler]

Comment on attachment 96499 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:345
> +    // If we have a script-created parser, we ignore data from the network
> +    // because that data belongs to the old parser.
> +    if (wasCreatedByScript())
> +        return;

Is this the right level to solve the problem at? Why does the loader try to feed in network data to the script-created parser?

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 96499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96499&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:345
> > +    // If we have a script-created parser, we ignore data from the network
> > +    // because that data belongs to the old parser.
> > +    if (wasCreatedByScript())
> > +        return;
> 
> Is this the right level to solve the problem at?

I'm not entirely sure.  We could solve this at the ScriptableDocumentParser level, which might be better because that's where the wasCreatedByScript bit lives.

> Why does the loader try to feed in network data to the script-created parser?

The loader just feeds the data to the current parser, whatever it is.  In principle, you could imagine a design there the loader grabs hold of its parser at the beginning and keeps feeding data to that exact parser.

[Tony Gentilcore]

> The loader just feeds the data to the current parser, whatever it is.  In principle, you could imagine a design there the loader grabs hold of its parser at the beginning and keeps feeding data to that exact parser.

I agree this sounds like a more robust approach. So the loader would just keep a reference to it, always send it data and let the isStopped() check in append() bail out if the original parser had been stopped or detached?

BTW, how did you come across this issue, is there a related bug filed somewhere else?

[Adam Barth]

There's a related issue with getting the EOF from the network, but that one is also very tricky to trigger.

> BTW, how did you come across this issue, is there a related bug filed somewhere else?

I found the issue while investigating a security bug.

[Adam Barth]

> I agree this sounds like a more robust approach.

Ok.  I'll take that approach.  I'm going to break this down into a few small patches because this stuff is delicate.

[Adam Barth]

Created attachment 96809 [details]
work in progress

[Adam Barth]

Created attachment 96811 [details]
Patch

[Darin Adler]

Comment on attachment 96811 [details]
Patch

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

> Source/WebCore/loader/DocumentWriter.cpp:204
> -    if (DocumentParser* parser = m_frame->document()->parser())
> -        parser->appendBytes(this, str, len, flush);
> +    if (m_frame->document()->parser())
> +        m_parser->appendBytes(this, str, len, flush);

It’s not clear what the relationship between the if and the line after it is. This may need a comment.

> Source/WebCore/loader/DocumentWriter.cpp:227
> -    if (DocumentParser* parser = m_frame->document()->parser())
> -        parser->finish();
> +    if (m_frame->document()->parser())
> +        m_parser->finish();

Same confusing idiom here.

[Adam Barth]

(In reply to comment #9)
> (From update of attachment 96811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96811&action=review
> 
> > Source/WebCore/loader/DocumentWriter.cpp:204
> > -    if (DocumentParser* parser = m_frame->document()->parser())
> > -        parser->appendBytes(this, str, len, flush);
> > +    if (m_frame->document()->parser())
> > +        m_parser->appendBytes(this, str, len, flush);
> 
> It’s not clear what the relationship between the if and the line after it is. This may need a comment.

Those branches should become more sensible in the next patch.  I tried to explain that in the ChangeLog:

[[
This patch is somewhat of an
incremental step in that DocumentWriter still looks at
document()->parser() to decide whether to call methods of m_parser.  In
a follow-up patch, I'll update those checks.
]]

I can make the change in bigger steps that changes the branch conditions at the same time, but I figured it was better to move in small, incremental steps.

[Adam Barth]

Created attachment 96823 [details]
Patch (larger step, but cleaner)

[Eric Seidel (no email)]

Comment on attachment 96823 [details]
Patch (larger step, but cleaner)

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

> Source/WebCore/loader/DocumentWriter.cpp:147
> +    m_parser = document->parser();

Ideally we'd add a comment as to why we do this.

> Source/WebCore/loader/DocumentWriter.cpp:228
> +    m_parser = 0;

Is this preferred over .clear()?  I never remember.

[WebKit Review Bot]

Comment on attachment 96823 [details]
Patch (larger step, but cleaner)

Clearing flags on attachment: 96823

Committed r88583: <http://trac.webkit.org/changeset/88583>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

(In reply to comment #12)
> (From update of attachment 96823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96823&action=review
> 
> > Source/WebCore/loader/DocumentWriter.cpp:147
> > +    m_parser = document->parser();
> 
> Ideally we'd add a comment as to why we do this.

Ok.  I'll try to remember to add a comment the next time I change this file.

> > Source/WebCore/loader/DocumentWriter.cpp:228
> > +    m_parser = 0;
> 
> Is this preferred over .clear()?  I never remember.

According to Anders, yes.

[Ryosuke Niwa]

This patch may have caused crashes on Snow Leopard Debug builds:
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r88585%20(583)/results.html

[Adam Barth]

(In reply to comment #16)
> This patch may have caused crashes on Snow Leopard Debug builds:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r88585%20(583)/results.html

Thanks.  I'll take a look.

[Tony Gentilcore]

Comment on attachment 96823 [details]
Patch (larger step, but cleaner)

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

Thanks for the fix, looks good.

> Source/WebCore/dom/Document.cpp:-2082
> -    loader()->writer()->endIfNotLoadingMainResource();

Really nice to see this go away.