Prevent the load event from firing inside DOMContentLoaded

Prevent the load event from firing inside DOMContentLoaded

BUG=406422
TEST=Updated results to fast/loader/create-frame-in-DOMContentLoaded.html

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

[Nate Chapin, 2014-11-14 00:32:34]

japhet@chromium.org changed reviewers:
+ dglazkov@chromium.org

[Nate Chapin, 2014-11-14 00:32:35]

Dimitri, is this something you're comfortable reviewing?

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
File LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt (left):

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt:1: CONSOLE
ERROR: line 34: Uncaught InvalidStateError: Failed to execute 'send' on
'XMLHttpRequest': The object's state must be OPENED.
The code changes in this CL may have neutered this test, I'm not certain.

[dglazkov, 2014-11-14 20:48:12]

I must admit, this is a little outside of my comfort zone.

[esprehn, 2014-11-14 21:33:11]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2014-11-14 21:33:12]

Document::Done seems sort of generally named, but otherwise this seems okay.
Instead of having isParsing() it almost seems like we want something more
specific to what's being checked inside the frame loader.

btw what fires the load event after DOMContentLoaded when you abort early there?

lgtm

[sof, 2014-11-14 21:34:02]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com
- esprehn@chromium.org

[sof, 2014-11-14 21:34:03]

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
File LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt (left):

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt:1: CONSOLE
ERROR: line 34: Uncaught InvalidStateError: Failed to execute 'send' on
'XMLHttpRequest': The object's state must be OPENED.
On 2014/11/14 00:32:35, Nate Chapin wrote:
> The code changes in this CL may have neutered this test, I'm not certain.

Could you investigate a bit further? i.e., should all occurrences of !parsing()
also encompass the InDocumentLoaded state - is that preventing the delivery of
DOMSubtreeModified?

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2677: if (m_parsingState == Parsing &&
!m_elementDataCache)
Use parsing() instead?

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.h#n...
Source/core/dom/Document.h:598: Done
Done is a bit short & generic... DoneParsing, FinishedParsing?

https://codereview.chromium.org/725593004/diff/1/Source/core/xml/parser/XMLDo...
File Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/725593004/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:1489:
document()->setParsingState(Document::Parsing);
Why isn't this Document::Done ?

https://codereview.chromium.org/725593004/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:1497:
document()->setParsingState(Document::Done);
Why isn't this Document::Parsing ?

[Nate Chapin, 2014-11-14 21:37:29]

On 2014/11/14 21:33:12, esprehn wrote:
> btw what fires the load event after DOMContentLoaded when you abort early
there?

A resource finishes loading, FrameLoader::finishedParsing gets called (which it
will after DOMContentLoaded finishes), a child frame completes its load event, a
couple of other cases.

[Nate Chapin, 2014-11-14 22:05:25]

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
File LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt (left):

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt:1: CONSOLE
ERROR: line 34: Uncaught InvalidStateError: Failed to execute 'send' on
'XMLHttpRequest': The object's state must be OPENED.
On 2014/11/14 21:34:02, sof wrote:
> On 2014/11/14 00:32:35, Nate Chapin wrote:
> > The code changes in this CL may have neutered this test, I'm not certain.
> 
> Could you investigate a bit further? i.e., should all occurrences of
!parsing()
> also encompass the InDocumentLoaded state - is that preventing the delivery of
> DOMSubtreeModified?

DOMSubtreeModified is firing the same number of times, but this test appears to
depend on those firing reentrantly because the load event is firing inside of
DOMContentLoaded. I'm not sure how to recreate that.

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2677: if (m_parsingState == Parsing &&
!m_elementDataCache)
On 2014/11/14 21:34:02, sof wrote:
> Use parsing() instead?

Done.

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/725593004/diff/1/Source/core/dom/Document.h#n...
Source/core/dom/Document.h:598: Done
On 2014/11/14 21:34:02, sof wrote:
> Done is a bit short & generic... DoneParsing, FinishedParsing?

Good point. FinishedParsing is probably the most consistent, will update.

https://codereview.chromium.org/725593004/diff/1/Source/core/xml/parser/XMLDo...
File Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/725593004/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:1489:
document()->setParsingState(Document::Parsing);
On 2014/11/14 21:34:02, sof wrote:
> Why isn't this Document::Done ?

Oops, thanks!

https://codereview.chromium.org/725593004/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:1497:
document()->setParsingState(Document::Done);
On 2014/11/14 21:34:02, sof wrote:
> Why isn't this Document::Parsing ?

Done.

[sof, 2014-11-14 22:16:07]

not that it is needed, but lgtm from here also.

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
File LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt (left):

https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt:1: CONSOLE
ERROR: line 34: Uncaught InvalidStateError: Failed to execute 'send' on
'XMLHttpRequest': The object's state must be OPENED.
On 2014/11/14 22:05:24, Nate Chapin wrote:
> On 2014/11/14 21:34:02, sof wrote:
> > On 2014/11/14 00:32:35, Nate Chapin wrote:
> > > The code changes in this CL may have neutered this test, I'm not certain.
> > 
> > Could you investigate a bit further? i.e., should all occurrences of
> !parsing()
> > also encompass the InDocumentLoaded state - is that preventing the delivery
of
> > DOMSubtreeModified?
> 
> DOMSubtreeModified is firing the same number of times, but this test appears
to
> depend on those firing reentrantly because the load event is firing inside of
> DOMContentLoaded. I'm not sure how to recreate that.

ok, that no DOMSubtreeModifieds were being muted was main concern. tyoshino
(Cc:ed) may have some ideas on how to rephrase the test, but fine with me to
move ahead.

[Nate Chapin, 2014-11-14 22:30:56]

On 2014/11/14 22:16:07, sof wrote:
> not that it is needed, but lgtm from here also.
> 
>
https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
> File LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt
(left):
> 
>
https://codereview.chromium.org/725593004/diff/1/LayoutTests/http/tests/xmlht...
> LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt:1: CONSOLE
> ERROR: line 34: Uncaught InvalidStateError: Failed to execute 'send' on
> 'XMLHttpRequest': The object's state must be OPENED.
> On 2014/11/14 22:05:24, Nate Chapin wrote:
> > On 2014/11/14 21:34:02, sof wrote:
> > > On 2014/11/14 00:32:35, Nate Chapin wrote:
> > > > The code changes in this CL may have neutered this test, I'm not
certain.
> > > 
> > > Could you investigate a bit further? i.e., should all occurrences of
> > !parsing()
> > > also encompass the InDocumentLoaded state - is that preventing the
delivery
> of
> > > DOMSubtreeModified?
> > 
> > DOMSubtreeModified is firing the same number of times, but this test appears
> to
> > depend on those firing reentrantly because the load event is firing inside
of
> > DOMContentLoaded. I'm not sure how to recreate that.
> 
> ok, that no DOMSubtreeModifieds were being muted was main concern. tyoshino
> (Cc:ed) may have some ideas on how to rephrase the test, but fine with me to
> move ahead.

Thanks!

[Nate Chapin, 2014-11-14 22:30:59]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-11-14 22:31:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/725593004/20001

[commit-bot: I haz the power, 2014-11-14 23:56:24]

Committed patchset #2 (id:20001) as 185397