Properly suspend HTMLDocumentParser

Properly suspend HTMLDocumentParser

When an ExecutionContext was suspended, the HTMLDocumentParser could
still handle parsed chunks, incrementaly build the tree, trigger resource
loads, and build iframes with new clean ExecutionContexts which were not
suspended, therefore triggering ASSERT(m_suspended) inside SuspendableTimer.

Also moved some Document specific code from ScopedPageLoadDeferrer to
FrameLoader, where it logically feels better so it's more related to the
defersLoading state handling, and so it makes the Page to FrameHost
migration easier.

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

[João Eiras, 2014-10-06 12:14:30]

joaoe@opera.com changed reviewers:
+ abarth@chromium.org, eseidel@chromium.org

[João Eiras, 2014-10-06 12:14:31]

https://codereview.chromium.org/625583002/diff/40001/Source/core/dom/Executio...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/625583002/diff/40001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.cpp:89:
ASSERT(!m_activeDOMObjectsAreSuspended);
This matches the asserts that already exist in SuspendableTimer, so it helps
finding problems sooner by putting them here.

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:286: if (m_haveBackgroundParser
|| !m_speculations.isEmpty()) {
I'm a bit unsure about how these two variables relate, hence I modified this
check because m_speculations is being added the chunks when the parser is
suspended. If you tell me that m_speculations depend on m_haveBackgroundParser
being set to true, then I can revert this line.

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:293: if (m_tokenizer)
As consequence of forcing a call to this method when resumeScheduledTasks is
called, m_tokenizer will be empty for document that have done all their parsing.
Instead of adding even more state to know if
didReceiveParsedChunkFromBackgroundParser was called or not, this is the
simplest way I found to avoid the assert inside pumpTokenizer().

https://codereview.chromium.org/625583002/diff/40001/Source/core/page/ScopedP...
File Source/core/page/ScopedPageLoadDeferrer.cpp (left):

https://codereview.chromium.org/625583002/diff/40001/Source/core/page/ScopedP...
Source/core/page/ScopedPageLoadDeferrer.cpp:54:
toLocalFrame(frame)->document()->suspendScheduledTasks();
I moved this code into FrameLoader::setDefersLoading() where it feels it belongs
better. It will also make the Page to FrameHost migration easier.

[eseidel, 2014-10-06 18:07:08]

Can you add a test case?

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:286: if (m_haveBackgroundParser
|| !m_speculations.isEmpty()) {
On 2014/10/06 12:14:30, João Eiras wrote:
> I'm a bit unsure about how these two variables relate, hence I modified this
> check because m_speculations is being added the chunks when the parser is
> suspended. If you tell me that m_speculations depend on m_haveBackgroundParser
> being set to true, then I can revert this line.

Speculations are parsed chunks sent to us from the background parser.  Unclear
how you could ever have speculations and not have a background parser...

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:293: if (m_tokenizer)
On 2014/10/06 12:14:31, João Eiras wrote:
> As consequence of forcing a call to this method when resumeScheduledTasks is
> called, m_tokenizer will be empty for document that have done all their
parsing.
> Instead of adding even more state to know if
> didReceiveParsedChunkFromBackgroundParser was called or not, this is the
> simplest way I found to avoid the assert inside pumpTokenizer().

This seems wrong.  We should just be more careful as to when we call this
method.

[João Eiras, 2014-10-07 13:25:07]

On 2014/10/06 18:07:08, eseidel wrote:
> Can you add a test case?
> 

Wouldn't know very well how to begin. The original tc is html+js which has an
alert() that needs user input (cliking the ok button). AFAIK, putting that tc in
LayoutTests won't work because there's no one to click the alert. The alert is
there to create the ScopedPageLoadDeferrer.

So, Id have to resort to a cpp test but that's going to be complex... lets see
what I can do.

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:286: if (m_haveBackgroundParser
|| !m_speculations.isEmpty()) {
OK, line reverted.

https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:293: if (m_tokenizer)
Ok, added a couple extra checks in HTMLDocumentParser::resumeScheduledTasks()
then

[João Eiras, 2014-11-12 17:40:55]

On 2014/10/07 13:25:07, João Eiras wrote:
> On 2014/10/06 18:07:08, eseidel wrote:
> > Can you add a test case?
> > 
> 

Hi Eric. Been busy with other stuff, and thing how to tackle this. Making a c++
testcase to explicitly load chunks of html and somewhere in the middle set the
defersLoading flag is an endeavor which I don't think I could accomplish besides
being laborious and quite complicated for the sake of a single CL.
So I just though of adding a pseudo-alert method to testRunner which emulates
what happens in the testcase linked on the bug report, by setting the
defersLoading flag for a small interval. What do you think ? Most of the code is
in test_runner.cc and is quite straightforward.

[João Eiras, 2014-11-18 15:09:52]

On 2014/10/06 18:07:08, eseidel wrote:
> Can you add a test case?
> 

I did, finally.

I added this extra API to testRunner
https://codereview.chromium.org/736723002
https://codereview.chromium.org/725593006

And this bug also has another sibling fix
https://codereview.chromium.org/737763002

[eseidel, 2014-11-19 04:13:59]

I've moved off the blink project.  Kouhei is a better reviewer for this now.

[kouhei (in TOK), 2014-11-19 07:56:10]

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

[kouhei (in TOK), 2014-11-19 07:56:11]

https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/html-document-parser-suspended-0.html (right):

https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser...
LayoutTests/fast/parser/html-document-parser-suspended-0.html:21: <p><iframe
src="data:text/html,%3Ch1%3EHi%3C%2Fh1%3E%3Ciframe%20src%3D%22data%3Atext%2Fhtml%2C%3Ch1%3EHi%22%3E%3C%2Fiframe%3E"></iframe></p>
Nit: Would you use data:text/html;charset=utf-8, and/or javascript so it is more
readable?

https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/html-document-parser-suspended-40.html (right):

https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser...
LayoutTests/fast/parser/html-document-parser-suspended-40.html:21: <p><iframe
src="data:text/html,%3Ch1%3EHi%3C%2Fh1%3E%3Ciframe%20src%3D%22data%3Atext%2Fhtml%2C%3Ch1%3EHi%22%3E%3C%2Fiframe%3E"></iframe></p>
Ditto.

https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.h:202: bool m_tasksWereSuspended;
Would you move this flag under HTMLParserScheduler? I think the flag is more
well-fitted there.

[João Eiras, 2014-11-21 12:52:35]

On 2014/10/06 18:07:08, eseidel wrote:
> Can you add a test case?
> 

After thinking a bit more, and seeing how people are reluctant about the tests I
added and the extra testRunner API, I have to say the test is unnecessary
because of the extra ASSERTs introduced by this CL. If the old behavior is
reintroduced, many of these ASSERTs will trigger. So, can I pretty please, skip
the unit test ? It is also quite hard to do a test for this, because it implies
suspending the Document, and then checking something does not happen, which will
always be dependent on timing, and with timing comes unstable tests.

[João Eiras, 2014-11-24 12:46:23]

On 2014/11/21 12:52:35, João Eiras wrote:
> On 2014/10/06 18:07:08, eseidel wrote:
> > Can you add a test case?
> > 
> 
> After thinking a bit more, and seeing how people are reluctant about the tests
I
> added and the extra testRunner API, I have to say the test is unnecessary
> because of the extra ASSERTs introduced by this CL. (...)

I also thought about changing the Sleep method to a BlockUntilIdle which would
call MessageLoop::RunUntilIdle(), but the bug depends on stuff being handled in
the html parser thread, so that would not be sufficient.

[kouhei (in TOK), 2014-11-25 01:43:32]

On 2014/11/24 12:46:23, João Eiras wrote:
> On 2014/11/21 12:52:35, João Eiras wrote:
> > On 2014/10/06 18:07:08, eseidel wrote:
> > > Can you add a test case?
> > > 
> > 
> > After thinking a bit more, and seeing how people are reluctant about the
tests
> I
> > added and the extra testRunner API, I have to say the test is unnecessary
> > because of the extra ASSERTs introduced by this CL. (...)
> 
> I also thought about changing the Sleep method to a BlockUntilIdle which would
> call MessageLoop::RunUntilIdle(), but the bug depends on stuff being handled
in
> the html parser thread, so that would not be sufficient.

This codepath will be covered in inspector tests after change in parser
scheduling:
https://codereview.chromium.org/673603002
So I'm ok with only ASSERTS for now.

[kouhei (in TOK), 2014-11-25 06:19:37]

> This codepath will be covered in inspector tests after change in parser
> scheduling:
> https://codereview.chromium.org/673603002
> So I'm ok with only ASSERTS for now.
Would you rebase the CL and remove the LayoutTests for now?

[João Eiras, 2014-11-25 16:01:49]

On 2014/11/25 06:19:37, kouhei wrote:
> > This codepath will be covered in inspector tests after change in parser
> > scheduling:

Ah, good. I like then to take a peek at those tests.

> > https://codereview.chromium.org/673603002
> > So I'm ok with only ASSERTS for now.
> Would you rebase the CL and remove the LayoutTests for now?

Done.

[kouhei (in TOK), 2014-11-26 03:00:24]

lgtm

[kouhei (in TOK), 2014-11-26 03:03:38]

On 2014/11/25 16:01:49, João Eiras wrote:
> On 2014/11/25 06:19:37, kouhei wrote:
> > > This codepath will be covered in inspector tests after change in parser
> > > scheduling:
> 
> Ah, good. I like then to take a peek at those tests.
inspector/sources/debugger/

Inspector seems to use suspend for breakpoints in debugger.

[João Eiras, 2014-11-27 01:18:38]

https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.h:202: bool m_tasksWereSuspended;
On 2014/11/19 07:56:10, kouhei wrote:
> Would you move this flag under HTMLParserScheduler? I think the flag is more
> well-fitted there.

Btw, what about this ? Still want me to change it (since you lgtm'ed the review)
?

[João Eiras, 2014-11-27 01:21:01]

> This codepath will be covered in inspector tests after change in parser
> scheduling:
> https://codereview.chromium.org/673603002
> So I'm ok with only ASSERTS for now.

I saw that you merge this CL (and got conflicts). Should I still try to commit
it ?

[kouhei (in TOK), 2014-11-27 01:51:04]

On 2014/11/27 01:21:01, João Eiras wrote:
> > This codepath will be covered in inspector tests after change in parser
> > scheduling:
> > https://codereview.chromium.org/673603002
> > So I'm ok with only ASSERTS for now.
> 
> I saw that you merge this CL (and got conflicts). Should I still try to commit
> it ?

The merge is just for trybot. Please commit this :)

https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.h:202: bool m_tasksWereSuspended;
On 2014/11/27 01:18:38, João Eiras wrote:
> On 2014/11/19 07:56:10, kouhei wrote:
> > Would you move this flag under HTMLParserScheduler? I think the flag is more
> > well-fitted there.
> 
> Btw, what about this ? Still want me to change it (since you lgtm'ed the
review)
> ? 
I think we should move the flag, but not necessarily in this CL.

[João Eiras, 2014-11-27 11:51:31]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-11-27 11:52:32]

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

[João Eiras, 2014-11-27 11:53:30]

The CQ bit was checked by joaoe@opera.com

[João Eiras, 2014-11-27 11:53:36]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-11-27 11:53:39]

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

[João Eiras, 2014-11-27 11:53:40]

The CQ bit was checked by joaoe@opera.com

[João Eiras, 2014-11-27 11:53:47]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-11-27 11:53:58]

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

[João Eiras, 2014-11-27 11:54:12]

The CQ bit was checked by joaoe@opera.com

[João Eiras, 2014-11-27 11:54:21]

The CQ bit was checked by joaoe@opera.com

[João Eiras, 2014-11-27 11:54:24]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-11-27 11:54:24]

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

[João Eiras, 2014-11-27 11:54:27]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-11-27 11:54:45]

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

[commit-bot: I haz the power, 2014-11-27 13:55:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-27 13:55:48]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790)

[kouhei (in TOK), 2014-11-28 02:10:45]

On 2014/11/27 13:55:48, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790)

https://codereview.chromium.org/764963002/ should address
inspector/sources/debugger/ failures.

[João Eiras, 2014-12-01 12:54:50]

On 2014/11/28 02:10:45, kouhei wrote:
> On 2014/11/27 13:55:48, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   win_blink_rel on tryserver.blink
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790)
> 
> https://codereview.chromium.org/764963002/ should address
> inspector/sources/debugger/ failures.

Hopefully :) I was debugging the problems last friday but didn't get too far.
Lets see how it goes now with a rebase.

[João Eiras, 2014-12-01 12:54:51]

On 2014/11/28 02:10:45, kouhei wrote:
> On 2014/11/27 13:55:48, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   win_blink_rel on tryserver.blink
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790)
> 
> https://codereview.chromium.org/764963002/ should address
> inspector/sources/debugger/ failures.

Hopefully :) I was debugging the problems last friday but didn't get too far.
Lets see how it goes now with a rebase.

[João Eiras, 2014-12-01 12:55:01]

The CQ bit was checked by joaoe@opera.com

[João Eiras, 2014-12-01 12:55:02]

The CQ bit was unchecked by joaoe@opera.com

[João Eiras, 2014-12-01 18:14:54]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-12-01 18:15:41]

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

[commit-bot: I haz the power, 2014-12-01 19:21:08]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186256