Check that PendingScript::m_streamer is always null when resource() is null

Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption, and now these methods are clearly splitted
into non-inline and inline script cases.
This CL also adds other assertions to hold and more checkState() calls.

BUG=686281
Review-Url: https://codereview.chromium.org/2706243006
Cr-Commit-Position: refs/heads/master@{#452515}
Committed: https://chromium.googlesource.com/chromium/src/+/edef4982396f33e6038ba37df5f200921bc63f79

[hiroshige, 2017-02-23 01:40:34]

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

[commit-bot: I haz the power, 2017-02-23 01:41:21]

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-23 01:42:36]

Description was changed from

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be non-null
even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

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

to

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption.

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

[hiroshige, 2017-02-23 01:42:36]

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

[hiroshige, 2017-02-23 01:45:13]

Description was changed from

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption.

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

to

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption, and now these methods are clearly splitted
into non-inline and inline script cases.
This CL also adds other assertions to hold and more checkState() calls.

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

[hiroshige, 2017-02-23 01:45:55]

Description was changed from

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption, and now these methods are clearly splitted
into non-inline and inline script cases.
This CL also adds other assertions to hold and more checkState() calls.

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

to

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption, and now these methods are clearly splitted
into non-inline and inline script cases.
This CL also adds other assertions to hold and more checkState() calls.

BUG=686281
==========

[hiroshige, 2017-02-23 01:46:39]

PTAL.

[kouhei (in TOK), 2017-02-23 01:54:18]

lgtm

https://codereview.chromium.org/2706243006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/2706243006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/PendingScript.cpp:76: checkState();
Doesn't have to be this CL, but calling dispose() on dispose()d PendingScript
seems wrong...

[hiroshige, 2017-02-23 01:58:51]

https://codereview.chromium.org/2706243006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/2706243006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/PendingScript.cpp:76: checkState();
On 2017/02/23 01:54:17, kouhei wrote:
> Doesn't have to be this CL, but calling dispose() on dispose()d PendingScript
> seems wrong...

dispose() is the prefinalizer of PendingScript, so it doesn't necessarily wrong
for me that the prefinalizer is called after someone explicitly calls dispose()
before that.

[commit-bot: I haz the power, 2017-02-23 03:24:15]

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

[commit-bot: I haz the power, 2017-02-23 03:24:15]

Dry run: This issue passed the CQ dry run.

[sof, 2017-02-23 07:26:23]

lgtm

https://codereview.chromium.org/2706243006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/2706243006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/PendingScript.cpp:76: checkState();
On 2017/02/23 01:58:51, hiroshige wrote:
> On 2017/02/23 01:54:17, kouhei wrote:
> > Doesn't have to be this CL, but calling dispose() on dispose()d
PendingScript
> > seems wrong...
> 
> dispose() is the prefinalizer of PendingScript, so it doesn't necessarily
wrong
> for me that the prefinalizer is called after someone explicitly calls
dispose()
> before that.

Yes, promptly disposing of this object is worthwhile to release resources a
little bit earlier. The lifetime end we know most of the time for PendingScript,
but not always, hence rely on prefinalizers for the rest.

We don't support unregistering prefinalizers, and I don't think we should/need
to here.

https://codereview.chromium.org/2706243006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/2706243006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.cpp:70: CHECK(m_isForTesting ||
m_element);
This method would be a good place to elucidate the assumed invariants. At the
moment, we have a number of mutually exclusive uses of PendingScript.
(Entertaining the thought of using classes to make that division clearer, is
perhaps worthwhile.)

[hiroshige, 2017-02-23 16:27:08]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-02-23 16:27:37]

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-23 16:33:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487867228834050,
"parent_rev": "a75c1a1abf9c5b450eab836966a246c368e49990", "commit_rev":
"edef4982396f33e6038ba37df5f200921bc63f79"}

[commit-bot: I haz the power, 2017-02-23 16:33:38]

Description was changed from

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption, and now these methods are clearly splitted
into non-inline and inline script cases.
This CL also adds other assertions to hold and more checkState() calls.

BUG=686281
==========

to

==========
Check that PendingScript::m_streamer is always null when resource() is null

PendingScript::errorOccurred() is written as if |m_streamer| can be
non-null even when resource() is null.

However, |m_streamer| can be (and should be) set to non-null only if
resource() is non-null, and resource() is not cleared unless
PendingScript::dispose()d.

This CL adds checkState() that asserts |m_streamer| is never non-null
when resource() is null, and
rewrites PendingScript::errorOccurred() and PendingScript::isReady()
based on this assumption, and now these methods are clearly splitted
into non-inline and inline script cases.
This CL also adds other assertions to hold and more checkState() calls.

BUG=686281

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

[commit-bot: I haz the power, 2017-02-23 16:33:39]

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