Let notifyScriptLoadError() handle already detached ScriptLoaders.

Let notifyScriptLoadError() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012
Committed: https://crrev.com/e7bf58190483dffac8e78506884170720165b198
Cr-Commit-Position: refs/heads/master@{#371772}

[sof, 2016-01-27 08:37:00]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org

[sof, 2016-01-27 08:37:01]

please take a look.

ps#1 verifies build sanity; will add suitable #if !ENABLE(OILPAN)s once done.

[haraken, 2016-01-27 08:46:46]

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

https://codereview.chromium.org/1644483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptRunner.cpp:208: foundLoader =
foundLoader || (scriptLoader->isDetached() && m_pendingAsyncScripts.isEmpty());

I understand the part of 'scriptLoader->isDetached()', but why do you need the
part of '&& m_pendingAsyncScripts.isEmpty()'?

https://codereview.chromium.org/1644483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptRunner.cpp:209:
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(foundLoader);

Nit: I'd write RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(foundLoader || ...)
here.

[sof, 2016-01-27 09:10:12]

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

https://codereview.chromium.org/1644483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptRunner.cpp:208: foundLoader =
foundLoader || (scriptLoader->isDetached() && m_pendingAsyncScripts.isEmpty());
On 2016/01/27 08:46:46, haraken wrote:
> 
> I understand the part of 'scriptLoader->isDetached()', but why do you need the
> part of '&& m_pendingAsyncScripts.isEmpty()'?

It is an indication of the ScriptRunner being in a disposed state, without
tracking that with a separate flag. i.e., I want to know the reason why the
ScriptLoader is detached (by the dispose() having happened), otherwise I don't
understand the reasons for how that can come to be.

https://codereview.chromium.org/1644483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptRunner.cpp:209:
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(foundLoader);
On 2016/01/27 08:46:46, haraken wrote:
> 
> Nit: I'd write RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(foundLoader || ...)
> here.

Sorry about the slight confusion - the previous line will be !ENABLE(OILPAN),
see ps#2, which is why it looks a bit odd.

[haraken, 2016-01-27 09:13:07]

LGTM

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

https://codereview.chromium.org/1644483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptRunner.cpp:208: foundLoader =
foundLoader || (scriptLoader->isDetached() && m_pendingAsyncScripts.isEmpty());
On 2016/01/27 09:10:11, sof wrote:
> On 2016/01/27 08:46:46, haraken wrote:
> > 
> > I understand the part of 'scriptLoader->isDetached()', but why do you need
the
> > part of '&& m_pendingAsyncScripts.isEmpty()'?
> 
> It is an indication of the ScriptRunner being in a disposed state, without
> tracking that with a separate flag. i.e., I want to know the reason why the
> ScriptLoader is detached (by the dispose() having happened), otherwise I don't
> understand the reasons for how that can come to be.
> 

Thanks, maybe worth adding a comment.

[sof, 2016-01-27 09:24:19]

On 2016/01/27 09:13:07, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1644483002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/ScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/1644483002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/ScriptRunner.cpp:208: foundLoader =
> foundLoader || (scriptLoader->isDetached() &&
m_pendingAsyncScripts.isEmpty());
> On 2016/01/27 09:10:11, sof wrote:
> > On 2016/01/27 08:46:46, haraken wrote:
> > > 
> > > I understand the part of 'scriptLoader->isDetached()', but why do you need
> the
> > > part of '&& m_pendingAsyncScripts.isEmpty()'?
> > 
> > It is an indication of the ScriptRunner being in a disposed state, without
> > tracking that with a separate flag. i.e., I want to know the reason why the
> > ScriptLoader is detached (by the dispose() having happened), otherwise I
don't
> > understand the reasons for how that can come to be.
> > 
> 
> Thanks, maybe worth adding a comment.

Certainly, done.

(While not a major problem here, that we now have no !ENABLE(OILPAN) trybot
coverage makes for more difficult bug handling wrt earlier branches.)

[sof, 2016-01-27 09:24:46]

Description was changed from

==========
Let notifyScriptLoader() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=
BUG=570012
==========

to

==========
Let notifyScriptLoader() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012
==========

[sof, 2016-01-27 09:25:00]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-01-27 09:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1644483002/#ps40001
(title: "add explanatory comment")

[commit-bot: I haz the power, 2016-01-27 09:26:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644483002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644483002/40001

[commit-bot: I haz the power, 2016-01-27 11:00:07]

Description was changed from

==========
Let notifyScriptLoader() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012
==========

to

==========
Let notifyScriptLoader() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012
==========

[commit-bot: I haz the power, 2016-01-27 11:00:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-27 11:01:07]

Description was changed from

==========
Let notifyScriptLoader() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012
==========

to

==========
Let notifyScriptLoader() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012

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

[commit-bot: I haz the power, 2016-01-27 11:01:08]

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

[sof, 2016-01-27 11:03:16]

Description was changed from

==========
Let notifyScriptLoader() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012

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

to

==========
Let notifyScriptLoadError() handle already detached ScriptLoaders.

If a ScriptRunner has been disposed of already, allow ScriptLoaders
to notify of their failure without asserting.

R=haraken
BUG=570012

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

[sof, 2016-01-28 09:44:32]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1640263004/ by sigbjornf@opera.com.

The reason for reverting is: The change here assumes PendingScripts are
separately allocated objects, an M50 change. This makes for more difficult
backporting. Unnecessarily so.

Hence reverting and will reland a variation (
https://codereview.chromium.org/1642863002/ ) that works for older branches..