Don't assert when ServiceWorker::from is passed an empty promise

Don't assert when ServiceWorker::from is passed an empty promise

If the document gets detached before Service Worker registration
completes, the CallbackPromiseAdapter promise gets cleared. Then when
registration completes, ServiceWorker::from is passed the resolver with
the empty promise. Before this patch, an assert would then occur because
ScriptPromise::then bails before adopting its fulfilled/rejected
arguments. This patch fixes an overly aggressive assert in
V8GarbageCollected and also makes ServiceWorker creation skip waiting
on an empty promise.

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

[falken, 2014-06-16 07:51:08]

yhirano@ and dominicc@ could you both look at this please?

[dominicc (has gone to gerrit), 2014-06-17 00:40:44]

LGTM

You need to adopt the script functions earlier because why?

[haraken, 2014-06-17 00:51:55]

https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
File Source/bindings/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromise.cpp:71: v8::Local<v8::Function> v8OnRejected =
ScriptFunction::adoptByGarbageCollector(onRejected);

Would you elaborate on this change? It seems a bit strange to me that just
adopting ScriptFunctions addresses some issue. Also in a case where a
ScriptPromise is empty, I wonder why we need to adopt ScriptFunctions.

https://codereview.chromium.org/337053004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/337053004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorker.cpp:160: if
(!resolver->promise().isEmpty())

Shall we move this check into ServiceWorker::waitOnPromise so that we don't need
to add the same check for all call sites (although we currently don't have any
other call site of waitOnPromise)?

[falken, 2014-06-17 01:07:52]

https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
File Source/bindings/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromise.cpp:71: v8::Local<v8::Function> v8OnRejected =
ScriptFunction::adoptByGarbageCollector(onRejected);
On 2014/06/17 00:51:55, haraken wrote:
> 
> Would you elaborate on this change? It seems a bit strange to me that just
> adopting ScriptFunctions addresses some issue. Also in a case where a
> ScriptPromise is empty, I wonder why we need to adopt ScriptFunctions.
> 

The destructor of PassOwnPtr deletes the ScriptFunction, but ScriptFunction
inherits V8GarbageCollected whose dtor will ASSERT that m_handle was set. AFAICT
the way to set m_handle is to call adoptbyGarbageCollector.

So I think the caller expects ScriptPromise::then to always adopt the args.

https://codereview.chromium.org/337053004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/337053004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorker.cpp:160: if
(!resolver->promise().isEmpty())
On 2014/06/17 00:51:55, haraken wrote:
> 
> Shall we move this check into ServiceWorker::waitOnPromise so that we don't
need
> to add the same check for all call sites (although we currently don't have any
> other call site of waitOnPromise)?

Good idea, I've done this.

[haraken, 2014-06-17 01:15:25]

https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
File Source/bindings/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromise.cpp:71: v8::Local<v8::Function> v8OnRejected =
ScriptFunction::adoptByGarbageCollector(onRejected);
On 2014/06/17 01:07:51, falken wrote:
> On 2014/06/17 00:51:55, haraken wrote:
> > 
> > Would you elaborate on this change? It seems a bit strange to me that just
> > adopting ScriptFunctions addresses some issue. Also in a case where a
> > ScriptPromise is empty, I wonder why we need to adopt ScriptFunctions.
> > 
> 
> The destructor of PassOwnPtr deletes the ScriptFunction, but ScriptFunction
> inherits V8GarbageCollected whose dtor will ASSERT that m_handle was set.
AFAICT
> the way to set m_handle is to call adoptbyGarbageCollector.
> 
> So I think the caller expects ScriptPromise::then to always adopt the args.

Thanks for the details; that ASSERT doesn't make much sense to me. Can we change
the ASSERT to something like ASSERT(alreadyAdopted || m_handle) ?

[falken, 2014-06-17 02:06:43]

On 2014/06/17 01:15:25, haraken wrote:
>
https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
> File Source/bindings/v8/ScriptPromise.cpp (right):
> 
>
https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
> Source/bindings/v8/ScriptPromise.cpp:71: v8::Local<v8::Function> v8OnRejected
=
> ScriptFunction::adoptByGarbageCollector(onRejected);
> On 2014/06/17 01:07:51, falken wrote:
> > On 2014/06/17 00:51:55, haraken wrote:
> > > 
> > > Would you elaborate on this change? It seems a bit strange to me that just
> > > adopting ScriptFunctions addresses some issue. Also in a case where a
> > > ScriptPromise is empty, I wonder why we need to adopt ScriptFunctions.
> > > 
> > 
> > The destructor of PassOwnPtr deletes the ScriptFunction, but ScriptFunction
> > inherits V8GarbageCollected whose dtor will ASSERT that m_handle was set.
> AFAICT
> > the way to set m_handle is to call adoptbyGarbageCollector.
> > 
> > So I think the caller expects ScriptPromise::then to always adopt the args.
> 
> Thanks for the details; that ASSERT doesn't make much sense to me. Can we
change
> the ASSERT to something like ASSERT(alreadyAdopted || m_handle) ?

I mispoke. The current ASSERT is m_handle.isEmpty(). The weakCallback passed to
m_handle.setWeak() in releaseToV8GarbageCollector is what clears the handle.
Should we do something like ASSERT(!adoptedByV8GarbageCollector ||
m_handle.isEmpty())?

[falken, 2014-06-17 02:16:14]

On 2014/06/17 02:06:43, falken wrote:
> On 2014/06/17 01:15:25, haraken wrote:
> >
>
https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
> > File Source/bindings/v8/ScriptPromise.cpp (right):
> > 
> >
>
https://codereview.chromium.org/337053004/diff/1/Source/bindings/v8/ScriptPro...
> > Source/bindings/v8/ScriptPromise.cpp:71: v8::Local<v8::Function>
v8OnRejected
> =
> > ScriptFunction::adoptByGarbageCollector(onRejected);
> > On 2014/06/17 01:07:51, falken wrote:
> > > On 2014/06/17 00:51:55, haraken wrote:
> > > > 
> > > > Would you elaborate on this change? It seems a bit strange to me that
just
> > > > adopting ScriptFunctions addresses some issue. Also in a case where a
> > > > ScriptPromise is empty, I wonder why we need to adopt ScriptFunctions.
> > > > 
> > > 
> > > The destructor of PassOwnPtr deletes the ScriptFunction, but
ScriptFunction
> > > inherits V8GarbageCollected whose dtor will ASSERT that m_handle was set.
> > AFAICT
> > > the way to set m_handle is to call adoptbyGarbageCollector.
> > > 
> > > So I think the caller expects ScriptPromise::then to always adopt the
args.
> > 
> > Thanks for the details; that ASSERT doesn't make much sense to me. Can we
> change
> > the ASSERT to something like ASSERT(alreadyAdopted || m_handle) ?
> 
> I mispoke. The current ASSERT is m_handle.isEmpty(). The weakCallback passed
to
> m_handle.setWeak() in releaseToV8GarbageCollector is what clears the handle.
> Should we do something like ASSERT(!adoptedByV8GarbageCollector ||
> m_handle.isEmpty())?

^^^ Did that in newest patch.

[haraken, 2014-06-17 02:46:25]

https://codereview.chromium.org/337053004/diff/40001/Source/bindings/v8/V8Gar...
File Source/bindings/v8/V8GarbageCollected.h (right):

https://codereview.chromium.org/337053004/diff/40001/Source/bindings/v8/V8Gar...
Source/bindings/v8/V8GarbageCollected.h:69:
ASSERT(!m_releasedToV8GarbageCollector || m_handle.isEmpty());

Can we use m_handle.IsWeak() instead of introducing
m_releasedToV8GarbageCollector? I think you can use ASSERT(m_handle.IsWeak() ||
m_handle.IsEmpty()).

(I'm fine with this CL, but we should remove V8GarbageCollected completely.
V8GarbageCollected is used only by ScriptFunction, and it's just adding a
complexity of unnecessary abstraction.)

[falken, 2014-06-17 03:43:05]

https://codereview.chromium.org/337053004/diff/40001/Source/bindings/v8/V8Gar...
File Source/bindings/v8/V8GarbageCollected.h (right):

https://codereview.chromium.org/337053004/diff/40001/Source/bindings/v8/V8Gar...
Source/bindings/v8/V8GarbageCollected.h:69:
ASSERT(!m_releasedToV8GarbageCollector || m_handle.isEmpty());
On 2014/06/17 02:46:24, haraken wrote:
> 
> Can we use m_handle.IsWeak() instead of introducing
> m_releasedToV8GarbageCollector? I think you can use ASSERT(m_handle.IsWeak()
||
> m_handle.IsEmpty()).

No, m_handle.IsWeak() is false until releaseToV8GarbageCollector() is called. It
should really be (!m_handle.IsWeak() || m_handle.IsEmpty()) but I don't think
that makes sense (I would think m_handle.IsWeak implies it's not empty).

> (I'm fine with this CL, but we should remove V8GarbageCollected completely.
> V8GarbageCollected is used only by ScriptFunction, and it's just adding a
> complexity of unnecessary abstraction.)

Yeah, I wonder if other classes were planning to use it too?

I also updated the ServiceWorker.cpp bit after talking with yhirano, who pointed
out it's confusing that ServiceWorker continues as normal with the empty
promise. I added a comment and changed the internal state to ContextStopped in
that case.

If things still look OK I'd like to land the patch.

[haraken, 2014-06-17 03:46:03]

On 2014/06/17 03:43:05, falken wrote:
>
https://codereview.chromium.org/337053004/diff/40001/Source/bindings/v8/V8Gar...
> File Source/bindings/v8/V8GarbageCollected.h (right):
> 
>
https://codereview.chromium.org/337053004/diff/40001/Source/bindings/v8/V8Gar...
> Source/bindings/v8/V8GarbageCollected.h:69:
> ASSERT(!m_releasedToV8GarbageCollector || m_handle.isEmpty());
> On 2014/06/17 02:46:24, haraken wrote:
> > 
> > Can we use m_handle.IsWeak() instead of introducing
> > m_releasedToV8GarbageCollector? I think you can use ASSERT(m_handle.IsWeak()
> ||
> > m_handle.IsEmpty()).
> 
> No, m_handle.IsWeak() is false until releaseToV8GarbageCollector() is called.
It
> should really be (!m_handle.IsWeak() || m_handle.IsEmpty()) but I don't think
> that makes sense (I would think m_handle.IsWeak implies it's not empty).
> 
> > (I'm fine with this CL, but we should remove V8GarbageCollected completely.
> > V8GarbageCollected is used only by ScriptFunction, and it's just adding a
> > complexity of unnecessary abstraction.)
> 
> Yeah, I wonder if other classes were planning to use it too?
> 
> I also updated the ServiceWorker.cpp bit after talking with yhirano, who
pointed
> out it's confusing that ServiceWorker continues as normal with the empty
> promise. I added a comment and changed the internal state to ContextStopped in
> that case.
> 
> If things still look OK I'd like to land the patch.

LGTM for the CL.

I'm not really happy to increase the users of V8GarbageCollected, since it's
hard to use it correctly without causing memory leaks. However, that's a
separate discussion.

[yhirano, 2014-06-17 03:47:45]

lgtm

[falken, 2014-06-17 03:47:50]

Thanks for the reviews!

[falken, 2014-06-17 03:48:16]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-06-17 03:49:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/337053004/60001

[falken, 2014-06-17 03:50:30]

The CQ bit was unchecked by falken@chromium.org

[falken, 2014-06-17 03:52:10]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-06-17 03:53:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/337053004/60001

[commit-bot: I haz the power, 2014-06-17 04:57:39]

Change committed as 176282