When pumping tokens yield for scripts waiting for resources.

When pumping tokens yield for scripts waiting for resources.

This fixes a null-deref which you can see if you open
third_party/WebKit/LayoutTests/fast/parser/resources/ (note:
resources) write-script-waiting-for-style-crash.html standalone.
HTMLDocumentParser::canTakeNextToken was assuming that only the tree
builder can block the parser; now it checks that.

BUG=573921
Committed: https://crrev.com/e75f7662727fbbd08cc730c41b3c63da7d61c36c
Cr-Commit-Position: refs/heads/master@{#402101}

[dominicc (has gone to gerrit), 2016-06-24 02:23:00]

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

[dominicc (has gone to gerrit), 2016-06-24 02:32:04]

PTAL

As you predicted, it looks like this is a callsite that wasn't updated for
scripts blocked on resources at some point.

I would still like to refactor the parser so the script-blocking flag matches
the spec more clearly. Having the "has a parser blocking script" state spread
between HTMLDocumentParser, HTMLScriptRunner and HTMLTreeBuilder is a bit
confusing. But I want to poke at the parser more to get more familiar with it.

[dominicc (has gone to gerrit), 2016-06-24 05:53:01]

Hmm, looks like the test needs work in release.

[dominicc (has gone to gerrit), 2016-06-24 05:53:16]

dominicc@chromium.org changed reviewers:
- kouhei@chromium.org

[dominicc (has gone to gerrit), 2016-06-24 06:43:36]

On 2016/06/24 at 05:53:01, dominicc wrote:
> Hmm, looks like the test needs work in release.

Hmm, looks like this is still crashing in release :) must be something more to
this I don't understand yet.

[dominicc (has gone to gerrit), 2016-06-24 08:02:02]

On 2016/06/24 at 06:43:36, dominicc wrote:
> On 2016/06/24 at 05:53:01, dominicc wrote:
> > Hmm, looks like the test needs work in release.
> 
> Hmm, looks like this is still crashing in release :) must be something more to
this I don't understand yet.

My mistake; that was an old build I had lying around. The release failures are
missing pixel expectations or something. I've made this text-based.

[dominicc (has gone to gerrit), 2016-06-24 08:02:10]

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

[dominicc (has gone to gerrit), 2016-06-24 08:02:55]

PTAL

[kouhei (in TOK), 2016-06-24 09:02:55]

Code change lgtm

https://codereview.chromium.org/2099583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/parser/resources/write-script-waiting-for-style-crash.html
(right):

https://codereview.chromium.org/2099583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/parser/resources/write-script-waiting-for-style-crash.html:11:
<iframe></iframe>
Please format.

Would it be possible to create a test case that directly triggers the issue?
If that is difficult, please add a reference to the crbug.

[dominicc (has gone to gerrit), 2016-06-27 01:54:20]

On 2016/06/24 at 09:02:55, kouhei wrote:
> Would it be possible to create a test case that directly triggers the issue?
> If that is difficult, please add a reference to the crbug.

I poked at this in a C++ unit test but I think this will need an event loop and
a loader so the didLoadAllBlockingScriptResources can happen. I'm interested in
unit testing HTML more but there might be more value in decoupling things and
writing small tests?

[dominicc (has gone to gerrit), 2016-06-27 01:54:24]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-06-27 01:54:25]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2099583002/#ps40001
(title: "Format the test files and add a reference to the bug.")

[commit-bot: I haz the power, 2016-06-27 01:54:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kouhei (in TOK), 2016-06-27 02:06:37]

still lgtm

On 2016/06/27 01:54:20, dominicc wrote:
> On 2016/06/24 at 09:02:55, kouhei wrote:
> > Would it be possible to create a test case that directly triggers the issue?
> > If that is difficult, please add a reference to the crbug.
> 
> I poked at this in a C++ unit test but I think this will need an event loop
and
> a loader so the didLoadAllBlockingScriptResources can happen. I'm interested
in
> unit testing HTML more but there might be more value in decoupling things and
> writing small tests?

The problems with having fuzz generated cases in layout test suite are that they
tend to be flaky and rely on the Blink design at the time. (In this case, how
style loader works)
However, when a concise test is difficult, having some test is better than no
test...

[commit-bot: I haz the power, 2016-06-27 03:03:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-27 03:04:53]

Description was changed from

==========
When pumping tokens yield for scripts waiting for resources.

This fixes a null-deref which you can see if you open
third_party/WebKit/LayoutTests/fast/parser/resources/ (note:
resources) write-script-waiting-for-style-crash.html standalone.
HTMLDocumentParser::canTakeNextToken was assuming that only the tree
builder can block the parser; now it checks that.

BUG=573921
==========

to

==========
When pumping tokens yield for scripts waiting for resources.

This fixes a null-deref which you can see if you open
third_party/WebKit/LayoutTests/fast/parser/resources/ (note:
resources) write-script-waiting-for-style-crash.html standalone.
HTMLDocumentParser::canTakeNextToken was assuming that only the tree
builder can block the parser; now it checks that.

BUG=573921

Committed: https://crrev.com/e75f7662727fbbd08cc730c41b3c63da7d61c36c
Cr-Commit-Position: refs/heads/master@{#402101}
==========

[commit-bot: I haz the power, 2016-06-27 03:04:54]

Patchset 3 (id:??) landed as
https://crrev.com/e75f7662727fbbd08cc730c41b3c63da7d61c36c
Cr-Commit-Position: refs/heads/master@{#402101}