Bug 107764

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

I remember now.

Created attachment 185188 [details] work in progress

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

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

Created attachment 185570 [details] Patch

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.

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

Created attachment 185581 [details] Patch for landing

Thank you for the explanations.

Comment on attachment 185581 [details] Patch for landing Clearing flags on attachment: 185581 Committed r141328: <http://trac.webkit.org/changeset/141328>

All reviewed patches have been landed. Closing bug.