Stop calling [de]ref in ScriptPromiseResolverWithContext destructor.

Stop calling [de]ref in ScriptPromiseResolverWithContext destructor.

The ScriptPromiseResolverWithContext destructor is checking if it is misused,
but the current code calls ref() and deref(), which are  invalid in the destructor.
This CL replaces the implementation with a simple assertion.

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

[haraken, 2014-06-19 08:06:12]

> ScriptPromiserResolverWithContext rejects the associated Promise in case
> it is destructed without calling resolve / reject and the associated Promise
> is passed to the JavaScript world (it's a bug).

I think we need to improve the ScriptPromiseResolver and fix the bug, instead of
hiding the bug with this CL.

[yhirano, 2014-06-19 08:09:11]

It's user's bug, not ScriptPromiseResolverWithContext's.
The code in the destructor helps to find it, I hope.

[haraken, 2014-06-19 08:15:29]

On 2014/06/19 08:09:11, yhirano wrote:
> It's user's bug, not ScriptPromiseResolverWithContext's.
> The code in the destructor helps to find it, I hope.

I'm a bit confused about the meaning of the "bug"... In terms of the spec, how
should we handle the case where a Promise is destructed without resolve/reject
being called?

[yhirano, 2014-06-19 08:23:09]

On 2014/06/19 08:15:29, haraken wrote:
> On 2014/06/19 08:09:11, yhirano wrote:
> > It's user's bug, not ScriptPromiseResolverWithContext's.
> > The code in the destructor helps to find it, I hope.
> 
> I'm a bit confused about the meaning of the "bug"... In terms of the spec, how
> should we handle the case where a Promise is destructed without resolve/reject
> being called?

The handlers waiting for the Promise will never be executed.
But, as a resolver in Blink, I think detaching a resolver without calling
resolve / reject AND
exposing the Promise to the JavaScript world never be desirable.
The destructor code helps to detect it.

Do you think deleting the destructor code is better?

[haraken, 2014-06-19 08:34:09]

On 2014/06/19 08:23:09, yhirano wrote:
> On 2014/06/19 08:15:29, haraken wrote:
> > On 2014/06/19 08:09:11, yhirano wrote:
> > > It's user's bug, not ScriptPromiseResolverWithContext's.
> > > The code in the destructor helps to find it, I hope.
> > 
> > I'm a bit confused about the meaning of the "bug"... In terms of the spec,
how
> > should we handle the case where a Promise is destructed without
resolve/reject
> > being called?
> 
> The handlers waiting for the Promise will never be executed.
> But, as a resolver in Blink, I think detaching a resolver without calling
> resolve / reject AND
> exposing the Promise to the JavaScript world never be desirable.
> The destructor code helps to detect it.
> 
> Do you think deleting the destructor code is better?

I guess a better fix would be to use RELEASE_ASSERT(m_state != Pending) in
Resolver's destructor and fix all bugs that hit the RELEASE_ASSERTs. What should
not happen should not happen, and I don't think it's a good idea to allow it
with some error message.

Is this a speculative fix? Or if we change it to the RELEASE_ASSERT, will we hit
the RELEASE_ASSERT in production builds?

[yhirano, 2014-06-19 09:03:49]

On 2014/06/19 08:34:09, haraken wrote:
> On 2014/06/19 08:23:09, yhirano wrote:
> > On 2014/06/19 08:15:29, haraken wrote:
> > > On 2014/06/19 08:09:11, yhirano wrote:
> > > > It's user's bug, not ScriptPromiseResolverWithContext's.
> > > > The code in the destructor helps to find it, I hope.
> > > 
> > > I'm a bit confused about the meaning of the "bug"... In terms of the spec,
> how
> > > should we handle the case where a Promise is destructed without
> resolve/reject
> > > being called?
> > 
> > The handlers waiting for the Promise will never be executed.
> > But, as a resolver in Blink, I think detaching a resolver without calling
> > resolve / reject AND
> > exposing the Promise to the JavaScript world never be desirable.
> > The destructor code helps to detect it.
> > 
> > Do you think deleting the destructor code is better?
> 
> I guess a better fix would be to use RELEASE_ASSERT(m_state != Pending) in
> Resolver's destructor and fix all bugs that hit the RELEASE_ASSERTs. What
should
> not happen should not happen, and I don't think it's a good idea to allow it
> with some error message.
> 
> Is this a speculative fix? Or if we change it to the RELEASE_ASSERT, will we
hit
> the RELEASE_ASSERT in production builds?


Hmm. I would permit to kill a pending resolver if it doesn't expose the Promise
to the JavaScript world. The simplest example is
{
  ScriptPromiseResolverWithContext::create(scriptState);
}
.

I deleted the destructor code at PS3.

[haraken, 2014-06-19 09:07:56]

On 2014/06/19 09:03:49, yhirano wrote:
> On 2014/06/19 08:34:09, haraken wrote:
> > On 2014/06/19 08:23:09, yhirano wrote:
> > > On 2014/06/19 08:15:29, haraken wrote:
> > > > On 2014/06/19 08:09:11, yhirano wrote:
> > > > > It's user's bug, not ScriptPromiseResolverWithContext's.
> > > > > The code in the destructor helps to find it, I hope.
> > > > 
> > > > I'm a bit confused about the meaning of the "bug"... In terms of the
spec,
> > how
> > > > should we handle the case where a Promise is destructed without
> > resolve/reject
> > > > being called?
> > > 
> > > The handlers waiting for the Promise will never be executed.
> > > But, as a resolver in Blink, I think detaching a resolver without calling
> > > resolve / reject AND
> > > exposing the Promise to the JavaScript world never be desirable.
> > > The destructor code helps to detect it.
> > > 
> > > Do you think deleting the destructor code is better?
> > 
> > I guess a better fix would be to use RELEASE_ASSERT(m_state != Pending) in
> > Resolver's destructor and fix all bugs that hit the RELEASE_ASSERTs. What
> should
> > not happen should not happen, and I don't think it's a good idea to allow it
> > with some error message.
> > 
> > Is this a speculative fix? Or if we change it to the RELEASE_ASSERT, will we
> hit
> > the RELEASE_ASSERT in production builds?
> 
> 
> Hmm. I would permit to kill a pending resolver if it doesn't expose the
Promise
> to the JavaScript world. The simplest example is
> {
>   ScriptPromiseResolverWithContext::create(scriptState);
> }
> .
> 
> I deleted the destructor code at PS3.

Then what we need is to add something like:

  RELEASE_ASSERT(!m_promiseCreated || m_state != Pending);

to the destructor, isn't it?

Either way, we might want to make it clear what can happen and what cannot
happen, and implement the code so that what cannot happen cannot happen.

[yhirano, 2014-06-19 11:56:43]

On 2014/06/19 09:07:56, haraken wrote:
> On 2014/06/19 09:03:49, yhirano wrote:
> > On 2014/06/19 08:34:09, haraken wrote:
> > > On 2014/06/19 08:23:09, yhirano wrote:
> > > > On 2014/06/19 08:15:29, haraken wrote:
> > > > > On 2014/06/19 08:09:11, yhirano wrote:
> > > > > > It's user's bug, not ScriptPromiseResolverWithContext's.
> > > > > > The code in the destructor helps to find it, I hope.
> > > > > 
> > > > > I'm a bit confused about the meaning of the "bug"... In terms of the
> spec,
> > > how
> > > > > should we handle the case where a Promise is destructed without
> > > resolve/reject
> > > > > being called?
> > > > 
> > > > The handlers waiting for the Promise will never be executed.
> > > > But, as a resolver in Blink, I think detaching a resolver without
calling
> > > > resolve / reject AND
> > > > exposing the Promise to the JavaScript world never be desirable.
> > > > The destructor code helps to detect it.
> > > > 
> > > > Do you think deleting the destructor code is better?
> > > 
> > > I guess a better fix would be to use RELEASE_ASSERT(m_state != Pending) in
> > > Resolver's destructor and fix all bugs that hit the RELEASE_ASSERTs. What
> > should
> > > not happen should not happen, and I don't think it's a good idea to allow
it
> > > with some error message.
> > > 
> > > Is this a speculative fix? Or if we change it to the RELEASE_ASSERT, will
we
> > hit
> > > the RELEASE_ASSERT in production builds?
> > 
> > 
> > Hmm. I would permit to kill a pending resolver if it doesn't expose the
> Promise
> > to the JavaScript world. The simplest example is
> > {
> >   ScriptPromiseResolverWithContext::create(scriptState);
> > }
> > .
> > 
> > I deleted the destructor code at PS3.
> 
> Then what we need is to add something like:
> 
>   RELEASE_ASSERT(!m_promiseCreated || m_state != Pending);
> 
> to the destructor, isn't it?
> 
> Either way, we might want to make it clear what can happen and what cannot
> happen, and implement the code so that what cannot happen cannot happen.

Done.

[haraken, 2014-06-19 11:58:49]

LGTM, thanks!

[yhirano, 2014-06-19 12:35:59]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-06-19 12:36:18]

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

[commit-bot: I haz the power, 2014-06-19 14:15:00]

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

[commit-bot: I haz the power, 2014-06-19 14:15:01]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[yhirano, 2014-06-19 14:32:22]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-06-19 14:33:16]

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

[commit-bot: I haz the power, 2014-06-19 15:35:41]

Change committed as 176518