RESOLVED FIXED 107764
HTMLDocumentParser::insert should be aware of threaded parsing
https://bugs.webkit.org/show_bug.cgi?id=107764
Summary HTMLDocumentParser::insert should be aware of threaded parsing
Adam Barth
Reported 2013-01-23 17:16:57 PST
HTMLDocumentParser should be aware of threaded parsing
Attachments
work in progress (9.93 KB, patch)
2013-01-29 00:42 PST, Adam Barth
no flags
Patch (13.12 KB, patch)
2013-01-30 14:18 PST, Adam Barth
no flags
Patch for landing (13.26 KB, patch)
2013-01-30 14:43 PST, Adam Barth
no flags
Adam Barth
Comment 1 2013-01-29 00:10:23 PST
I'm not sure what I had in mind for this bug.
Adam Barth
Comment 2 2013-01-29 00:14:36 PST
I remember now.
Adam Barth
Comment 3 2013-01-29 00:42:58 PST
Created attachment 185188 [details] work in progress
Eric Seidel (no email)
Comment 4 2013-01-29 14:38:22 PST
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
Comment 5 2013-01-29 14:40:57 PST
(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
Comment 6 2013-01-30 14:18:18 PST
Eric Seidel (no email)
Comment 7 2013-01-30 14:31:36 PST
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
Comment 8 2013-01-30 14:39:32 PST
> 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
Comment 9 2013-01-30 14:43:00 PST
Created attachment 185581 [details] Patch for landing
Eric Seidel (no email)
Comment 10 2013-01-30 15:09:42 PST
Thank you for the explanations.
WebKit Review Bot
Comment 11 2013-01-30 15:17:35 PST
Comment on attachment 185581 [details] Patch for landing Clearing flags on attachment: 185581 Committed r141328: <http://trac.webkit.org/changeset/141328>
WebKit Review Bot
Comment 12 2013-01-30 15:17:40 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.