Abort and reschedule if script is not ready during execution time

Abort and reschedule if script is not ready during execution time

If revalidation occurs after the script is scheduled for execution,
the script may not be ready anymore when it will be executed.
When this happens, abort execution and reschedule when the script is
ready again.

BUG=716439

[landell, 2017-04-28 12:10:07]

Description was changed from

==========
Abort and reschedule if script is not ready during execution time

If revalidation occurs after the script is scheduled for execution,
the script may not be ready anymore when it will be executed.
When this happens, abort execution and reschedule when the script is
ready again.

BUG=716439
==========

to

==========
Abort and reschedule if script is not ready during execution time

If revalidation occurs after the script is scheduled for execution,
the script may not be ready anymore when it will be executed.
When this happens, abort execution and reschedule when the script is
ready again.

BUG=716439
==========

[landell, 2017-04-28 12:10:07]

landell@opera.com changed reviewers:
+ japhet@chromium.org

[landell, 2017-04-28 12:12:24]

@reviewers:
This is a very tentative fix that I kind of assume is too simple to solve this
problem correctly.

PTAL and see if I am on the right track here.

[Nate Chapin, 2017-04-28 20:29:02]

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

[Nate Chapin, 2017-04-28 20:29:03]

+hiroshige, who knows ScriptLoader better than I do at this point.

Might it be possible to write a unit test that reliably causes this timing
issue? core/dom/ScriptRunnerTest.cpp has some tests at the right layer, but
they're not as invasive as might be needed to get this to crash.

[hiroshige, 2017-04-28 21:01:49]

(The issue is a dup of Issue 692856)

Unfortunately, re-doing WatchForLoad() (or something similar) in Execute() (or
somewhere around GetSource()) is not a good solution for this issue because it
breaks the execution order of scripts.
For example, for the HTML:
<script defer src="a.js"></script>
<script defer src="b.js"></script>
a.js must be executed before b.js, but if a.js is pushed into pending state
again in Execute(), then b.js is executed first.

I think a better direction is to avoid IsReady() from becoming false once after
IsReady() becomes true and PendingScriptClient is notified.

My draft CL is https://codereview.chromium.org/2724673002/.
This saves all necessary data around notifyFinished() (where revalidation is
never triggered yet) and makes GetSource() to use the saved data without
referencing to Resource.
In this way, GetSource() and ClassicPendingScript is not affected by
revalidation.
Probably this is a good time for me to revisit my draft CL.
(I have been inactive for this issue because I was busy for another issue and I
thought it was not urgent because according to the experiments on canary this
DCHECK() failure doesn't affect the observable behavior).

Also, jbroman@ is planning to clean up the state transition of PendingScript and
as a result making IsReady() to be true after it becomes true.
(jbroman@'s draft CL: https://codereview.chromium.org/2828193003/)

(Also, this CL doesn't work because, in the case of
finish-then-revalidation-start case, the (Classic)PendingScript is already
notified of finish by ScriptResource, and is never notified again.)

[landell, 2017-05-02 07:47:59]

On 2017/04/28 21:01:49, hiroshige wrote:
> (The issue is a dup of Issue 692856)
> 
> Unfortunately, re-doing WatchForLoad() (or something similar) in Execute() (or
> somewhere around GetSource()) is not a good solution for this issue because it
> breaks the execution order of scripts.
> For example, for the HTML:
> <script defer src="a.js"></script>
> <script defer src="b.js"></script>
> a.js must be executed before b.js, but if a.js is pushed into pending state
> again in Execute(), then b.js is executed first.
> 
> I think a better direction is to avoid IsReady() from becoming false once
after
> IsReady() becomes true and PendingScriptClient is notified.
> 
> My draft CL is https://codereview.chromium.org/2724673002/.
> This saves all necessary data around notifyFinished() (where revalidation is
> never triggered yet) and makes GetSource() to use the saved data without
> referencing to Resource.
> In this way, GetSource() and ClassicPendingScript is not affected by
> revalidation.
> Probably this is a good time for me to revisit my draft CL.
> (I have been inactive for this issue because I was busy for another issue and
I
> thought it was not urgent because according to the experiments on canary this
> DCHECK() failure doesn't affect the observable behavior).
> 
> Also, jbroman@ is planning to clean up the state transition of PendingScript
and
> as a result making IsReady() to be true after it becomes true.
> (jbroman@'s draft CL: https://codereview.chromium.org/2828193003/)
> 
> (Also, this CL doesn't work because, in the case of
> finish-then-revalidation-start case, the (Classic)PendingScript is already
> notified of finish by ScriptResource, and is never notified again.)

Thanks for the input. I expected this would need a more elaborate solution. I am
dropping this and let you handle it.