Clear pointers to PendingScript when dispose()d in pendingScriptFinished()

Clear pointers to PendingScript when dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer to it is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|
    (no test case found so far).

This CL fixes these cases by clearing |m_parserBlockingScript| or
removing |m_scriptsToExecuteAfterParsing.first()|.

This CL also adds CHECK(false) for (2), which should be removed shortly,
hoping clusterfuzz find a test case for (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
Review-Url: https://codereview.chromium.org/2720683003
Cr-Commit-Position: refs/heads/master@{#453564}
Committed: https://chromium.googlesource.com/chromium/src/+/a08766e7f8ec19cc09341d18b91107377ccfbcd6

[hiroshige, 2017-02-27 22:41:11]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[hiroshige, 2017-02-27 22:42:02]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that is
already dispose()d later, causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
the fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

BUG=
==========

to

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that is
already dispose()d later, causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
the fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

BUG=695730
==========

[hiroshige, 2017-02-27 22:42:02]

hiroshige@chromium.org changed reviewers:
+ kouhei@chromium.org, sigbjornf@opera.com

[commit-bot: I haz the power, 2017-02-27 22:42:12]

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

[hiroshige, 2017-02-27 22:42:31]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that is
already dispose()d later, causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
the fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

BUG=695730
==========

to

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that is
already dispose()d later, causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
the fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

BUG=695730, 686281
==========

[hiroshige, 2017-02-27 22:45:33]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that is
already dispose()d later, causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
the fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

BUG=695730, 686281
==========

to

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that
is already dispose()d later, causing CHECK() failure in
PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
the fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 686281
==========

[hiroshige, 2017-02-27 22:53:06]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that
is already dispose()d later, causing CHECK() failure in
PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
the fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 686281
==========

to

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that
is already dispose()d later, causing CHECK() failure in
PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
a complete fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 686281
==========

[hiroshige, 2017-02-27 22:53:06]

hiroshige@chromium.org changed reviewers:
+ jbroman@chromium.org

[hiroshige, 2017-02-27 23:10:11]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that
is already dispose()d later, causing CHECK() failure in
PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
a complete fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 686281
==========

to

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that
is already dispose()d later, causing CHECK() failure in
PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
a complete fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

[hiroshige, 2017-02-28 00:24:58]

PTAL.

[hiroshige, 2017-02-28 00:28:29]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the corresponding
pointer is not cleared and thus causing the use of PendingScript that
is already dispose()d later, causing CHECK() failure in
PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) otherwise.

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK().
This is to fix (2) separately, if needed, because
a complete fix for (2) will be not so trivial compared to this CL, and
so far clusterfuzz and I haven't found actual test cases exposing (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

to

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) otherwise (no test case found so far).

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK(), hoping clusterfuzz find a
test case for (2).
A following CL will fix the case of (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

[commit-bot: I haz the power, 2017-02-28 00:36:31]

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

[commit-bot: I haz the power, 2017-02-28 00:36:31]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-02-28 00:39:26]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 00:40:03]

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

[hiroshige, 2017-02-28 00:41:36]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) otherwise (no test case found so far).

This CL fixes the case of (1) by clearing |m_parserBlockingScript| and
make the case of (2) to crash by CHECK(), hoping clusterfuzz find a
test case for (2).
A following CL will fix the case of (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

to

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|
    (no test case found so far).

This CL fixes these cases by clearing |m_parserBlockingScript| or
removing |m_scriptsToExecuteAfterParsing.first()|.

This CL also adds CHECK(false) for (2), which should be removed shortly,
hoping clusterfuzz find a test case for (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

[kouhei (in TOK), 2017-02-28 00:41:48]

lgtm

https://codereview.chromium.org/2720683003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
(right):

https://codereview.chromium.org/2720683003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:370: //
|m_parserBlockingScript == m_scriptsToExecuteAfterParsing.first()|.
I'm not sure if we should do that per se. I'd rather rm member
m_parserBlockingScript and unify to one queue (possibly
m_scriptsToExecuteAfterParsing)

[kouhei (in TOK), 2017-02-28 00:43:36]

still lgtm

[hiroshige, 2017-02-28 00:44:37]

Sorry, I just uploaded Patch Set 2 because I thought fixing (1) and (2) in this
CL and removing CHECK() for clusterfuzz might be clearer. WDYT?

[hiroshige, 2017-02-28 00:48:55]

https://codereview.chromium.org/2720683003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
(right):

https://codereview.chromium.org/2720683003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:370: //
|m_parserBlockingScript == m_scriptsToExecuteAfterParsing.first()|.
On 2017/02/28 00:41:48, kouhei wrote:
> I'm not sure if we should do that per se. I'd rather rm member
> m_parserBlockingScript and unify to one queue (possibly
> m_scriptsToExecuteAfterParsing)

|m_parserBlockingScript| and |m_scriptsToExecuteAfterParsing| has separate
concepts in the spec, as commented in HTMLParserScriptRunner.h, so having these
two separately makes the code directly corresponds to the spec.

(It might be nice if we can unify these two also in the spec. I feel the
assumptions/invariants about these two are not so clear in impl or spec)

[hiroshige, 2017-02-28 00:56:58]

Description was changed from

==========
Clear |m_parserBlockingScript| when it is dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|
    (no test case found so far).

This CL fixes these cases by clearing |m_parserBlockingScript| or
removing |m_scriptsToExecuteAfterParsing.first()|.

This CL also adds CHECK(false) for (2), which should be removed shortly,
hoping clusterfuzz find a test case for (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

to

==========
Clear pointers to PendingScript when dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer to it is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|
    (no test case found so far).

This CL fixes these cases by clearing |m_parserBlockingScript| or
removing |m_scriptsToExecuteAfterParsing.first()|.

This CL also adds CHECK(false) for (2), which should be removed shortly,
hoping clusterfuzz find a test case for (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

[kouhei (in TOK), 2017-02-28 00:59:40]

lgtm

On 2017/02/28 00:48:55, hiroshige wrote:
>
https://codereview.chromium.org/2720683003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
> (right):
> 
>
https://codereview.chromium.org/2720683003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:370: //
> |m_parserBlockingScript == m_scriptsToExecuteAfterParsing.first()|.
> On 2017/02/28 00:41:48, kouhei wrote:
> > I'm not sure if we should do that per se. I'd rather rm member
> > m_parserBlockingScript and unify to one queue (possibly
> > m_scriptsToExecuteAfterParsing)
> 
> |m_parserBlockingScript| and |m_scriptsToExecuteAfterParsing| has separate
> concepts in the spec, as commented in HTMLParserScriptRunner.h, so having
these
> two separately makes the code directly corresponds to the spec.
> 
> (It might be nice if we can unify these two also in the spec. I feel the
> assumptions/invariants about these two are not so clear in impl or spec)
Thanks for clarification

[commit-bot: I haz the power, 2017-02-28 02:57:14]

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

[commit-bot: I haz the power, 2017-02-28 02:57:15]

Dry run: This issue passed the CQ dry run.

[sof, 2017-02-28 07:13:35]

lgtm

[hiroshige, 2017-02-28 10:23:21]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-02-28 10:23:34]

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

[commit-bot: I haz the power, 2017-02-28 10:28:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488277401506480,
"parent_rev": "584b59ac597f611fbac882fbbe90d538bf60db4e", "commit_rev":
"a08766e7f8ec19cc09341d18b91107377ccfbcd6"}

[commit-bot: I haz the power, 2017-02-28 10:28:45]

Description was changed from

==========
Clear pointers to PendingScript when dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer to it is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|
    (no test case found so far).

This CL fixes these cases by clearing |m_parserBlockingScript| or
removing |m_scriptsToExecuteAfterParsing.first()|.

This CL also adds CHECK(false) for (2), which should be removed shortly,
hoping clusterfuzz find a test case for (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281
==========

to

==========
Clear pointers to PendingScript when dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer to it is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at Issue 695730), or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|
    (no test case found so far).

This CL fixes these cases by clearing |m_parserBlockingScript| or
removing |m_scriptsToExecuteAfterParsing.first()|.

This CL also adds CHECK(false) for (2), which should be removed shortly,
hoping clusterfuzz find a test case for (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG=695730, 696775, 686281

Review-Url: https://codereview.chromium.org/2720683003
Cr-Commit-Position: refs/heads/master@{#453564}
Committed:
https://chromium.googlesource.com/chromium/src/+/a08766e7f8ec19cc09341d18b911...
==========

[commit-bot: I haz the power, 2017-02-28 10:28:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a08766e7f8ec19cc09341d18b911...