Bug 107764 – HTMLDocumentParser::insert should be aware of threaded parsing

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

[Adam Barth]

HTMLDocumentParser should be aware of threaded parsing

[Adam Barth]

I'm not sure what I had in mind for this bug.

[Adam Barth]

I remember now.

[Adam Barth]

Created attachment 185188 [details]
work in progress

[Eric Seidel (no email)]

Comment on attachment 185188 [details]
work in progress

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:81
> +    , m_token(m_options.useThreading ? 0 : adoptPtr(new HTMLToken))
> +    , m_tokenizer(m_options.useThreading ? 0 : HTMLTokenizer::create(m_options))

I'm not entirely sure what you're going for here, but I like the idea of controlling the lifetime of these objects more carefully when using the threaded parser.  That can allow us to better ASSERT that we're doing things in the order we expect (and on the right thread).

[Adam Barth]

(In reply to comment #4)
> (From update of attachment 185188 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185188&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:81
> > +    , m_token(m_options.useThreading ? 0 : adoptPtr(new HTMLToken))
> > +    , m_tokenizer(m_options.useThreading ? 0 : HTMLTokenizer::create(m_options))
> 
> I'm not entirely sure what you're going for here, but I like the idea of controlling the lifetime of these objects more carefully when using the threaded parser.  That can allow us to better ASSERT that we're doing things in the order we expect (and on the right thread).

Yeah, this part of the change isn't strictly needed, but it seems cleaner to have a tokenizer on the main thread only if we're actually using it.

[Adam Barth]

Created attachment 185570 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 185570 [details]
Patch

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

I assume this causes a change in behavior?  Could you explain better in your ChangeLog how this affects tests (because it doesn't seem they can pass yet w/o implementing the speculation abort.)

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:706
> +            if (isWaitingForScripts() || isStopped())

Could you explain this change?  It looks right.  I assume you were pumping when you didnt' expec to?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:856
> +        if (m_parser->tokenizer())
> +            m_parser->tokenizer()->setState(HTMLTokenizerState::RCDATAState);

Should this just be setTokenizerState() on the parser, and the parser should handle the check?  That would make it possibly easier for you to compare the state after insert() with the expected state.

[Adam Barth]

> I assume this causes a change in behavior?  Could you explain better in your ChangeLog how this affects tests (because it doesn't seem they can pass yet w/o implementing the speculation abort.)

Oh, sorry.  This patch doesn't affect behavior.  I'll mention that in the ChangeLog.  It's just setting us up for the next patch which actually implements the speculation abort.  (The uberpatch is in bug 107753 if you want to see the whole picture.)

> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:706
> > +            if (isWaitingForScripts() || isStopped())
> 
> Could you explain this change?  It looks right.  I assume you were pumping when you didnt' expec to?

Yeah, if processing tokens from the background thread causes the parser to stop, then we need to not process further tokens.  (We'll hit a crash from zero document.)

That's covered by fast/parser/iframe-onload-document-close-with-external-script* (or maybe it was a different one---I made this change based on one of the fast/parser tests.)

> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:856
> > +        if (m_parser->tokenizer())
> > +            m_parser->tokenizer()->setState(HTMLTokenizerState::RCDATAState);
> 
> Should this just be setTokenizerState() on the parser, and the parser should handle the check?  That would make it possibly easier for you to compare the state after insert() with the expected state.

You're a brilliant man.  Yes, that solves two problems.  If you don't mind, I'd like to make that change when we get smarter about not blowing away the speculative parsing buffer on every document.write (which is when we'll want to hook that call).

[Adam Barth]

Created attachment 185581 [details]
Patch for landing

[Eric Seidel (no email)]

Thank you for the explanations.

[WebKit Review Bot]

Comment on attachment 185581 [details]
Patch for landing

Clearing flags on attachment: 185581

Committed r141328: <http://trac.webkit.org/changeset/141328>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.