ServiceWorker: Bypass resolving a promise when ExecutionContext is gone

ServiceWorker: Bypass resolving a promise when ExecutionContext is gone

This change makes CallbackPromiseAdapter bypass resolving/rejecting a promise
if ExecutionContext is gone and calls WebType::dispose() to clean up passed
WebType instance.


BUG=385906, 386501
TEST=run_webkit_tests.py --debug --enable-leak-detection http/tests/serviceworker/chromium/resolve-after-window-close.html
TEST=run_webkit_tests.py --debug --enable-leak-detection push_messaging/push-messaging-resolve-after-detached.html
TEST=manual (run crash.html in the issue)

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179043

[nhiroki, 2014-07-16 09:17:32]

Ptal, thanks!

[yhirano1, 2014-07-17 01:45:00]

+haraken

Should we suppress resolution / rejection in CallbackPromiseAdapter when the
context is empty?
I don't know when the context gets empty, but I imagine the execution context is
stopped in such a case.
If it is true, ScriptPromiseResolver::resolve does nothing in such a case, so we
don't have to call |from| in CallbackPromiseAdapter.

[haraken, 2014-07-17 01:51:33]

On 2014/07/17 01:45:00, yhirano1 wrote:
> +haraken
> 
> Should we suppress resolution / rejection in CallbackPromiseAdapter when the
> context is empty?
> I don't know when the context gets empty, but I imagine the execution context
is
> stopped in such a case.
> If it is true, ScriptPromiseResolver::resolve does nothing in such a case, so
we
> don't have to call |from| in CallbackPromiseAdapter.

Yeah, I think your proposal is a right fix.

A context gets empty when the associated V8 context (i.e., window object) is
gone. At this point the execution context must be stopped. In this case, there
is no way to run any JavaScript code (unless you redirect the JavaScript
execution to another context) so it's meaningless to try to resolve/reject.

[nhiroki, 2014-07-17 04:06:00]

Thank you for your comments!

I chatted with yhirano1@ and we decided to take his proposal.
He'll start to work on it soon (thanks, hirano-san!)

[nhiroki, 2014-07-17 05:57:52]

Hmmm... apparently we cannot skip calling from() in CallbackPromiseAdapter even
when the context is gone because from() takes ownership of WebType instance (ie.
WebServiceWorker) and needs to clean it up if the promise cannot be resolved.

Let me revive this CL.

[nhiroki, 2014-07-17 05:59:00]

falken@: can you take a look?

I　have only a little confidence in this memory management..., but
ServiceWorkerContainer appears to take the same way:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Thanks.

[yhirano, 2014-07-17 06:20:48]

Sorry for the confusion. lgtm of fixing the issue in ServiceWorker::from.

[falken, 2014-07-17 07:49:57]

Can a test be added?

On 2014/07/17 05:59:00, nhiroki wrote:
> falken@: can you take a look?
> 
> I　have only a little confidence in this memory management..., but
> ServiceWorkerContainer appears to take the same way:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Thanks.

Yep, I think it's correct. If there's no proxy then no ServiceWorker owns the
WebServiceWorker so it should be deleted.

[haraken, 2014-07-17 07:53:36]

https://codereview.chromium.org/396963002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/396963002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:159: if
(resolver->scriptState()->contextIsEmpty()) {

Shouldn't this check be if(!resolver->scriptState->executionContext()) ?

It's possible that a V8 context is not empty but the execution context is gone.
I guess what we want to check here is to check the existence of the execution
context.

https://codereview.chromium.org/396963002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:161: delete worker;

I don't understand the memory management here either...

[haraken, 2014-07-17 07:55:26]

On 2014/07/17 07:49:57, falken wrote:
> Can a test be added?
> 
> On 2014/07/17 05:59:00, nhiroki wrote:
> > falken@: can you take a look?
> > 
> > I　have only a little confidence in this memory management..., but
> > ServiceWorkerContainer appears to take the same way:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Thanks.
> 
> Yep, I think it's correct. If there's no proxy then no ServiceWorker owns the
> WebServiceWorker so it should be deleted.

Then why do we not have the similar deletion code in
ServiceWorker::from(ExecutionContext*, WebType*)?

It looks a bit inconsistent where the deletion code should be inserted.

[falken, 2014-07-17 08:26:14]

You're right, it's inconsistent. 

ServiceWorker::from(resolver) is different since it's only called by
CallbackPromiseAdapter. To make things consistent, we could either move the
cleanups from callsites in ServiceWorkerContainer to
ServiceWorker::from(executionContext), or do the cleanup this patch introduces
in a new class ServiceWorkerCallbackPromiseAdapter. I think the latter may be
better as it avoids the needless call to resolve pointed out earlier.

[nhiroki, 2014-07-17 08:30:02]

https://codereview.chromium.org/396963002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/396963002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:159: if
(resolver->scriptState()->contextIsEmpty()) {
On 2014/07/17 07:53:36, haraken wrote:
> 
> Shouldn't this check be if(!resolver->scriptState->executionContext()) ?
> 
> It's possible that a V8 context is not empty but the execution context is
gone.
> I guess what we want to check here is to check the existence of the execution
> context.
> 

ScriptState::executionContext() refers to V8::Context and it crashes if the v8
context is empty. Should we check an existence of the v8 context in
ScriptState::executionContext() ...?

[yhirano1, 2014-07-17 08:32:03]

ScriptPromiseResolver is an ActiveDOMObject, so you can get the ExecutionContext
from it.

[haraken, 2014-07-17 08:34:39]

> ScriptPromiseResolver is an ActiveDOMObject, so you can get the
ExecutionContext
> from it.

Yeah, this is one solution. It's a shame that we have two ways to get
ExecutionContexts (i.e., scriptState->executionContext() and
ActiveDOMObject::executionContext()), and their results can be different.

> ScriptState::executionContext() refers to V8::Context and it crashes if the v8
> context is empty. Should we check an existence of the v8 context in
> ScriptState::executionContext() ...?

Yes, this is a good thing. We can return 0 from toExecutionContext if the passed
context is empty.

[nhiroki, 2014-07-17 08:49:07]

On 2014/07/17 08:34:39, haraken wrote:
> > ScriptPromiseResolver is an ActiveDOMObject, so you can get the
> ExecutionContext
> > from it.
> 
> Yeah, this is one solution. It's a shame that we have two ways to get
> ExecutionContexts (i.e., scriptState->executionContext() and
> ActiveDOMObject::executionContext()), and their results can be different.
> 
> > ScriptState::executionContext() refers to V8::Context and it crashes if the
v8
> > context is empty. Should we check an existence of the v8 context in
> > ScriptState::executionContext() ...?
> 
> Yes, this is a good thing. We can return 0 from toExecutionContext if the
passed
> context is empty.

Thanks! I'll make a separate patch for this and send a review request soon...

[nhiroki, 2014-07-17 08:51:22]

On 2014/07/17 08:26:14, falken wrote:
> You're right, it's inconsistent. 
> 
> ServiceWorker::from(resolver) is different since it's only called by
> CallbackPromiseAdapter. To make things consistent, we could either move the
> cleanups from callsites in ServiceWorkerContainer to
> ServiceWorker::from(executionContext), or do the cleanup this patch introduces
> in a new class ServiceWorkerCallbackPromiseAdapter. I think the latter may be
> better as it avoids the needless call to resolve pointed out earlier.

yhirano1@: what do you think about a new adapter? If looks good to you, I'll try
it.

[nhiroki, 2014-07-18 07:11:03]

Hi, I remade this CL based on the discussion with yhirano1@ and falken@. Can you
take another look?

Patchset3 adds WebType::dispose() instead of new
ServiceWorkerCallbackPromiseAdapter (see falken@'s comment on c#11) because this
way seems more scalable than creating a new adapter per an unusual case.

Thanks!

[yhirano, 2014-07-18 09:00:02]

https://codereview.chromium.org/396963002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/396963002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/CallbackPromiseAdapter.h:89: if (!m_resolver ||
!m_resolver->executionContext()) {
m_resolver cannot be null (you can add ASSERT(m_resolver) in the constructor if
you prefer).
!m_resolver->executionContext() ||
m_resolver->executionContext()->activeDOMObjectsAreStopped() is better.

https://codereview.chromium.org/396963002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/CallbackPromiseAdapter.h:98: if (!m_resolver ||
!m_resolver->executionContext()) {
ditto

[nhiroki, 2014-07-18 10:28:56]

https://codereview.chromium.org/396963002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/396963002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/CallbackPromiseAdapter.h:89: if (!m_resolver ||
!m_resolver->executionContext()) {
On 2014/07/18 09:00:02, yhirano wrote:
> m_resolver cannot be null (you can add ASSERT(m_resolver) in the constructor
if
> you prefer).
> !m_resolver->executionContext() ||
> m_resolver->executionContext()->activeDOMObjectsAreStopped() is better.

Done.

https://codereview.chromium.org/396963002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/CallbackPromiseAdapter.h:98: if (!m_resolver ||
!m_resolver->executionContext()) {
On 2014/07/18 09:00:02, yhirano wrote:
> ditto

Done.

[yhirano, 2014-07-22 05:56:22]

lgtm

[dominicc (has gone to gerrit), 2014-07-22 07:05:24]

LGTM

[nhiroki, 2014-07-23 00:00:35]

+mvanouwerkerk@, could you review Source/modules/push_messaging/* as an OWNER?
Thanks.

[nhiroki, 2014-07-24 13:06:20]

On 2014/07/23 00:00:35, nhiroki wrote:
> +mvanouwerkerk@, could you review Source/modules/push_messaging/* as an OWNER?
> Thanks.

mvanouwerkerk@: Friendly ping.

[Michael van Ouwerkerk, 2014-07-24 14:38:51]

Thanks for fixing this!

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html
(right):

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html:6:
description('Test that resolving a promise after the window gets closed does not
assert or crash');
Could you add a similar test for Push Messaging?

https://codereview.chromium.org/396963002/diff/80001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistration.h (right):

https://codereview.chromium.org/396963002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistration.h:27: static void
dispose(WebType* registrationRaw)
Why is the dispose method inlined in PushRegistration, but not in PushError? It
seems inconsistent.

[nhiroki, 2014-07-24 15:52:32]

Thank you for reviewing!

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html
(right):

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html:6:
description('Test that resolving a promise after the window gets closed does not
assert or crash');
On 2014/07/24 14:38:51, Michael van Ouwerkerk wrote:
> Could you add a similar test for Push Messaging?

I couldn't find a place of layout tests for Push Messaging. Where can I put the
test?

https://codereview.chromium.org/396963002/diff/80001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistration.h (right):

https://codereview.chromium.org/396963002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistration.h:27: static void
dispose(WebType* registrationRaw)
On 2014/07/24 14:38:51, Michael van Ouwerkerk wrote:
> Why is the dispose method inlined in PushRegistration, but not in PushError?
It
> seems inconsistent.

Moved from() & dispose() to *.cpp.

I meant to make consistent with from() which is also used for
CallbackPromiseAdapter.

[nhiroki, 2014-07-24 15:55:33]

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html
(right):

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html:6:
description('Test that resolving a promise after the window gets closed does not
assert or crash');
On 2014/07/24 15:52:31, nhiroki wrote:
> On 2014/07/24 14:38:51, Michael van Ouwerkerk wrote:
> > Could you add a similar test for Push Messaging?
> 
> I couldn't find a place of layout tests for Push Messaging. Where can I put
the
> test?

Oops... sorry, I just found "WebKit/LayoutTests/push_messaging". I'll add a
test.

[nhiroki, 2014-07-24 16:35:39]

mvanouwerkerk@: Could you take another look?

[nhiroki, 2014-07-24 16:35:52]

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html
(right):

https://codereview.chromium.org/396963002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resolve-after-window-close.html:6:
description('Test that resolving a promise after the window gets closed does not
assert or crash');
On 2014/07/24 15:55:33, nhiroki wrote:
> On 2014/07/24 15:52:31, nhiroki wrote:
> > On 2014/07/24 14:38:51, Michael van Ouwerkerk wrote:
> > > Could you add a similar test for Push Messaging?
> > 
> > I couldn't find a place of layout tests for Push Messaging. Where can I put
> the
> > test?
> 
> Oops... sorry, I just found "WebKit/LayoutTests/push_messaging". I'll add a
> test.

Done.

[nhiroki, 2014-07-28 09:34:00]

mvanouwerkerk@: Friendly ping. I'd like to commit this since some issues to be
fixed by this CL have been reported.

[Michael van Ouwerkerk, 2014-07-28 10:42:55]

lgtm thanks!

[nhiroki, 2014-07-28 14:46:19]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-07-28 14:47:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/396963002/140001

[commit-bot: I haz the power, 2014-07-28 15:58:14]

Change committed as 179043