Make XHR use the background HTML parser

Make XHR use background HTML parser

This patch enables background html parser for documents with no frames,
which are XMLHttpRequest response documents. HTML imports and
inspector/DOMPatchSupport will still to use the synchronous mode as the
current code expect the change to happen synchronously.

Before this change, XMLHttpRequest immediately set its state to DONE after
receiving the last data. After this change, XMLHttpRequest will wait for
|Document::finishedParsing| before changing its state to DONE if it has
responseType == "document". This is needed because |DocumentParser::finish()|
do not synchronously give the complete document, but we have to wait until all
tokens from the background parser is processed.

BUG=409461
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181684

[kouhei (in TOK), 2014-09-01 03:56:09]

kouhei@chromium.org changed reviewers:
+ abarth@chromium.org, eseidel@chromium.org, tyoshino@chromium.org

[kouhei (in TOK), 2014-09-01 03:56:09]

Would you take a look? This CL depends on
https://codereview.chromium.org/521403002/ so please take a look at the other CL
first.

[tyoshino (SeeGerritForStatus), 2014-09-01 05:28:03]

wrap the description to 80 col?

https://codereview.chromium.org/521363002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:260: if (!m_responseDocument->wellFormed()) {
maybe you left this after removing additional line(s) you had but removed

https://codereview.chromium.org/521363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1336: // This should only be called when
response document is asynchronous parsed.
asynchronously

[kouhei (in TOK), 2014-09-01 20:44:28]

PTAL.

https://codereview.chromium.org/521363002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:260: if (!m_responseDocument->wellFormed()) {
On 2014/09/01 05:28:02, tyoshino wrote:
> maybe you left this after removing additional line(s) you had but removed

Done.

https://codereview.chromium.org/521363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:947: // FIXME: abort parser
Removed this comment, as this is addressed in the other CL:
https://codereview.chromium.org/521403002/

[tyoshino (SeeGerritForStatus), 2014-09-02 04:12:16]

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:952: }
I recommend that we place this close to stopParsing() call.

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.h:155: // When we use the off-thread HTML parser,
we don't have a complete document just after |HTMLDocumentParser::finish()|.
let's wrap comments in 80 col

[tyoshino (SeeGerritForStatus), 2014-09-02 04:19:19]

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1308: m_responseDocumentParser->finish();
We are never notified of completion synchronously? If it's possible for some
cases, how about adding this to xhrPendingDocumentParserMap() before calling
finish()?

[kouhei (in TOK), 2014-09-02 16:17:27]

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:952: }
On 2014/09/02 04:12:16, tyoshino wrote:
> I recommend that we place this close to stopParsing() call.

Done.

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1308: m_responseDocumentParser->finish();
On 2014/09/02 04:19:19, tyoshino wrote:
> We are never notified of completion synchronously? If it's possible for some
> cases, how about adding this to xhrPendingDocumentParserMap() before calling
> finish()?

Great catch. Done.

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/521363002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.h:155: // When we use the off-thread HTML parser,
we don't have a complete document just after |HTMLDocumentParser::finish()|.
On 2014/09/02 04:12:16, tyoshino wrote:
> let's wrap comments in 80 col

Done.

[abarth-chromium, 2014-09-02 20:13:19]

https://codereview.chromium.org/521363002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/521363002/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4699: xhr->didFinishParsingDocument();
Can we make this into a generic client interface instead of teaching this code
about all of its callers?

https://codereview.chromium.org/521363002/diff/100001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/100001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:139: }
Rather than using a HashMap, we should use a client interface and a WeakPtr.

[kouhei (in TOK), 2014-09-03 03:23:51]

https://codereview.chromium.org/521363002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/521363002/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4699: xhr->didFinishParsingDocument();
On 2014/09/02 20:13:19, abarth wrote:
> Can we make this into a generic client interface instead of teaching this code
> about all of its callers?

Got it. That makes more sense. Let me create a separate CL for it, and come back
to this CL after the interface is ready.

[kouhei (in TOK), 2014-09-09 22:14:41]

Would you take another look?

https://codereview.chromium.org/521363002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/521363002/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4699: xhr->didFinishParsingDocument();
On 2014/09/03 03:23:50, kouhei wrote:
> On 2014/09/02 20:13:19, abarth wrote:
> > Can we make this into a generic client interface instead of teaching this
code
> > about all of its callers?
> 
> Got it. That makes more sense. Let me create a separate CL for it, and come
back
> to this CL after the interface is ready.

Done. Rewrote the patch to use the DocumentParserClient interface.

[eseidel, 2014-09-09 22:18:20]

lgtm

https://codereview.chromium.org/521363002/diff/200001/Source/core/inspector/D...
File Source/core/inspector/DOMPatchSupport.cpp (right):

https://codereview.chromium.org/521363002/diff/200001/Source/core/inspector/D...
Source/core/inspector/DOMPatchSupport.cpp:100: parser->pinToMainThread();
This shouldn't be necessary with insert(), but seems like a good idea
regardless.

[kouhei (in TOK), 2014-09-09 22:25:31]

On 2014/09/09 22:18:20, eseidel wrote:
> lgtm
> 
>
https://codereview.chromium.org/521363002/diff/200001/Source/core/inspector/D...
> File Source/core/inspector/DOMPatchSupport.cpp (right):
> 
>
https://codereview.chromium.org/521363002/diff/200001/Source/core/inspector/D...
> Source/core/inspector/DOMPatchSupport.cpp:100: parser->pinToMainThread();
> This shouldn't be necessary with insert(), but seems like a good idea
> regardless.

This was to avoid assert:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[kouhei (in TOK), 2014-09-09 22:29:00]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:30:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/521363002/200001

[commit-bot: I haz the power, 2014-09-09 22:46:12]

Committed patchset #11 (id:200001) as 181684

[tyoshino (SeeGerritForStatus), 2014-09-10 06:46:37]

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:930: if (m_responseDocumentParser &&
!m_responseDocumentParser->isStopped())
how about calling
m_responseDocumentParser->removeClient(this);
here?

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:1048: 
if the loader is cancelled by someone else than the client, we reach here. we're
currently not calling internalAbort(). So, we clears m_responseDocument but not
m_responseDocumentParser until open() calls internalAbort().

I'm going to add internalAbort() call in
https://codereview.chromium.org/490083002/.

[kouhei (in TOK), 2014-09-10 20:50:42]

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:930: if (m_responseDocumentParser &&
!m_responseDocumentParser->isStopped())
On 2014/09/10 06:46:36, tyoshino wrote:
> how about calling
> m_responseDocumentParser->removeClient(this);
> here?

Would you elabolate on it? I want to know why you prefer it.
I thought addClient(this) at where parser created, and removeClient(this) at
where parser ref is disposed is simple.

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:1048: 
On 2014/09/10 06:46:36, tyoshino wrote:
> if the loader is cancelled by someone else than the client, we reach here.
we're
> currently not calling internalAbort(). So, we clears m_responseDocument but
not
> m_responseDocumentParser until open() calls internalAbort().
> 
> I'm going to add internalAbort() call in
> https://codereview.chromium.org/490083002/.
Hmm. I guess you mean that handleDidFailGeneric doesn't call
clearVariablesForLoading. This is indeed weird. your patch sgtm

[tyoshino (SeeGerritForStatus), 2014-09-11 02:58:00]

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:930: if (m_responseDocumentParser &&
!m_responseDocumentParser->isStopped())
On 2014/09/10 20:50:42, kouhei wrote:
> On 2014/09/10 06:46:36, tyoshino wrote:
> > how about calling
> > m_responseDocumentParser->removeClient(this);
> > here?
> 
> Would you elabolate on it? I want to know why you prefer it.
> I thought addClient(this) at where parser created, and removeClient(this) at
> where parser ref is disposed is simple.

I saw "notifyParserStopped" checking m_error and if it's true becomes no-op. So,
I guessed we could just prevent it from being invoked by calling removeClient.
but right. We continue to use m_responseDocumentParser. So, it's not the right
thing to do. Never mind. Thanks

[tyoshino (SeeGerritForStatus), 2014-09-11 03:41:32]

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/521363002/diff/200001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:1048: 
On 2014/09/10 20:50:42, kouhei wrote:
> On 2014/09/10 06:46:36, tyoshino wrote:
> > if the loader is cancelled by someone else than the client, we reach here.
> we're
> > currently not calling internalAbort(). So, we clears m_responseDocument but
> not
> > m_responseDocumentParser until open() calls internalAbort().
> > 
> > I'm going to add internalAbort() call in
> > https://codereview.chromium.org/490083002/.
> Hmm. I guess you mean that handleDidFailGeneric doesn't call
> clearVariablesForLoading. This is indeed weird. your patch sgtm

Right!