[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

1: https://crrev.com/d4b43c621eb323605c258a1a7f238bf9d6fb7523

BUG=614272
Committed: https://crrev.com/a153f873d904d8cdd0dafb9507f7a5b560251426
Cr-Commit-Position: refs/heads/master@{#397625}

[yhirano, 2016-06-01 08:07:54]

Description was changed from

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

BUG=614272
==========

to

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

BUG=614272
==========

[yhirano, 2016-06-01 08:08:10]

Description was changed from

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

BUG=614272
==========

to

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

1: https://crrev.com/d4b43c621eb323605c258a1a7f238bf9d6fb7523

BUG=614272
==========

[yhirano, 2016-06-01 08:10:32]

yhirano@chromium.org changed reviewers:
+ falken@chromium.org, haraken@chromium.org

[haraken, 2016-06-01 12:16:59]

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

https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:72:
v8::Local<v8::Value> event = toV8(this, scriptState);

Given that both objects have wrappers, can we use [SetWrapperReferenceFrom/To]
instead?

https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:80: // Sets an
hidden value in order to teach V8 the dependency from

a hidden value

https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);

yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which is much
faster. Can you try V8PrivateProperty? :)

[yhirano, 2016-06-01 12:22:51]

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

https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);
On 2016/06/01 12:16:59, haraken wrote:
> 
> yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which is much
> faster. Can you try V8PrivateProperty? :)

Does it work with M52? If this CL fixes crashes, I will want to merge it.

[haraken, 2016-06-01 12:42:52]

On 2016/06/01 12:22:51, yhirano wrote:
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right):
> 
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
> V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
> V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);
> On 2016/06/01 12:16:59, haraken wrote:
> > 
> > yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which is much
> > faster. Can you try V8PrivateProperty? :)
> 
> Does it work with M52? If this CL fixes crashes, I will want to merge it.

Ah, then we need to use V8HiddenValue. But I'm wondering we can just use
[SetWrapperReferenceFrom/To].

[yhirano, 2016-06-02 04:29:46]

On 2016/06/01 12:42:52, haraken wrote:
> On 2016/06/01 12:22:51, yhirano wrote:
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
> > V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
> > V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);
> > On 2016/06/01 12:16:59, haraken wrote:
> > > 
> > > yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which is
much
> > > faster. Can you try V8PrivateProperty? :)
> > 
> > Does it work with M52? If this CL fixes crashes, I will want to merge it.
> 
> Ah, then we need to use V8HiddenValue. But I'm wondering we can just use
> [SetWrapperReferenceFrom/To].

Do you mean something like PS4? Unfortunately it's not working on my environment
(i.e., the newly added test doesn't stop crashing). If I use the attribute
wrongly, please let me know.

[haraken, 2016-06-02 04:48:45]

On 2016/06/02 04:29:46, yhirano wrote:
> On 2016/06/01 12:42:52, haraken wrote:
> > On 2016/06/01 12:22:51, yhirano wrote:
> > >
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
> > > V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
> > > V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);
> > > On 2016/06/01 12:16:59, haraken wrote:
> > > > 
> > > > yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which is
> much
> > > > faster. Can you try V8PrivateProperty? :)
> > > 
> > > Does it work with M52? If this CL fixes crashes, I will want to merge it.
> > 
> > Ah, then we need to use V8HiddenValue. But I'm wondering we can just use
> > [SetWrapperReferenceFrom/To].
> 
> Do you mean something like PS4? Unfortunately it's not working on my
environment
> (i.e., the newly added test doesn't stop crashing). If I use the attribute
> wrongly, please let me know.

[SetWrapperReferenceTo] works only if the target wrapper exists. Can you make
sure that the wrapper exists (i.e., create the wrapper in FetchEvent's
constructor)?

(This is a bug but our plan is to remove [SetWrapperReferenceTo] in favor of
traceWrappers...)

[Yuki, 2016-06-02 04:51:11]

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

[Yuki, 2016-06-02 04:51:12]

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

https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);
On 2016/06/01 12:22:51, yhirano wrote:
> On 2016/06/01 12:16:59, haraken wrote:
> > 
> > yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which is much
> > faster. Can you try V8PrivateProperty? :)
> 
> Does it work with M52? If this CL fixes crashes, I will want to merge it.

No, V8PrivateProperty gets supported since M53.  Please keep using V8HiddenValue
for M52.

[yhirano, 2016-06-02 05:02:36]

On 2016/06/02 04:48:45, haraken wrote:
> On 2016/06/02 04:29:46, yhirano wrote:
> > On 2016/06/01 12:42:52, haraken wrote:
> > > On 2016/06/01 12:22:51, yhirano wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
> > > > V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
> > > > V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);
> > > > On 2016/06/01 12:16:59, haraken wrote:
> > > > > 
> > > > > yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which
is
> > much
> > > > > faster. Can you try V8PrivateProperty? :)
> > > > 
> > > > Does it work with M52? If this CL fixes crashes, I will want to merge
it.
> > > 
> > > Ah, then we need to use V8HiddenValue. But I'm wondering we can just use
> > > [SetWrapperReferenceFrom/To].
> > 
> > Do you mean something like PS4? Unfortunately it's not working on my
> environment
> > (i.e., the newly added test doesn't stop crashing). If I use the attribute
> > wrongly, please let me know.
> 
> [SetWrapperReferenceTo] works only if the target wrapper exists. Can you make
> sure that the wrapper exists (i.e., create the wrapper in FetchEvent's
> constructor)?
> 
> (This is a bug but our plan is to remove [SetWrapperReferenceTo] in favor of
> traceWrappers...)

PS5 still doesn't work (PS3 works, FYI)

[haraken, 2016-06-02 05:06:28]

On 2016/06/02 05:02:36, yhirano wrote:
> On 2016/06/02 04:48:45, haraken wrote:
> > On 2016/06/02 04:29:46, yhirano wrote:
> > > On 2016/06/01 12:42:52, haraken wrote:
> > > > On 2016/06/01 12:22:51, yhirano wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > > > > File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
> > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
> > > > > third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:82:
> > > > > V8HiddenValue::setHiddenValue(scriptState, event.As<v8::Object>(),
> > > > > V8HiddenValue::requestInFetchEvent(scriptState->isolate()), request);
> > > > > On 2016/06/01 12:16:59, haraken wrote:
> > > > > > 
> > > > > > yukishiino@ is replacing V8HiddenValue with V8PrivateProperty, which
> is
> > > much
> > > > > > faster. Can you try V8PrivateProperty? :)
> > > > > 
> > > > > Does it work with M52? If this CL fixes crashes, I will want to merge
> it.
> > > > 
> > > > Ah, then we need to use V8HiddenValue. But I'm wondering we can just use
> > > > [SetWrapperReferenceFrom/To].
> > > 
> > > Do you mean something like PS4? Unfortunately it's not working on my
> > environment
> > > (i.e., the newly added test doesn't stop crashing). If I use the attribute
> > > wrongly, please let me know.
> > 
> > [SetWrapperReferenceTo] works only if the target wrapper exists. Can you
make
> > sure that the wrapper exists (i.e., create the wrapper in FetchEvent's
> > constructor)?
> > 
> > (This is a bug but our plan is to remove [SetWrapperReferenceTo] in favor of
> > traceWrappers...)
> 
> PS5 still doesn't work (PS3 works, FYI)

Where are you crashing?

[yhirano, 2016-06-02 05:13:38]

On 2016/06/02 05:06:28, haraken wrote:
> On 2016/06/02 05:02:36, yhirano wrote:
(snip)
> > PS5 still doesn't work (PS3 works, FYI)
> 
> Where are you crashing?

In fetch-request-with-gc.js, we call fetch with |e.request|. But that request
wrapper is gone while |e| is alive. modules/fetch stores some information in
that request wrapper and it leads to a crash.

[haraken, 2016-06-02 05:22:05]

On 2016/06/02 05:13:38, yhirano wrote:
> On 2016/06/02 05:06:28, haraken wrote:
> > On 2016/06/02 05:02:36, yhirano wrote:
> (snip)
> > > PS5 still doesn't work (PS3 works, FYI)
> > 
> > Where are you crashing?
> 
> In fetch-request-with-gc.js, we call fetch with |e.request|. But that request
> wrapper is gone while |e| is alive. modules/fetch stores some information in
> that request wrapper and it leads to a crash.

Can you confirm if DOMWrapperWorld::setWrapperReferencesInAllWorlds is called
for the event's wrapper?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[yhirano, 2016-06-02 05:52:52]

On 2016/06/02 05:22:05, haraken wrote:
> On 2016/06/02 05:13:38, yhirano wrote:
> > On 2016/06/02 05:06:28, haraken wrote:
> > > On 2016/06/02 05:02:36, yhirano wrote:
> > (snip)
> > > > PS5 still doesn't work (PS3 works, FYI)
> > > 
> > > Where are you crashing?
> > 
> > In fetch-request-with-gc.js, we call fetch with |e.request|. But that
request
> > wrapper is gone while |e| is alive. modules/fetch stores some information in
> > that request wrapper and it leads to a crash.
> 
> Can you confirm if DOMWrapperWorld::setWrapperReferencesInAllWorlds is called
> for the event's wrapper?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It's called but ScriptWrappable::containsWrapper returns false (and here's not
the main thread).
It looks setWrapperReferencesInAllWorlds is not working correctly with worker
worlds to me...

[haraken, 2016-06-02 07:47:09]

On 2016/06/02 05:52:52, yhirano wrote:
> On 2016/06/02 05:22:05, haraken wrote:
> > On 2016/06/02 05:13:38, yhirano wrote:
> > > On 2016/06/02 05:06:28, haraken wrote:
> > > > On 2016/06/02 05:02:36, yhirano wrote:
> > > (snip)
> > > > > PS5 still doesn't work (PS3 works, FYI)
> > > > 
> > > > Where are you crashing?
> > > 
> > > In fetch-request-with-gc.js, we call fetch with |e.request|. But that
> request
> > > wrapper is gone while |e| is alive. modules/fetch stores some information
in
> > > that request wrapper and it leads to a crash.
> > 
> > Can you confirm if DOMWrapperWorld::setWrapperReferencesInAllWorlds is
called
> > for the event's wrapper?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> It's called but ScriptWrappable::containsWrapper returns false (and here's not
> the main thread).
> It looks setWrapperReferencesInAllWorlds is not working correctly with worker
> worlds to me...

Thanks for pointing it out... This is a bug of
DOMWrapperWorld::setWrapperReferencesInAllWorlds. We need to trace worker's
world as well.

(I can take a look tomorrow if you're not in a hurry.)

[yhirano, 2016-06-02 08:09:05]

On 2016/06/02 07:47:09, haraken wrote:
> On 2016/06/02 05:52:52, yhirano wrote:
> > On 2016/06/02 05:22:05, haraken wrote:
> > > On 2016/06/02 05:13:38, yhirano wrote:
> > > > On 2016/06/02 05:06:28, haraken wrote:
> > > > > On 2016/06/02 05:02:36, yhirano wrote:
> > > > (snip)
> > > > > > PS5 still doesn't work (PS3 works, FYI)
> > > > > 
> > > > > Where are you crashing?
> > > > 
> > > > In fetch-request-with-gc.js, we call fetch with |e.request|. But that
> > request
> > > > wrapper is gone while |e| is alive. modules/fetch stores some
information
> in
> > > > that request wrapper and it leads to a crash.
> > > 
> > > Can you confirm if DOMWrapperWorld::setWrapperReferencesInAllWorlds is
> called
> > > for the event's wrapper?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > It's called but ScriptWrappable::containsWrapper returns false (and here's
not
> > the main thread).
> > It looks setWrapperReferencesInAllWorlds is not working correctly with
worker
> > worlds to me...
> 
> Thanks for pointing it out... This is a bug of
> DOMWrapperWorld::setWrapperReferencesInAllWorlds. We need to trace worker's
> world as well.
> 
> (I can take a look tomorrow if you're not in a hurry.)

Are you going to merge the fix to M52? If not, I would like to fix the crash
without SetWrapperReferenceTo.

[haraken, 2016-06-02 08:09:44]

On 2016/06/02 08:09:05, yhirano wrote:
> On 2016/06/02 07:47:09, haraken wrote:
> > On 2016/06/02 05:52:52, yhirano wrote:
> > > On 2016/06/02 05:22:05, haraken wrote:
> > > > On 2016/06/02 05:13:38, yhirano wrote:
> > > > > On 2016/06/02 05:06:28, haraken wrote:
> > > > > > On 2016/06/02 05:02:36, yhirano wrote:
> > > > > (snip)
> > > > > > > PS5 still doesn't work (PS3 works, FYI)
> > > > > > 
> > > > > > Where are you crashing?
> > > > > 
> > > > > In fetch-request-with-gc.js, we call fetch with |e.request|. But that
> > > request
> > > > > wrapper is gone while |e| is alive. modules/fetch stores some
> information
> > in
> > > > > that request wrapper and it leads to a crash.
> > > > 
> > > > Can you confirm if DOMWrapperWorld::setWrapperReferencesInAllWorlds is
> > called
> > > > for the event's wrapper?
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > It's called but ScriptWrappable::containsWrapper returns false (and here's
> not
> > > the main thread).
> > > It looks setWrapperReferencesInAllWorlds is not working correctly with
> worker
> > > worlds to me...
> > 
> > Thanks for pointing it out... This is a bug of
> > DOMWrapperWorld::setWrapperReferencesInAllWorlds. We need to trace worker's
> > world as well.
> > 
> > (I can take a look tomorrow if you're not in a hurry.)
> 
> Are you going to merge the fix to M52? If not, I would like to fix the crash
> without SetWrapperReferenceTo.

Good point... Let's go with PS3.

[haraken, 2016-06-02 08:10:27]

PS3 LGTM

[yhirano, 2016-06-02 08:18:56]

falken@, can you take a look at PS6?

[falken, 2016-06-02 08:36:32]

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html:1:
<!DOCTYPE html>
can you put a comment about why this is in chromium/
(something like: this tests that Request is still usable after garbage
collection, which is an implementation detail)

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html:2:
<title>Service Worker: CORS XHR of fetch()</title>
update title

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html:39:
~
:~)

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc-iframe.html
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc-iframe.html:7:
const url = get_host_info()['HTTP_ORIGIN'] + base_path();
do you need the get_host_info stuff? can it just be fetch(base_path())?

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc.js
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc.js:4:
});
indent an additional two
(or make the whole thing one line without braces?) 

slightly prefer resolve instead of res, since res reminds me of response.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:75: // We don't
want the renderer to crash in such cases.
request.isEmpty() is not needed?

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:76: //
TODO(yhirano): Replace this branch with an assertion.
Can you add more details about what's required before this TODO can be done?

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:83: // As
written above, we don't check the return value.
I don't understand this comment, since the above checks the return value.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:84: //
TODO(yhirano): Add an assertion.
Same here.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEvent.cpp
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEvent.cpp:79: //
TODO(yhirano): Add an assertion.
Similar comments here as in FetchEvent (too bad this code is duplicated, is it
worth trying to refactor it out?)

[yhirano, 2016-06-02 10:10:12]

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html:1:
<!DOCTYPE html>
On 2016/06/02 08:36:31, falken wrote:
> can you put a comment about why this is in chromium/
> (something like: this tests that Request is still usable after garbage
> collection, which is an implementation detail)

Done.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html:2:
<title>Service Worker: CORS XHR of fetch()</title>
On 2016/06/02 08:36:31, falken wrote:
> update title

Done.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/fetch-request-with-gc.html:39:
~
On 2016/06/02 08:36:31, falken wrote:
> :~)

Done.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc-iframe.html
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc-iframe.html:7:
const url = get_host_info()['HTTP_ORIGIN'] + base_path();
On 2016/06/02 08:36:31, falken wrote:
> do you need the get_host_info stuff? can it just be fetch(base_path())?

Done.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc.js
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/fetch-request-with-gc.js:4:
});
On 2016/06/02 08:36:32, falken wrote:
> indent an additional two
> (or make the whole thing one line without braces?) 
> 
> slightly prefer resolve instead of res, since res reminds me of response.

Done.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:75: // We don't
want the renderer to crash in such cases.
On 2016/06/02 08:36:32, falken wrote:
> request.isEmpty() is not needed?

It's technically not needed because when request is empty event will be empty
too.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:76: //
TODO(yhirano): Replace this branch with an assertion.
On 2016/06/02 08:36:32, falken wrote:
> Can you add more details about what's required before this TODO can be done?

Done.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:83: // As
written above, we don't check the return value.
On 2016/06/02 08:36:32, falken wrote:
> I don't understand this comment, since the above checks the return value.

I want to put DCHECK but I can't from the same reason as above.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:84: //
TODO(yhirano): Add an assertion.
On 2016/06/02 08:36:32, falken wrote:
> Same here.

Done.

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEvent.cpp
(right):

https://codereview.chromium.org/2025143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEvent.cpp:79: //
TODO(yhirano): Add an assertion.
On 2016/06/02 08:36:32, falken wrote:
> Similar comments here as in FetchEvent (too bad this code is duplicated, is it
> worth trying to refactor it out?)

I think it's OK to be duplicated because I will update the implementation later.
With proper mechanisms this code will be gone (See PS4 or PS5).

[falken, 2016-06-02 10:24:49]

lgtm

https://codereview.chromium.org/2025143003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/2025143003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:86: // mechanism
is introduced.
nit: Maybe clearer as (if I understand correctly): 
// From the same reason as above, setHiddenValue can return false.
// TODO(yhirano): Add an assertion that it returns true when the graceful
shutdown
// mechanism is introduced.

[yhirano, 2016-06-03 02:09:45]

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

https://codereview.chromium.org/2025143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:80: // Sets an
hidden value in order to teach V8 the dependency from
On 2016/06/01 12:16:59, haraken wrote:
> 
> a hidden value

Done.

https://codereview.chromium.org/2025143003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/2025143003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:86: // mechanism
is introduced.
On 2016/06/02 10:24:49, falken wrote:
> nit: Maybe clearer as (if I understand correctly): 
> // From the same reason as above, setHiddenValue can return false.
> // TODO(yhirano): Add an assertion that it returns true when the graceful
> shutdown
> // mechanism is introduced.

Done.

[yhirano, 2016-06-03 02:09:54]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-06-03 02:09:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2025143003/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-06-03 02:11:12]

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

[commit-bot: I haz the power, 2016-06-03 04:20:10]

Description was changed from

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

1: https://crrev.com/d4b43c621eb323605c258a1a7f238bf9d6fb7523

BUG=614272
==========

to

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

1: https://crrev.com/d4b43c621eb323605c258a1a7f238bf9d6fb7523

BUG=614272
==========

[commit-bot: I haz the power, 2016-06-03 04:20:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-03 04:21:29]

Description was changed from

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

1: https://crrev.com/d4b43c621eb323605c258a1a7f238bf9d6fb7523

BUG=614272
==========

to

==========
[Fetch API] Request's JS wrapper should be kept alive in FetchEvent

From the same reason as [1], we need to keep blink::Request's wrapper
in FetchEvent and ForeignEvent's wrappers. This CL adds hidden
values in order to teach v8 the dependency.

1: https://crrev.com/d4b43c621eb323605c258a1a7f238bf9d6fb7523

BUG=614272

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

[commit-bot: I haz the power, 2016-06-03 04:21:30]

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