Handle parser-blocking script cancellations while nested.

Handle parser-blocking script cancellations while nested.

Gracefully handle cancellation of a parser-blocking script load that
was initiated while already executing scripts. That is, quietly stop
waiting for it to load without notifying the host.

R=marja@chromium.org,kouhei@chromium.org,haraken@chromium.org
BUG=398090
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185122

[sof, 2014-11-10 11:46:26]

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

[sof, 2014-11-10 11:46:26]

Please take a look.

[marja, 2014-11-10 12:02:50]

I wonder how this plays with --enable-v8-script-streaming... maybe it works out
of the box?

(I'll have a closer look at this soon.)

[sof, 2014-11-10 12:17:40]

On 2014/11/10 12:02:50, marja wrote:
> I wonder how this plays with --enable-v8-script-streaming... maybe it works
out
> of the box?
> 
> (I'll have a closer look at this soon.)

To aid answering that Q -- for the test added, the behavior before & after is
the same with script streaming enabled.

[marja, 2014-11-10 13:32:10]

One thing that confuses me is that why HTMLScriptRunner::detach is not called
before the script loading finishes...

[sof, 2014-11-10 14:53:46]

On 2014/11/10 13:32:10, marja wrote:
> One thing that confuses me is that why HTMLScriptRunner::detach is not called
> before the script loading finishes...

That global cleanup step of pending scripts should only be needed during
finalization.

But, generalizing the PendingScript watch cancellation done by notifyFinished()
makes sense - now done. Does that address your concern?

[marja, 2014-11-10 15:36:05]

On 2014/11/10 14:53:46, sof wrote:
> On 2014/11/10 13:32:10, marja wrote:
> > One thing that confuses me is that why HTMLScriptRunner::detach is not
called
> > before the script loading finishes...
> 
> That global cleanup step of pending scripts should only be needed during
> finalization.
> 
> But, generalizing the PendingScript watch cancellation done by
notifyFinished()
> makes sense - now done. Does that address your concern?

Hmm, no, I meant, when window.close() is called (in your test case), and the
script loading is cancelled, why is HTMLScriptRunner::detach() not called at
that point?

(However, I'm generally convinced by this CL, I'm just confused by the
surrounding code, but it's not this CL's fault. If somebody wants to go ahead
and review, feel free to.)

[marja, 2014-11-11 09:41:03]

(But I do think that calling both stopWatchingForLoad and releaseAndClearElement
is a good idea; it's cleaner.)

[marja, 2014-11-11 09:49:05]

lgtm; I'm convinced that this works (can't really disagree with the test), and I
don't want to block this work because of my own confusion :)

[sof, 2014-11-11 09:57:46]

On 2014/11/11 09:49:05, marja wrote:
> lgtm; I'm convinced that this works (can't really disagree with the test), and
I
> don't want to block this work because of my own confusion :)

Thanks for carefully going over the details, helpful; my understanding of all
script runner mechanics is a WIP also.

[kouhei (in TOK), 2014-11-11 10:02:51]

lgtm 2

[haraken, 2014-11-11 10:23:39]

LGTM

[sof, 2014-11-11 10:38:24]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-11-11 10:39:18]

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

[commit-bot: I haz the power, 2014-11-11 12:14:56]

Committed patchset #3 (id:40001) as 185122

[Noel Gordon, 2014-11-11 13:26:48]

message-port-close.html started failing after this change:
 
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9/builds/...

related?

[Noel Gordon, 2014-11-11 13:28:04]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[sof, 2014-11-11 13:34:12]

On 2014/11/11 13:28:05, noel gordon wrote:

[marja, 2014-11-11 13:34:38]

Sounds surprising, since that test shouldn't go via this code path, but who
knows. When in doubt, feel free to tentatively revert.

[sof, 2014-11-11 13:35:20]

On 2014/11/11 13:26:48, noel gordon wrote:
> message-port-close.html started failing after this change:
>  
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9/builds/...
> 
> related?

Not clear, seems like it started a bit earlier?

 
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/22675
 
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=Ch...

[Noel Gordon, 2014-11-11 13:42:55]

Not clear to me either.  Some change in behavior around here ...

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...

which the gardeners will need to clean up.

[sof, 2014-11-11 13:52:28]

On 2014/11/11 13:42:55, noel gordon wrote:
> Not clear to me either.  Some change in behavior around here ...
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...
> 
> which the gardeners will need to clean up.

Yes, looking into it (modulo slow local building of ToT.)

[sof, 2014-11-11 14:23:28]

On 2014/11/11 13:52:28, sof wrote:
> On 2014/11/11 13:42:55, noel gordon wrote:
> > Not clear to me either.  Some change in behavior around here ...
> > 
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...
> > 
> > which the gardeners will need to clean up.
> 
> Yes, looking into it (modulo slow local building of ToT.)

Due to https://codereview.chromium.org/704933002