Keep the wrapper of FetchEvent alive while waiting for the preload response.

Keep the wrapper of FetchEvent alive while waiting for the preload response.

This CL uses ActiveScriptWrappable and implements HasPendingActivity() to keep the wrapper of FetchEvent alive while waiting for the preload response.

FetchEvents are already kept alive by ServiceWorkerGlobalScopeProxy's |pending_preload_fetch_events_| until the preload response.
But the resolver function is not called in the current implementation if GC happens. (https://crbug.com/728013#c28)
That is because if the wrapper of FetchEvent are GCed, ScriptPromiseProperty can't call the resolver.

BUG=728013
Review-Url: https://codereview.chromium.org/2922863002
Cr-Commit-Position: refs/heads/master@{#476977}
Committed: https://chromium.googlesource.com/chromium/src/+/7f1ac9b18fed816431798435d68d8ee90668d392

[horo, 2017-06-04 04:44:06]

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

[commit-bot: I haz the power, 2017-06-04 04:44:13]

Dry run: CQ is trying da patch.

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

[horo, 2017-06-04 05:08:08]

Description was changed from

==========
Keep FetchEvent alive while waiting for the preload response by using
ActiveScriptWrappable.

This CL uses ActiveScriptWrappable and implements HasPendingActivity() to keep
FetchEvent alive while waiting for the preload response.

I think FetchEvents are already kept alive by ServiceWorkerGlobalScopeProxy's
|pending_preload_fetch_events_| until the preload response.
But I don't know why, the resolver function is GCed in the current
implementation. (https://crbug.com/728013#c28)
And the function is not GCed with this CL.

BUG=728013
==========

to

==========
Keep the wrapper of FetchEvent alive while waiting for the preload response.

This CL uses ActiveScriptWrappable and implements HasPendingActivity() to keep
the wrapper of FetchEvent alive while waiting for the preload response.

FetchEvents are already kept alive by ServiceWorkerGlobalScopeProxy's
|pending_preload_fetch_events_| until the preload response.
But the resolver function is not called in the current implementation if GC
happens. (https://crbug.com/728013#c28)
That is because if the wrapper of FetchEvent are GCed, ScriptPromiseProperty
can't call the resolver.

BUG=728013
==========

[horo, 2017-06-04 06:06:23]

horo@chromium.org changed reviewers:
+ yhirano@chromium.org

[horo, 2017-06-04 06:06:24]

yhirano@
Could you please review this?

[commit-bot: I haz the power, 2017-06-04 07:19:14]

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

[commit-bot: I haz the power, 2017-06-04 07:19:15]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[horo, 2017-06-04 10:16:15]

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

[commit-bot: I haz the power, 2017-06-04 10:16:25]

Dry run: 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-06-04 11:46:38]

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

[commit-bot: I haz the power, 2017-06-04 11:46:39]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-06-05 01:58:15]

On 2017/06/04 06:06:24, horo wrote:
> yhirano@
> Could you please review this?

Why does FetchEvent need to be a ContextClient?

[haraken, 2017-06-05 02:02:53]

On 2017/06/05 01:58:15, yhirano wrote:
> On 2017/06/04 06:06:24, horo wrote:
> > yhirano@
> > Could you please review this?
> 
> Why does FetchEvent need to be a ContextClient?

Because ActiveScriptWrappable needs to use IsContextDestroyed().

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/bindi...

Yes, ideally we should make ActiveScriptWrappable inherit from ContextClient
instead of asking each client to inherit from ContextClient...

LGTM.

[yhirano, 2017-06-05 02:10:13]

lgtm

https://codereview.chromium.org/2922863002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right):

https://codereview.chromium.org/2922863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:99: bool
waiting_for_preload_response_;
You can rely on preload_response_propty_->GetState() instead of introducing a
new boolean.

[horo, 2017-06-05 11:11:51]

Thank you.

https://codereview.chromium.org/2922863002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right):

https://codereview.chromium.org/2922863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:99: bool
waiting_for_preload_response_;
On 2017/06/05 02:10:13, yhirano wrote:
> You can rely on preload_response_propty_->GetState() instead of introducing a
> new boolean.

Done.

[horo, 2017-06-05 11:12:03]

The CQ bit was checked by horo@chromium.org

[horo, 2017-06-05 11:12:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2922863002/#ps40001
(title: "incorporated yhirano's comment")

[commit-bot: I haz the power, 2017-06-05 11:12:16]

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-06-05 13:33:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496661123410210,
"parent_rev": "0b90518f33e00c7c45a2c2658dc026088e28edb3", "commit_rev":
"7f1ac9b18fed816431798435d68d8ee90668d392"}

[commit-bot: I haz the power, 2017-06-05 13:33:58]

Description was changed from

==========
Keep the wrapper of FetchEvent alive while waiting for the preload response.

This CL uses ActiveScriptWrappable and implements HasPendingActivity() to keep
the wrapper of FetchEvent alive while waiting for the preload response.

FetchEvents are already kept alive by ServiceWorkerGlobalScopeProxy's
|pending_preload_fetch_events_| until the preload response.
But the resolver function is not called in the current implementation if GC
happens. (https://crbug.com/728013#c28)
That is because if the wrapper of FetchEvent are GCed, ScriptPromiseProperty
can't call the resolver.

BUG=728013
==========

to

==========
Keep the wrapper of FetchEvent alive while waiting for the preload response.

This CL uses ActiveScriptWrappable and implements HasPendingActivity() to keep
the wrapper of FetchEvent alive while waiting for the preload response.

FetchEvents are already kept alive by ServiceWorkerGlobalScopeProxy's
|pending_preload_fetch_events_| until the preload response.
But the resolver function is not called in the current implementation if GC
happens. (https://crbug.com/728013#c28)
That is because if the wrapper of FetchEvent are GCed, ScriptPromiseProperty
can't call the resolver.

BUG=728013

Review-Url: https://codereview.chromium.org/2922863002
Cr-Commit-Position: refs/heads/master@{#476977}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f1ac9b18fed816431798435d68d...
==========

[commit-bot: I haz the power, 2017-06-05 13:33:59]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7f1ac9b18fed816431798435d68d...