NoStatePrefetch early prototype (issue 2172613002 by droger@chromium.org)

[dro...@chromium.org]

Reviewers:
CL: https://codereview.chromium.org/2172613002/

Message:
lizeb, mattcary, pasko: FYI this is what I'm experimenting with for prefetching
on the renderer side.

Description:
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual requests, and drops
everything else on the floor, preventing the actual
construction and execution of the page.

Uses the PageVisibilityState to control this behavior.

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+23, -1 lines):
M third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp


Index: third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
diff --git a/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp b/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
index 7908a5d662ad1803203fed61efc3a9c9597b5b0d..2e6feabc100063ac8055497b35f64622fc31df0e 100644
--- a/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
+++ b/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
@@ -296,10 +296,14 @@ void HTMLDocumentParser::notifyPendingParsedChunks()
if (!isParsing())
return;

+ bool isPrefetch =
+ document()->pageVisibilityState() == PageVisibilityStatePrerender;
+
// ApplicationCache needs to be initialized before issuing preloads.
// We suspend preload until HTMLHTMLElement is inserted and
// ApplicationCache is initialized.
- if (!document()->documentElement()) {
+ // Prefetching never initializes AppCache: is this a problem?
+ if (!isPrefetch && !document()->documentElement()) {
for (auto& chunk : pendingChunks) {
for (auto& request : chunk->preloads)
m_queuedPreloads.append(std::move(request));
@@ -319,9 +323,14 @@ void HTMLDocumentParser::notifyPendingParsedChunks()
// document.write script evaluation takes place. Preloading these
// scripts is valuable and comparably cheap, while evaluating JS can be
// expensive.
+ printf("------------------------------------------\n");
+ printf("HTMLDocumentParser:\n");
for (auto& chunk : pendingChunks) {
+ for (const auto& request : chunk->preloads)
+ printf("%s\n", request->resourceURL().ascii().data());
m_preloader->takeAndPreload(chunk->preloads);
}
+ printf("------------------------------------------\n");
for (auto& chunk : pendingChunks) {
for (auto& index : chunk->likelyDocumentWriteScriptIndices) {
const CompactHTMLToken& token = chunk->tokens->at(index);
@@ -334,6 +343,19 @@ void HTMLDocumentParser::notifyPendingParsedChunks()
for (auto& chunk : pendingChunks)
m_speculations.append(std::move(chunk));

+ if (isPrefetch) {
+ // Do nothing, but tell the background parser that we processed
+ // everything.
+ while (!m_speculations.isEmpty()) {
+ std::unique_ptr<ParsedChunk> speculation =
+ m_speculations.takeFirst();
+ postTaskToLookaheadParser(
+ Asynchronous, &BackgroundHTMLParser::startedChunkWithCheckpoint,
+ m_backgroundParser, speculation->inputCheckpoint);
+ }
+ return;
+ }
+
if (!isWaitingForScripts() && !isScheduledForResume()) {
if (m_tasksWereSuspended)
m_parserScheduler->forceResumeAfterYield();

[dro...@chromium.org]

Using the PageVisibilityState is probably not the right way to know if we're
doing a prefetch.
In particular this enum is part of the web platform, and adding a new value to
it is probably not easy and not what we want.

I'm trying to figure out the plumbing, how we could distinguish between a
prefetch and a renderer.

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

On 2016/07/25 14:33:18, droger wrote:
> prefetch and a renderer.

Oops, should be:

prefetch and a prerender.



https://codereview.chromium.org/2172613002/

[Matthew Cary]

OK, I just started looking into this as well.

Good to know that adding the enum to PageVisibilityState isn't practical, that was one of my alternatives.

[dro...@chromium.org]

Added some more plumbing to get the prefetching bit from the prerender helper

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

Finished plumbing to the IPC.

As per offline discussion, it may be possible to avoid the change to the
Document API by fiddling with the many various Prerender clients and helpers,
but it requires at least a static cast (or two) and almost certainly exposing
more implementation details.

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

+CC csharrison

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

+kouhei

Hm I haven't taken a long look at this but it seems like we could do something
simpler? Using HTMLDocumentParser in full seems overkill, especially if we
aren't actually parsing / running scripts / etc.

I wonder if you can turn off the threaded parser and just use HTMLPreloadScanner
strings from HTMLDocumentParser::append. This avoids doing unnecessary
multithreading / XSS scanning that the BackgroundHTMLParser does. There's a ton
of overhead there.


https://codereview.chromium.org/2172613002/

[pa...@chromium.org]

Thanks for input!

Yeah, I think we should get to something of this sort. Though maybe slightly
later. The overhead is arguably not on the critical path, we'll be able to
optimize it later. The aim of this patch is to make a quick broken prototype (no
security checks, etc.), and piggybacking on BackgroundHTMLParser seems indeed
easier to cook.

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

Sure, whatever works. Though I think the suggestion would not necessarily be
more complicated.

For a prototype, the simplest implementation SGTM.

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

On 2016/07/27 15:27:39, csharrison wrote:
> +kouhei
>
> Hm I haven't taken a long look at this but it seems like we could do something
> simpler? Using HTMLDocumentParser in full seems overkill, especially if we
> aren't actually parsing / running scripts / etc.
>
> I wonder if you can turn off the threaded parser and just use
HTMLPreloadScanner
> strings from HTMLDocumentParser::append. This avoids doing unnecessary
> multithreading / XSS scanning that the BackgroundHTMLParser does. There's a
ton
> of overhead there.

Thanks. I'll look into this and see how it goes!

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

On 2016/07/27 16:35:59, csharrison wrote:
>
> Sure, whatever works. Though I think the suggestion would not necessarily be
> more complicated.
>

I uploaded a new version (patch 6) implementing the suggestion, let me know what
you think!


https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

On 2016/07/28 13:26:08, droger wrote:
> On 2016/07/27 16:35:59, csharrison wrote:
> >
> > Sure, whatever works. Though I think the suggestion would not necessarily be
> > more complicated.
> >
>
> I uploaded a new version (patch 6) implementing the suggestion, let me know
what
> you think!

Yes! This is exactly what I was thinking! Thanks for implementing it :)

What is the plan with this CL? Do you want a full review?

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

Yes, the plan is to implement the feature, so you can do a full review.
This code path will not be used yet, as we also need to make corresponding
changes on the browser side.

I'm not familiar with webkit, but maybe I should consider writing some tests?

Thanks

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

Okay cool. I am not an owner, so I will review in addition to another Blink
owner (I've cced Kouhei, who is an owner).
Tests would be good. Unit tests are pretty easy in Blink, though they are not as
common as they should be. Browser tests would also work if that's doable with
the current code.

Would you mind expanding on the CL description and say how this is all being
hooked up?

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

Added unit tests. Please take a look.

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

Thanks! cc shivanisha@ for the "fire and forget" question.

It would be nice to also see (potentially in a follow up):
- A browser test that successfully observes requests going through the network
stack.
- A test that confirms that we aren't actually parsing the document.


https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome_content_renderer_client.cc
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome_content_renderer_client.cc#newcode489
chrome/renderer/chrome_content_renderer_client.cc:489: RenderFrame*
main_frame =
Optional: It might make sense to hide some of this logic into
PrerendererHelper's constructor (i.e. getting the mode) to simplify
construction sites.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/dom/Document.h
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/dom/Document.h#newcode355
third_party/WebKit/Source/core/dom/Document.h:355: bool isPrefetchOnly()
const;
Please document this method, especially since "Prefetch" is such an
overloaded term.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode93
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:93:
HTMLDocumentParser* HTMLDocumentParser::create(
I have a slight preference for passing the sync policy from the
callsite, instead of reaching into the document in
threadingAllowedForDocument.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode727
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:727:
m_insertionPreloadScanner->scanAndPreload(
nit: Unnecessary line break.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode814
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);
This just preloads the tokens normally. Do we want to add load flags
like LOAD_PREFETCH so all resources are forced into the HTTP cache?

Also, if we aren't using this render process for the final page load,
this will bloat the MemoryCache with in-memory resources that will never
be used.

+shivanisha@ is thinking of implementing a "fire-and-forget" system in
Blink that will send requests but ignore results (make sure they don't
land in the memory cache). It might make sense to collaborate on this.

(note: I understand this is just a prototype, I just want to understand
what the final design will look like.)

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode1066
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066:
if (!length || isStopped())
Can you DCHECK(!document()->isPrefetchOnly()) here?

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h#newcode82
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:82: //
Exposed for testing.
Append "ForTesting" to the end of methods that should only be used for
testing.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
File
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp#newcode18
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:18:

So, it is against Blink style to indent in a nested namespace. This is
really confusing because I think clang format does this sometimes.
Hopefully this will all be fixed once chromium style is merged... :(

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp#newcode99
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:99:
EXPECT_TRUE(scriptRunnerHost->hasPreloadScanner());
might make more sense to be checking m_haveBackgroundParser.
hasPreloadScanner() is less clear.

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

Thanks for the review!

Re: adding a test to check that the document is not parsed.
This is what I was intending to test by checking the test of the tokenizer in
the unittest. Were you thinking of another way of testing this?

https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome_content_renderer_client.cc
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome_content_renderer_client.cc#newcode489
chrome/renderer/chrome_content_renderer_client.cc:489: RenderFrame*
main_frame =

On 2016/08/03 14:40:03, csharrison wrote:
> Optional: It might make sense to hide some of this logic into
> PrerendererHelper's constructor (i.e. getting the mode) to simplify
construction
> sites.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/dom/Document.h
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/dom/Document.h#newcode355
third_party/WebKit/Source/core/dom/Document.h:355: bool isPrefetchOnly()
const;

On 2016/08/03 14:40:03, csharrison wrote:
> Please document this method, especially since "Prefetch" is such an
overloaded
> term.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode93
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:93:
HTMLDocumentParser* HTMLDocumentParser::create(

On 2016/08/03 14:40:03, csharrison wrote:
> I have a slight preference for passing the sync policy from the
callsite,
> instead of reaching into the document in threadingAllowedForDocument.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode727
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:727:
m_insertionPreloadScanner->scanAndPreload(

On 2016/08/03 14:40:03, csharrison wrote:
> nit: Unnecessary line break.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode814
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);

On 2016/08/03 14:40:03, csharrison wrote:
> This just preloads the tokens normally. Do we want to add load flags
like
> LOAD_PREFETCH so all resources are forced into the HTTP cache?
>

This uses the same code as the standard preload scanner. I would think
that it uses the LOAD_PREFETCH flags accordingly, and (at first at
least) we want to have the same behavior for prefetch.

> Also, if we aren't using this render process for the final page load,
this will
> bloat the MemoryCache with in-memory resources that will never be
used.
>

Yes, this is a good point. Disabling the memory cache would probably be
nice.

> +shivanisha@ is thinking of implementing a "fire-and-forget" system in
Blink
> that will send requests but ignore results (make sure they don't land
in the
> memory cache). It might make sense to collaborate on this.
>
> (note: I understand this is just a prototype, I just want to
understand what the
> final design will look like.)

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode1066
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066:
if (!length || isStopped())

On 2016/08/03 14:40:03, csharrison wrote:
> Can you DCHECK(!document()->isPrefetchOnly()) here?

Why? Is this because isStopped() should never be true when prefetching?

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
File
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp#newcode18
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:18:

On 2016/08/03 14:40:04, csharrison wrote:
> So, it is against Blink style to indent in a nested namespace. This is
really
> confusing because I think clang format does this sometimes. Hopefully
this will
> all be fixed once chromium style is merged... :(

Yes, `git cl format` did this.
Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp#newcode99
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:99:
EXPECT_TRUE(scriptRunnerHost->hasPreloadScanner());

On 2016/08/03 14:40:04, csharrison wrote:
> might make more sense to be checking m_haveBackgroundParser.
hasPreloadScanner()
> is less clear.

Why checking the background parser? Since we are in synchronous mode,
there should not be one.

I'm checking the hasPreloadScanner as a way to check that prefetching
has been done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp#newcode100
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:100:
EXPECT_EQ(HTMLTokenizer::DataState, parser->tokenizer()->getState());
This is intended to check that the page has is not being parsed (as
DataState seems to be the default idle state for the Tokenizer).

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

Thanks for the clarification!

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode814
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);

Nope, LOAD_PREFETCH afaik is only used for the link rel="prefetch"
feature. These loads will look like normal requests from the net stack,
and will not follow special caching behavior like LOAD_PREFETCH.

I did a brief trace and you can see that the load flag is added when the
request has RequestContext::RequestContextPrefetch which only happens
for link rel prefetches. One thing that you could do in this patch to
solve this problem is to inspect the preloads that are being scanned,
and set their type to LinkRelPrefetch. This would require:
1. Adding "setForPrefetch" to PreloadRequest.h which sets the resource
type to LinkRelPrefetch.
2. one of:
a. Modifying HTMLPreloadScanner in some way to do this for you.
b. Using a TokenPreloadScanner instead of HTMLPreloadScanner (look at
HTMLPreloadScanner for how to do this)

If you'd like LOAD_PREFETCH on your requests, you'll need to do this
otherwise you will double download no-store resources, etc.

This technique sounds reasonable to me, but again, I am not an owner so
you'll need to get sign-off from some blink owner :)

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode1066
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066:
if (!length || isStopped())

On 2016/08/03 16:45:50, droger wrote:
> On 2016/08/03 14:40:03, csharrison wrote:
> > Can you DCHECK(!document()->isPrefetchOnly()) here?
>
> Why? Is this because isStopped() should never be true when
prefetching?

This method should not run in the non-threaded model (pretty sure).

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
File
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp#newcode99
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:99:
EXPECT_TRUE(scriptRunnerHost->hasPreloadScanner());

On 2016/08/03 16:45:50, droger wrote:
> On 2016/08/03 14:40:04, csharrison wrote:
> > might make more sense to be checking m_haveBackgroundParser.
> hasPreloadScanner()
> > is less clear.
>
> Why checking the background parser? Since we are in synchronous mode,
there
> should not be one.
>
> I'm checking the hasPreloadScanner as a way to check that prefetching
has been
> done.

Ah nevermind, makes sense.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp#newcode100
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:100:
EXPECT_EQ(HTMLTokenizer::DataState, parser->tokenizer()->getState());

On 2016/08/03 16:45:50, droger wrote:
> This is intended to check that the page has is not being parsed (as
DataState
> seems to be the default idle state for the Tokenizer).

Nice! Yeah that works.

https://codereview.chromium.org/2172613002/

[pa...@chromium.org]

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode814
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);

csharrison: thank you for investigation and the info. Let's handle load
flags properly in a separate patch. I indeed see that LOAD_PREFETCH is
not set in the net stack, but strangely the following page load still
fetches from cache ignoring validators. Hmm.. I thought only
LOAD_PREFETCH can do that..

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

Try killing the tab before loading the page again, they may be still in
MemoryCache?

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

I agree, lets do this in a follow-up. This patch is already very large.

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

SGTM. The patch as-is looks good to me as long as we add TODOs to add the load
flags. I will try to a final pass today or Monday.

Very exciting work!

https://codereview.chromium.org/2172613002/

[pa...@chromium.org]

On 2016/08/04 12:26:01, csharrison wrote:
> > csharrison: thank you for investigation and the info. Let's handle load
flags
> > properly in a separate patch. I indeed see that LOAD_PREFETCH is not set in
> the
> > net stack, but strangely the following page load still fetches from cache
> > ignoring validators. Hmm.. I thought only LOAD_PREFETCH can do that..
>

> Try killing the tab before loading the page again, they may be still in
> MemoryCache?

yah, good idea, checking this. I assumed that my browser-initiated navigation
would choose another process, but it could be that omnibox swapped in our
prerender.

https://codereview.chromium.org/2172613002/

[dro...@chromium.org]

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode1066
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066:
if (!length || isStopped())

On 2016/08/03 17:15:14, csharrison wrote:
> On 2016/08/03 16:45:50, droger wrote:
> > On 2016/08/03 14:40:03, csharrison wrote:

> > > Can you DCHECK(!document()->isPrefetchOnly()) here?
> >
> > Why? Is this because isStopped() should never be true when
prefetching?
>
> This method should not run in the non-threaded model (pretty sure).

Is "This method" appendBytes()? Or isStopped()?

appendBytes() is run for me, and looking at the "if
(shouldUseThreading())" line 1069 below, I think this is expected.

https://codereview.chromium.org/2172613002/

[cshar...@chromium.org]

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp#newcode1066
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066:
if (!length || isStopped())

On 2016/08/04 12:42:29, droger wrote:
> On 2016/08/03 17:15:14, csharrison wrote:
> > On 2016/08/03 16:45:50, droger wrote:
> > > On 2016/08/03 14:40:03, csharrison wrote:
> > > > Can you DCHECK(!document()->isPrefetchOnly()) here?
> > >
> > > Why? Is this because isStopped() should never be true when
prefetching?
> >
> > This method should not run in the non-threaded model (pretty sure).
>
> Is "This method" appendBytes()? Or isStopped()?
>
> appendBytes() is run for me, and looking at the "if
(shouldUseThreading())" line
> 1069 below, I think this is expected.

Ah you're right, I was misremember the control flow for the non-threaded
case. Apologies.

https://codereview.chromium.org/2172613002/